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

Converting node_cron_comments_scale.

Files: 
CommentFileSizeAuthor
#18 comment-scale-state-1831486-18.patch3.41 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 48,038 pass(es).
[ View ]
#15 1831486_convert_comment_variables_with_tests_and_uninstall_15.patch3.37 KBmiro_dietiker
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1831486_convert_comment_variables_with_tests_and_uninstall_15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 1831486_convert_comment_variables_with_tests_and_uninstall_12.patch3.05 KBmiro_dietiker
PASSED: [[SimpleTest]]: [MySQL] 47,839 pass(es).
[ View ]
#11 1831486_convert_comment_variables_with_tests_and_uninstall_11.patch3.06 KBmiro_dietiker
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/tests/upgrade/drupal-7.state.system.database.php.
[ View ]
#6 1831486_convert_comment_variables_with_tests_and_uninstall.patch3.38 KBmiro_dietiker
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1831486_convert_comment_variables_with_tests_and_uninstall.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 1831486_convert_comment_variables_with_tests.patch3.06 KBmiro_dietiker
PASSED: [[SimpleTest]]: [MySQL] 47,565 pass(es).
[ View ]
#1 1831486_convert_comment_variables.patch1.66 KBmiro_dietiker
PASSED: [[SimpleTest]]: [MySQL] 47,561 pass(es).
[ View ]

Comments

miro_dietiker’s picture

Status:Active» Needs review
StatusFileSize
new1.66 KB
PASSED: [[SimpleTest]]: [MySQL] 47,561 pass(es).
[ View ]

First try!

miro_dietiker’s picture

StatusFileSize
new3.06 KB
PASSED: [[SimpleTest]]: [MySQL] 47,565 pass(es).
[ View ]

Now with test coverage.

alexpott’s picture

Issue tags:+Configuration system

Tagging...

aspilicious’s picture

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

@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

StatusFileSize
new3.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1831486_convert_comment_variables_with_tests_and_uninstall.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs work» Needs review
Berdir’s picture

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

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

Status:Needs work» Needs review
StatusFileSize
new3.06 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/tests/upgrade/drupal-7.state.system.database.php.
[ View ]

Rerolled.

Status:Needs review» Needs work
miro_dietiker’s picture

Status:Needs work» Needs review
StatusFileSize
new3.05 KB
PASSED: [[SimpleTest]]: [MySQL] 47,839 pass(es).
[ View ]

Bad merge :-) retry!

alexpott’s picture

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

Status:Needs work» Needs review
StatusFileSize
new3.37 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1831486_convert_comment_variables_with_tests_and_uninstall_15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Now #6 rerolled cleanly.

Berdir’s picture

Status:Needs review» Needs work
Issue tags:+Configuration system
Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new3.41 KB
PASSED: [[SimpleTest]]: [MySQL] 48,038 pass(es).
[ View ]

Re-rolled.

aspilicious’s picture

Status:Needs review» Reviewed & tested by the community

Looks good now

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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