Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tayzlor’s picture

Status: Active » Needs review
FileSize
1.25 KB
alexpott’s picture

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

tayzlor’s picture

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
FileSize
1.72 KB
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
FileSize
1.71 KB
1.73 KB

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

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
FileSize
726 bytes
1.67 KB

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

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
FileSize
3.02 KB
2.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.