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.
Comment | File | Size | Author |
---|---|---|---|
#15 | javascript_parsed-1798796-15.patch | 4.45 KB | alexpott |
#14 | javascript_parsed-1798796-14.patch | 4.45 KB | LinL |
#13 | javascript_parsed-1798796-13.patch | 4.46 KB | LinL |
#10 | javascript_parsed-1798796-10.patch | 4.46 KB | LinL |
#8 | javascript_parsed-1798796-8.patch | 4.26 KB | LinL |
Comments
Comment #1
LinL CreditAttribution: LinL commentedHere's the patch.
Comment #2
LinL CreditAttribution: LinL commentedOoops, forgot status.
Comment #3
alexpottTagging...
Tested. Looks good. Thanks!
Comment #4
nod_tagging too.
Comment #5
alexpottActually 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')
Comment #6
LinL CreditAttribution: LinL commentedUpdated 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.
Comment #7
alexpottThanks for the progress on this.
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:
Leaving at need review so other people will review the patch.
Comment #8
LinL CreditAttribution: LinL commentedThanks for all your help, alexpott. Here's an updated patch.
Comment #9
alexpottWe'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
tosystem.javascript_parsed
Comment #10
LinL CreditAttribution: LinL commentedHere'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
tosystem.javascript_parsed_count
?And the update function
system_update_8022
may need its N updating prior to commit.Comment #11
alexpott@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!
Comment #12
alexpottActually the system_update_8022 already needs an update as the routing patch has landed.
Comment #13
LinL CreditAttribution: LinL commented@alexpott Thanks for the review and clarification.
Here's a re-roll.
Comment #14
LinL CreditAttribution: LinL commentedRe-roll for update to system_update_8024.
Comment #15
alexpottBumped update to system_update_8026. No commit credit necessary. This looks good.
Comment #16
webchickLooks 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. :)