Comments

tayzlor’s picture

Status: Active » Needs review
StatusFileSize
new1.25 KB
alexpott’s picture

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

tayzlor’s picture

StatusFileSize
new1.65 KB

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

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

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

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

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