Support from Acquia helps fund testing for Drupal Acquia logo

Comments

miro_dietiker’s picture

Status: Active » Needs review
FileSize
1.66 KB

First try!

miro_dietiker’s picture

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

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
FileSize
3.06 KB

Rerolled.

Status: Needs review » Needs work
miro_dietiker’s picture

Status: Needs work » Needs review
FileSize
3.05 KB

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
FileSize
3.37 KB

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
FileSize
3.41 KB

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.