Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Referring to
#1775842: [meta] Convert all variables to state and/or config systems
Converting node_cron_comments_scale.
Comments
Comment #1
miro_dietikerFirst try!
Comment #2
miro_dietikerNow with test coverage.
Comment #3
alexpottTagging...
Comment #4
aspilicious CreditAttribution: aspilicious commentedhmm 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.
Comment #5
Berdir@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.
Comment #6
miro_dietikerProviding new version. Now with uninstall.
Renamed to comment.node_comment_statistics_scale since it's a query against {node_comment_statistics}. And yes, nothing to do with cron.
Comment #7
miro_dietikerComment #8
BerdirThis 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.
Comment #9
catch#6: 1831486_convert_comment_variables_with_tests_and_uninstall.patch queued for re-testing.
Comment #11
miro_dietikerRerolled.
Comment #13
miro_dietikerBad merge :-) retry!
Comment #14
alexpottState is not automatically cleaned up on uninstall (at the moment) so we should add a
state('')->delete()
tocomment_uninstall()
. And yep this was missing for the current variable.Otherwise looks good.
Comment #15
miro_dietikerHmm... #6 had uninstall // delete already. So reroll #11 / #13 where wrong.
Now #6 rerolled cleanly.
Comment #16
Berdir#15: 1831486_convert_comment_variables_with_tests_and_uninstall_15.patch queued for re-testing.
Comment #18
BerdirRe-rolled.
Comment #19
aspilicious CreditAttribution: aspilicious commentedLooks good now
Comment #20
webchickCommitted and pushed to 8.x. Thanks!