Fatal error when viewing node with content moderation enabled if a module which implements hook_node_grants() is enabled

I have replicated this issue and can confirm that the #5 resolves the issue for me.

Nice bug finding and the fix looks correct. We need an automated test for this that pretty much does what the excellent steps to reproduce in the issue summary outlines.

Nits ahoy! Ye have been warned.

1. +++ b/core/modules/content_moderation/src/Tests/NodeAccessWithGrantTest.php
   @@ -0,0 +1,162 @@
   +class NodeAccessWithGrantTest extends ModerationStateTestBase {
   

   Can you upload a test-only patch to demo the fail?

2. +++ b/core/modules/content_moderation/src/Tests/NodeAccessWithGrantTest.php
   @@ -0,0 +1,162 @@
   +  /**
   +   * Editor user.
   +   *
   +   * @var \Drupal\Core\Session\AccountInterface
   +   */
   +  protected $editorUser;
   

   Style thing, but can't both account properties be next to each other?

3. +++ b/core/modules/content_moderation/src/Tests/NodeAccessWithGrantTest.php
   @@ -0,0 +1,162 @@
   +    'content_moderation_test_grants'
   

   nit, trailing comma

4. +++ b/core/modules/content_moderation/src/Tests/NodeAccessWithGrantTest.php
   @@ -0,0 +1,162 @@
   +    $nodes = \Drupal::entityTypeManager()
   +      ->getStorage('node')
   +      ->loadByProperties([
   +        'title' => 'moderated content',
   +      ]);
   

   You can use \Drupal\simpletest\NodeCreationTrait::getNodeByTitle

5. +++ b/core/modules/content_moderation/src/Tests/NodeAccessWithGrantTest.php
   @@ -0,0 +1,162 @@
   +    if (!$nodes) {
   +      $this->fail('Test node was not saved correctly.');
   +      return;
   +    }
   

   Can we just assert the success message you get from the UI when you create content?

6. +++ b/core/modules/content_moderation/src/Tests/NodeAccessWithGrantTest.php
   @@ -0,0 +1,162 @@
   +    $view_path = 'node/' . $node->id();
   +    $edit_path = 'node/' . $node->id() . '/edit';
   +    $latest_path = 'node/' . $node->id() . '/latest';
   

   A method would also mean we wouldn't need extra variables for the paths.

7. +++ b/core/modules/content_moderation/src/Tests/NodeAccessWithGrantTest.php
   @@ -0,0 +1,162 @@
   +
   +
   

   extra newlines

8. +++ b/core/modules/content_moderation/src/Tests/NodeAccessWithGrantTest.php
   @@ -0,0 +1,162 @@
   +    $this->drupalGet($edit_path);
   +    $this->assertResponse(200);
   +    $this->drupalGet($view_path);
   +    $this->assertResponse(200);
   +    // There is not yet latest revision.
   +    $this->drupalGet($latest_path);
   +    $this->assertResponse(403);
   

   This block is used 3 times, maybe we should create a method called assertPathResponses with three arguments, one for each status code?

9. +++ b/core/modules/content_moderation/src/Tests/NodeAccessWithGrantTest.php
   @@ -0,0 +1,162 @@
   +
   

   trailing newline

I didn't realise this test was actually pretty much what \Drupal\content_moderation\Tests\NodeAccessTest. Maybe some others can chime in here, but why not keep "content_moderation_test_grants" and just enable that during that test. Saves us from duplicating a fair bit test code.

Looks good to me.

I hate that we need to add new modules for tests like this, but I guess that's the price we pay for awesome test coverage.

Also going to add a test against 8.3.x, a lot has changed in Content Moderation, so need to make sure it works.

Looks like this needs re-rolling for 8.3.x

Rerolled to 8.3.x

Done for demonstration at Kerala Ruby Users Group meetup on patch rerolls.

Testing reroll.

Messed up previous reroll

It'd be nice to get an interdiff to see what's changed between the patches. I still don't think #38 will pass against 8.3.x because of the workflows module.

The fail looks genuine.

I hit this problem a lot while working on the tests for #2858431: Book storage and UI is not revision aware, changes to drafts leak into live site, so here's a cleaned up patch which should be good to go for 8.4.x.

+++ b/core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationStateConstraintValidator.php
@@ -117,7 +117,9 @@ public function validate($value, Constraint $constraint) {
-    $original_id = $original_entity->moderation_state;
+    if ($original_entity) {
+      $original_id = $original_entity->moderation_state->target_id;
+    }

This is the reason for the recent test fails. Accessing the field item list object is not the same thing as accessing one of its properties :)

Also, we don't need a new module that implements hook_node_grants(), we have node_access_test_empty.module for that.

#46 looks good.

Committed and pushed 2dad9ec to 8.4.x and 670c1a8 to 8.3.x. Thanks!

@timmillwood, @Sam152, @gabesullice - thanks for reviewing the patch. There were no comments that resulted in patch changes or provided test evidence that would result in commit credit. And of course thanks @flocondetoile, @anish.a, @Thew and @amateescu for working on the patch.

Seems like this issue also fixed #2838452: Permissions: View any unpublished content not working.

Shouldn't $original_entity->moderation_state be $original_entity->moderation_state->value here ??

@eyilmaz - this was committed nearly 2 months ago, if you are seeing issues, please open a new issue.

@eyilmaz, that does look a bit weird indeed. Do you mind opening a new issue to investigate the change you proposed and link it back here so everyone who worked on this can follow along?

Thanks!

Updated the patch here:
https://www.drupal.org/node/2848775
since it was more or less related.