Move node_access_needs_rebuild variable to the state system
Move variable_get/_set/_delete to state()->get/set/delete

Files: 
CommentFileSizeAuthor
#19 15-19-interdiff.txt2.91 KBalexpott
#19 node_access_needs_rebuild-1798804-19.patch3.02 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 46,365 pass(es).
[ View ]
#15 node_access_needs_rebuild-1798804-15.patch3.06 KBAlbert Volkman
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.install.
[ View ]
#15 interdiff.txt1.39 KBAlbert Volkman
#13 node_access_needs_rebuild-1798804-13.patch1.67 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 42,816 pass(es).
[ View ]
#13 interdiff.txt726 bytesAlbert Volkman
#11 node_access_needs_rebuild-1798804-11.patch1.76 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 42,559 pass(es).
[ View ]
#9 node_access_needs_rebuild-1798804-9.patch1.73 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 42,254 pass(es).
[ View ]
#9 interdiff.txt1.71 KBAlbert Volkman
#5 1798804-node-access-needs-rebuild.patch1.72 KBtayzlor
PASSED: [[SimpleTest]]: [MySQL] 41,902 pass(es).
[ View ]
#3 1798804-node-access-needs-rebuild.patch1.65 KBtayzlor
PASSED: [[SimpleTest]]: [MySQL] 41,902 pass(es).
[ View ]
#1 1798804-node-access-needs-rebuild.patch1.25 KBtayzlor
PASSED: [[SimpleTest]]: [MySQL] 41,896 pass(es).
[ View ]

Comments

tayzlor’s picture

Status:Active» Needs review
StatusFileSize
new1.25 KB
PASSED: [[SimpleTest]]: [MySQL] 41,896 pass(es).
[ View ]
alexpott’s picture

Does this need to be brought over from a d7 during upgrade?

tayzlor’s picture

StatusFileSize
new1.65 KB
PASSED: [[SimpleTest]]: [MySQL] 41,902 pass(es).
[ View ]

New patch with the hook_update_N() to shift the variable to state system.

alexpott’s picture

Status:Needs review» Needs work
+++ b/core/modules/node/node.installundefined
@@ -601,6 +601,15 @@ function node_update_8004() {
+function node_update_8005() {
+  state()->set('node_access_needs_rebuild', update_variable_get('node_access_needs_rebuild', FALSE));
+}

No need to set if node_access_needs_rebuild is false.

Need to also do an update_variable_del() too?

tayzlor’s picture

Status:Needs work» Needs review
StatusFileSize
new1.72 KB
PASSED: [[SimpleTest]]: [MySQL] 41,902 pass(es).
[ View ]
alexpott’s picture

Status:Needs review» Reviewed & tested by the community

Tested... 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.

alexpott’s picture

oh... and @tayzlor thanks for the work!

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

We've decided to namespace the key value names so we need to change the variable name from node_access_needs_rebuild to something else. Perhaps node.node_access_needs_rebuild

Albert Volkman’s picture

Status:Needs work» Needs review
StatusFileSize
new1.71 KB
new1.73 KB
PASSED: [[SimpleTest]]: [MySQL] 42,254 pass(es).
[ View ]

Namespaced. I chose to go with node.access_needs_rebuilt to remove the redundancy.

alexpott’s picture

Not 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? :)

Albert Volkman’s picture

StatusFileSize
new1.76 KB
PASSED: [[SimpleTest]]: [MySQL] 42,559 pass(es).
[ View ]

No, definitely no bikeshed. Just thought that was the way it was supposed to be. Here it is with your suggested name.

alexpott’s picture

Status:Needs review» Needs work

We 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...

+++ b/core/modules/node/node.installundefined
@@ -678,6 +678,18 @@ function node_update_8006(&$sandbox) {
+ * @ingroup state_upgrade

Change to @ingroup config_upgrade

Albert Volkman’s picture

Status:Needs work» Needs review
StatusFileSize
new726 bytes
new1.67 KB
PASSED: [[SimpleTest]]: [MySQL] 42,816 pass(es).
[ View ]

Ah, 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.

alexpott’s picture

Can 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

db_insert('variable')->fields(array(
  'name',
  'value',
))
->values(array(
  'name' => 'node_access_needs_rebuild',
  'value'=> serialize(TRUE),
));

And then add something like this to the test:

    $expected_state['node.node_access_needs_rebuild'] = array(
      'value' => TRUE,
      'variable_name' => 'node_access_needs_rebuild',
    );

Otherwise looks good!

Albert Volkman’s picture

StatusFileSize
new1.39 KB
new3.06 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.install.
[ View ]

Status:Needs review» Needs work
Issue tags:-Configuration system, -State system

The last submitted patch, node_access_needs_rebuild-1798804-15.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Configuration system, +State system

The last submitted patch, node_access_needs_rebuild-1798804-15.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new3.02 KB
PASSED: [[SimpleTest]]: [MySQL] 46,365 pass(es).
[ View ]
new2.91 KB

Bumped node_update_N so patch works and moved tests to the state test class so that they work too.

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

Tests passed, re-roll looks good to me.

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.