When there are existing nodes of a particular type, and then you enable moderation for that node type, you then get a fatal error upon saving a new draft of the existing content:

Fatal error: Call to a member function label() on a non-object in /var/www/moderation.local/web/modules/workbench_moderation/src/Plugin/Validation/Constraint/ModerationStateValidator.php on line 78

To duplicate:

* On a fresh install, create an article "Foo"
* Enable moderation for the article content type
* Edit the article "Foo"
* --> see the error

CommentFileSizeAuthor
#3 existing-content.patch4.74 KBCrell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

becw created an issue. See original summary.

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net for Acquia commented
Assigned: Unassigned » Crell

Have verified with a test, will post a fix shortly.

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net for Acquia commented
Status: Active » Needs review
FileSize
existing-content.patch4.74 KB

And here's the patch with test. Took a while to figure out the right kind of test, but this works. :-)

(Code is also in the existing-content branch.)

becw’s picture

becw CreditAttribution: becw at Palantir.net for Acquia commented 
  1. +++ b/src/Plugin/Validation/Constraint/ModerationStateValidator.php
@@ -66,18 +67,49 @@ class ModerationStateValidator extends ConstraintValidator implements ContainerI
+    if (!$this->validation->isTransitionAllowed($original_moderation_state_id, $next_moderation_state_id)) {

    So this validation doesn't check whether the given moderation state is available based on the bundle moderation config, does it? Just whether the transition exists.

    This isn't new though :) 

  2. +++ b/tests/src/Kernel/EntityStateChangeValidationTest.php
@@ -79,4 +79,32 @@ class EntityStateChangeValidationTest extends KernelTestBase {
+      'type' => 'example',

    Maybe use $this->randomMachineName() and $this->randomString().

This patch fixes the issue described in the ticket.

However, have you tried disabling workbench moderation on content with a forward revision? This leads to an unrelated (non-validator) issue, which I will hassle you about elsewhere.

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net for Acquia commented

Whether the transition is allowed or not is determined by StateTransitionValidation. Its logic doesn't change here. It looks like you're correct, it's not validating that the destination state is legal for the entity in question. I don't know if we should fix that here or elsewhere. I'm thinking elsewhere, as there's other cleanup that StateTransitionValidation desperately needs, unless the fix for it would be done in the method we're already modifying here... (The way StateTransitionValidation is wired to the container is so wrong it's painful.)

Is there an advantage to using random strings for the machine names instead of a known human-friendly string? I am always highly skeptical about the word "random" anywhere near a test class. (I also copied that code from elsewhere in the same class, so we're already using a static string.)

becw’s picture

becw CreditAttribution: becw at Palantir.net for Acquia commented
Status: Needs review » Reviewed & tested by the community

Yeah, using 'example' is... fine, it just increases the chance of collisions.

  • becw committed 7f60bdf on 8.x-1.x 
    Issue #2645608 by Crell: Can't edit existing content after enabling...
becw’s picture

becw CreditAttribution: becw at Palantir.net for Acquia commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.