Fatal error rebuilding node access permissions

I wanted to RTBC this but realised we don't have any tests for this...

Nice find, we also need a combined patch with the fix and the tests now.

Test looks correct, just a bunch of nitpicks...

1. +++ b/core/modules/node/src/Tests/NodeAccessRebuildNodeGrantsTest.php
   @@ -0,0 +1,39 @@
   + * Definition of Drupal\node\Tests\NodeAccessRebuildNodeGrantsTest.
   

   Should be contains, with a leading \: \Drupal\node\...

2. +++ b/core/modules/node/src/Tests/NodeAccessRebuildNodeGrantsTest.php
   @@ -0,0 +1,39 @@
   +
   +
   +namespace Drupal\node\Tests;
   

   Should only have one empty line.

3. +++ b/core/modules/node/src/Tests/NodeAccessRebuildNodeGrantsTest.php
   @@ -0,0 +1,39 @@
   +class NodeAccessRebuildNodeGrantsTest extends NodeTestBase{
   

   Missing space before {

4. +++ b/core/modules/node/src/Tests/NodeAccessRebuildNodeGrantsTest.php
   @@ -0,0 +1,39 @@
   +
   +  public static $modules = array('node', 'node_access_test');
   +
   +  protected function setUp() {
   +    parent::setUp();
   

   setUp() needs an @inheritdoc and the modules needs the usual Modules to enable @var array.

   Isn't node already enabled by default in NodeTestBase?

5. +++ b/core/modules/node/src/Tests/NodeAccessRebuildNodeGrantsTest.php
   @@ -0,0 +1,39 @@
   +    $web_user = $this->drupalCreateUser(array('administer site configuration', 'access administration pages', 'access site reports'));
   +    $this->drupalLogin($web_user);
   +    $this->web_user = $web_user;
   

   No need to assign the user, you don't need it anymore later.

Wow!

1. +++ b/core/modules/node/src/Tests/NodeAccessRebuildNodeGrantsTest.php
   @@ -0,0 +1,45 @@
   + * Definition of \Drupal\node\Tests\NodeAccessRebuildNodeGrantsTest.
   

   Certainly optional nitpick: You could use "Contains \Drupal\node ..."

2. +++ b/core/modules/node/src/Tests/NodeAccessRebuildNodeGrantsTest.php
   @@ -0,0 +1,45 @@
   +    $this->drupalGet('admin/reports/status');
   +    $this->clickLink(t('Rebuild permissions'));
   +    $this->drupalPostForm(NULL, array(), t('Rebuild permissions'));
   +    $this->assertText(t('The content access permissions have been rebuilt.'));
   

   What about adding a quick check that the rebuild was actually successfull? You could for example try the 'node.grant_storage' service ... to fetch entries.

.

New patch taking into account #8, added a few tests using the grant storage service

1) There are ~35 test cases that use "Definition of Drupal\node\.." ( without a leading \ ) in the node module.. what is the way to follow?

Contains is kind of the standard since 2 years, but we just had a hard time updating the old one :)

nice tests!

I found two small nitpicks, sorry :/

1. +++ b/core/modules/node/src/Tests/NodeAccessRebuildNodeGrantsTest.php
   @@ -0,0 +1,85 @@
   +  protected $web_user;
   

   Any reason to not use camel case here? :)

2. +++ b/core/modules/node/src/Tests/NodeAccessRebuildNodeGrantsTest.php
   @@ -0,0 +1,85 @@
   +  function testNodeAccessRebuildNodeGrants() {
   ...
   +  function testNodeAccessRebuildNoAccessModules() {
   

   These two need the member visibility (public).

For #14

Fixing the title to be a bit more specific.

This actually must be a critical because if this does not work then there are possible security implications.

Thanks for the review @amateescu, here's a new patch fixing #14

Feedback got addressed

Yup :)

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 33eb65a and pushed to 8.0.x. Thanks!