Referring to
#1775842: [meta] Convert all variables to state and/or config systems

Converting node_cron_comments_scale.

CommentFileSizeAuthor
#18 comment-scale-state-1831486-18.patch3.41 KBberdir
#15 1831486_convert_comment_variables_with_tests_and_uninstall_15.patch3.37 KBmiro_dietiker
#13 1831486_convert_comment_variables_with_tests_and_uninstall_12.patch3.05 KBmiro_dietiker
#11 1831486_convert_comment_variables_with_tests_and_uninstall_11.patch3.06 KBmiro_dietiker
#6 1831486_convert_comment_variables_with_tests_and_uninstall.patch3.38 KBmiro_dietiker
#2 1831486_convert_comment_variables_with_tests.patch3.06 KBmiro_dietiker
#1 1831486_convert_comment_variables.patch1.66 KBmiro_dietiker

Comments

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new 1831486_convert_comment_variables.patch1.66 KB

First try!

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
StatusFileSize
new 1831486_convert_comment_variables_with_tests.patch3.06 KB

Now with test coverage.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: +Configuration system

Tagging...

aspilicious’s picture

aspilicious commented
Status: Needs review » Needs work

hmm in the state api you can't just replace 'node_cron_comments_scale' with 'comment.count_scale'. Because nobody will know it has something to do with nodes on cron. You should just replace variable_set/variable_get with state()->set and state()->get.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

@aspilicious: We can't just replace variable_get() with state, because are required to prefix it with the module. So it would be 'comment.node_cron_comments_scale'. Which is very long. And IMHO wrong, because it doesn't have to do with nodes on cron. It is based on the highest count of comments a single node has.

So maybe comment.count_scale is too short, but I'm not sure what would be better.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
StatusFileSize
new 1831486_convert_comment_variables_with_tests_and_uninstall.patch3.38 KB

Providing new version. Now with uninstall.

SELECT MAX(comment_count) FROM {node_comment_statistics}

Renamed to comment.node_comment_statistics_scale since it's a query against {node_comment_statistics}. And yes, nothing to do with cron.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs work » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

This looks good to me. The state name is now consistent with the meaning IMHO, makes sense to use the same as the table that we're calculating the scale for.

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: -Configuration system

#6: 1831486_convert_comment_variables_with_tests_and_uninstall.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Configuration system

The last submitted patch, 1831486_convert_comment_variables_with_tests_and_uninstall.patch, failed testing.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new 1831486_convert_comment_variables_with_tests_and_uninstall_11.patch3.06 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 1831486_convert_comment_variables_with_tests_and_uninstall_11.patch, failed testing.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new 1831486_convert_comment_variables_with_tests_and_uninstall_12.patch3.05 KB

Bad merge :-) retry!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

State is not automatically cleaned up on uninstall (at the moment) so we should add a state('')->delete() to comment_uninstall(). And yep this was missing for the current variable.

Otherwise looks good.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new 1831486_convert_comment_variables_with_tests_and_uninstall_15.patch3.37 KB

Hmm... #6 had uninstall // delete already. So reroll #11 / #13 where wrong.

Now #6 rerolled cleanly.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: -Configuration system

#15: 1831486_convert_comment_variables_with_tests_and_uninstall_15.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, 1831486_convert_comment_variables_with_tests_and_uninstall_15.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new comment-scale-state-1831486-18.patch3.41 KB

Re-rolled.

aspilicious’s picture

aspilicious commented
Status: Needs review » Reviewed & tested by the community

Looks good now

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.