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.
Move node_access_needs_rebuild variable to the state system
Move variable_get/_set/_delete to state()->get/set/delete
Comment | File | Size | Author |
---|---|---|---|
#19 | 15-19-interdiff.txt | 2.91 KB | alexpott |
#19 | node_access_needs_rebuild-1798804-19.patch | 3.02 KB | alexpott |
#15 | node_access_needs_rebuild-1798804-15.patch | 3.06 KB | Albert Volkman |
#15 | interdiff.txt | 1.39 KB | Albert Volkman |
#13 | node_access_needs_rebuild-1798804-13.patch | 1.67 KB | Albert Volkman |
Comments
Comment #1
tayzlor CreditAttribution: tayzlor commentedComment #2
alexpottDoes this need to be brought over from a d7 during upgrade?
Comment #3
tayzlor CreditAttribution: tayzlor commentedNew patch with the hook_update_N() to shift the variable to state system.
Comment #4
alexpottNo need to set if node_access_needs_rebuild is false.
Need to also do an update_variable_del() too?
Comment #5
tayzlor CreditAttribution: tayzlor commentedComment #6
alexpottTested... works as expected.
Reviewers: note that node_access_rebuild is only ever set to TRUE - otherwise it is deleted. That's why in the update_N function the variable is only deleted if it is set to TRUE.
Comment #7
alexpottoh... and @tayzlor thanks for the work!
Comment #8
alexpottWe've decided to namespace the key value names so we need to change the variable name from
node_access_needs_rebuild
to something else. Perhapsnode.node_access_needs_rebuild
Comment #9
Albert Volkman CreditAttribution: Albert Volkman commentedNamespaced. I chose to go with node.access_needs_rebuilt to remove the redundancy.
Comment #10
alexpottNot sure about node.access_needs_rebuild as node_access is the table that is being rebuilt!... that's why I plumped for node.node_access_needs_rebuild...
Bikeshed anyone? :)
Comment #11
Albert Volkman CreditAttribution: Albert Volkman commentedNo, definitely no bikeshed. Just thought that was the way it was supposed to be. Here it is with your suggested name.
Comment #12
alexpottWe have a new function for updating variables to state...
update_variables_to_state()
Have a look at
system_update_8026()
for how to use this.Also @sun and I have agreed that all state upgrades should just be documented with the same group as config. So...
Change to @ingroup config_upgrade
Comment #13
Albert Volkman CreditAttribution: Albert Volkman commentedAh, I had been watching #1798732: Convert install_task, install_time and install_current_batch to use the state system but didn't realize that it had landed. Been traveling in Ireland (on a train now from Cork to Dublin, actually). Patch updated.
Comment #14
alexpottCan you add to the test in Drupal\system\Tests\Upgrade\StateSystemUpgradePathTest?
You will need to set the value to TRUE using drupal-7.state.system.database.php
And then add something like this to the test:
Otherwise looks good!
Comment #15
Albert Volkman CreditAttribution: Albert Volkman commentedComment #17
Berdir#15: node_access_needs_rebuild-1798804-15.patch queued for re-testing.
Comment #19
alexpottBumped node_update_N so patch works and moved tests to the state test class so that they work too.
Comment #20
BerdirTests passed, re-roll looks good to me.
Comment #21
webchickCommitted and pushed to 8.x. Thanks!