Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LinL’s picture

Here's the patch.

LinL’s picture

Status: Active » Needs review

Ooops, forgot status.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +State system

Tagging...

Tested. Looks good. Thanks!

nod_’s picture

Issue tags: +JavaScript

tagging too.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Actually just realised that we should be cleaning up the variables table on upgrade! :)

So in this case we need a system_update_N function to do a update_variable_del('javascript_parsed')

LinL’s picture

Status: Needs work » Needs review
FileSize
4.23 KB

Updated patch to include cleanup. I've used system_update_8022() and there are 2 other pending patches using 8022, so may need a re-roll depending which ones land first.

alexpott’s picture

Thanks for the progress on this.

+++ b/core/modules/system/system.installundefined
@@ -1976,6 +1976,13 @@ function system_update_8021() {
 /**
+ * Clean up javascript_parsed variable.
+ */

Just a minor nitpick but the text here should be "Cleans up javascript_parsed variable." See http://drupal.org/node/1354#functions. Also can you add a doxygen tag to the doc block - so that we can easily find all the state sub system conversions. So the final doc block should lo0k something like this:

/**
 * Cleans up javascript_parsed variable.
 *
 * @ingroup system_upgrade
 */

Leaving at need review so other people will review the patch.

LinL’s picture

Thanks for all your help, alexpott. Here's an updated patch.

alexpott’s picture

Status: Needs review » Needs work

We've decided to namespace the key names so that it's easy to identify the "owner"... so we need to change the variable name to from javascript_parsed to system.javascript_parsed

LinL’s picture

Status: Needs work » Needs review
FileSize
4.46 KB

Here's an updated patch with variable name changed to system.javascript_parsed.

Some questions:

Should I change references to javascript_parsed in /core/modules/system/tests/upgrade/drupal-7.language.database.php or do they refer to the old D7 variable?

Would it be better to change the name of javascript_parsed_count in /core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php to system.javascript_parsed_count?

And the update function system_update_8022 may need its N updating prior to commit.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

@LinL thanks for all the work.

In answer to your questions:

Yep it's the old D7 variable - should be left alone.

No need to change the variable name in the code. The reason we have added the system. to the front of the name in the state table is to make it easy to identify the owner - just by looking at the table data. This is not an issue with php variable names.

This is good to go!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Actually the system_update_8022 already needs an update as the routing patch has landed.

LinL’s picture

Status: Needs work » Needs review
FileSize
4.46 KB

@alexpott Thanks for the review and clarification.

Here's a re-roll.

LinL’s picture

Re-roll for update to system_update_8024.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
4.45 KB

Bumped update to system_update_8026. No commit credit necessary. This looks good.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks great! Committed and pushed to 8.x (after incrementing that number once again to 8027 after #1798732: Convert install_task, install_time and install_current_batch to use the state system). Thanks!

Also, alex, I decided to credit you in both patches because you're helping mentor people on how to use the state system. :)

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