After enabling Workflows and Content Moderation there is a fatal error when trying to edit content translation

I think it might be better to work out why $entity->moderation_state->value is returning NULL, it should always return something.

Also, we'll need tests.

Hi, I did enable content moderation on a multilingual website and I got the same issue on pre-existing content.

If I create some content after enabling content moderation, I do not have any issue.

If I want to enable content moderation for multiple content type with existing content, I need to remove the node translation, and re-translate it to avoid having this issue on existing content.

Cheers.

Can confirm this when enabling content_moderation on an entity type with existing translations, you can edit and save one of those translations and then it fails with this exceptions when trying to edit another.

Should be possible to write a test for that, I'll see what I can do.

Let's add some tests. :)

+++ b/core/modules/content_moderation/tests/src/Functional/ModerationContentTranslationTest.php
@@ -0,0 +1,108 @@
+    // Enable moderation on Article node type.
+    $this->drupalCreateContentType(['type' => 'article', 'name' => 'Article'])->save();
+    $edit = [
+      'predefined_langcode' => 'fr',

left-over comment, we actually don't do that here in this test.

True, updating some other comment too. :)

+++ b/core/modules/content_moderation/src/EntityTypeInfo.php
@@ -347,7 +347,9 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
+          $workflow = $this->moderationInfo->getWorkflowForEntity($entity);
+          $default = $entity->moderation_state->value ? $workflow->getTypePlugin()->getState($entity->moderation_state->value) : $workflow->getTypePlugin()->getInitialState($entity);

I wonder if we have a bug in ModerationStateFieldItemList here, from memory the computed field should call out to getInitialState internally. It's probably worth stepping into that computed part just to validate there isn't something obvious that can be fixed at that level.

Definitely, we do need a better fix.

Started looking into it, in \Drupal\content_moderation\Plugin\Field\ModerationStateFieldItemList::getModerationStateId() , the call to loadContentModerationStateRevision() does return a $content_moderation_state but its state value is NULL.

The problem there is the call inside to addTranslation(), the existing values must be provided to that or it's initialized with empty fields. Like this:

$content_moderation_state->addTranslation($langcode, $content_moderation_state->toArray());

That should fix it.

Yep, Berdir your comment in #15 solves the issue for me. Converted it to a patch + interdiff attached,

It's the final line here in an existing test that is failing:

    // Move the French node back to draft.
    $french_node = $this->reloadEntity($english_node)->getTranslation('fr');
    $this->assertTrue($french_node->isPublished());
    $french_node->moderation_state->value = 'draft';
    // Revision 5 (en, fr).
    $french_node->save();
    $french_node = $this->reloadEntity($english_node, 5)->getTranslation('fr');
    $this->assertFalse($french_node->isPublished());

core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php

That test is a bit strange, the revision ids seem to be off by one. It says that the first french revision is *also* revision one, but I think by fixing this bug, we actually force a new revision already on that save (line 300) and that's correct because currently, content_moderation always forces a new revision. This happens in \Drupal\content_moderation\EntityOperations::entityPresave which checks for having a moderation state. that wasn't true by default but is now. I actually think that condition there was bogus and might have been hiding this bug, as pointed out in this issue, there should always be a value.

If you increase all the revision ids by one, the test should pass. Right now, it's still loading and comparing the previous revision, surprised that it tokes so many resaves to actually have a fail :)

I had the same kind of issue in #3006078: Content forms with translation throw error when content_moderation is added after the entity exists already even with a really similar patch. This one fixes the problem for me too.

Nice sleuthing, in addition to the fix it would be great to have some additional test cases in ModerationStateFieldItemListTest.

Right, having passing tests now.

What do we exactly want to test in ModerationStateFieldItemListTest?

Nice spot with the revision IDs in that existing test.

So we have the new browser test in this patch that makes sure we don't have the fatal error.

For the kernal test Sam152 mentioned, perhaps we can test that states exist for all translations when enabling content moderation and running getModerationState(). So before running that we can assert false that there is no state existing for the translation (in particular conditions) then run getModerationState() and assert true that a state now exists for the translation (ie, prove that addTranslation() side effect of the method occurs).

That sound about right?

Sounds pretty good to me, but we should just use the public API and not worry about getModerationState. Attached a kernel test which reproduces this.

In the functional test, this is the section of code which causes the fatal error:

        if (isset($form['meta']['published'])) {
          $form['meta']['published']['#markup'] = $this->moderationInfo->getWorkflowForEntity($entity)->getTypePlugin()->getState($entity->moderation_state->value)->label();
        }

So in order to test the buggy part of ModerationStateFieldItemList we have to go through a long series of slow UI steps, which only happens to trigger our problematic code path because we're attempting to show the label of the current state in the sidebar. Problems with this approach are:

• It's harder to maintain larger tests like this.
• It depends on implementation details (like our use of the field item list for display purposes in the sidebar) to test the code.
• It couples the coverage of our bug to an unrelated feature of the module.

The caveat is however that we won't have full end to end functional integration tests which cover this bug. In my opinion that's an acceptable tradeoff given the translation functionality in general does have this kind of coverage, so my preference would be to include only the kernel test with this patch.

I actually think that condition there was bogus and might have been hiding this bug, as pointed out in this issue, there should always be a value.

Should we add a follow up to evaluate this condition and remove it?

+++ b/core/modules/content_moderation/tests/src/Kernel/ModerationStateFieldItemListTest.php
@@ -314,4 +317,36 @@ public function testWorkflowCustomisedInitialState() {
+    ]);
+    $node->save();
+    $translation = $node->addTranslation('de');
+    $translation->title = 'Translated';
+    $translation->save();

this actually has the same "bug" now like the fix we're doing, should include $node->toArray() as second argument for consistency. Might not directly affect the test, but it's the correct thing to do.

Great to have the kernel test coverage but I think it would be useful to keep the browser test too. Even if we do change something in the future that would no longer trigger this specific problem, it makes sense to have end-to-end test coverage of it IMHO.

Good catch!

Happy to defer to your judgement on the need for an end to end test. Uploading everything combined so it's reviewable.

I also don't think it hurts to have the functional test coverage, showing the moderation state in Seven's sidebar is an important feature for site editors so it makes sense to have it working and tested in various scenarios :)

I only have a minor request for the patch itself:

+++ b/core/modules/content_moderation/tests/src/Functional/ModerationContentTranslationTest.php
@@ -0,0 +1,109 @@
+    $this->drupalPostForm('node/add/article', $edit, t('Save'));
+    $this->assertSession()->pageTextContains(t('Article Published English node has been created.'));
...
+    $this->clickLink(t('Add'));
...
+    $this->drupalPostForm(NULL, $edit, t('Save (this translation)'));
+    $this->assertSession()->pageTextContains(t('Article Published French node has been updated.'));
...
+    $this->drupalPostForm(NULL, $edit, t('Save'));
...
+    $this->assertSession()->pageTextContains(t('Article Published English new node has been updated.'));
...
+    $this->drupalPostForm(NULL, $edit, t('Save (this translation)'));
...
+    $this->assertSession()->pageTextContains(t('Article Published French new node has been updated.'));

We shouldn't use t() in tests unless we're actually testing the translation system :)

Updated the test without the use of t(). Re-ran the two test classes and they both correctly fail without core/modules/content_moderation/src/Plugin/Field/ModerationStateFieldItemList.php and both correctly pass with.

Nice, looks good to me.

LGTM as well :)

Patch on #32 tested on core 8.5.6, works well to resolve mentioned issue. I had the same issue after applying the workflow to article content. Thank You.

Adding issue credits

Committed 666388d and pushed to 8.7.x. Thanks!

Does not apply to 8.6.x

both modified: core/modules/content_moderation/tests/src/Kernel/ModerationStateFieldItemListTest.php

Hm, I think this broke HEAD. Seems to be failing for me.

Yep this broke HEAD

Testing Drupal\Tests\content_moderation\Kernel\ModerationStateFieldItemListTest
....................gF                                             21 / 21 (100%)

Time: 19.85 seconds, Memory: 4.00MB

There was 1 failure:

1) Drupal\Tests\content_moderation\Kernel\ModerationStateFieldItemListTest::testWithExistingUnmoderatedContent
Failed asserting that null matches expected 'published'.

And reverting fixes it.

Weird, requested a retest, This is the test we added here and it used to pass?

In #2943899: Moderation state field cannot be updated via REST, because special handling in ModerationStateFieldItemList we made "null" a valid value for the moderation_state field, so we broke our test case. Just need to reload the node after enabling moderation and it should be good again.

Attaching the interdiff, test only patch, the patch and the backport.

Fixing the backport and reuploading the 8.7 patch for clarity.

Okay, phew! Both branches are green.

Second time lucky.
Committed 925b4b5 and pushed to 8.7.x. Thanks!
Committed 46ad14aa7c and pushed to 8.6.x. Thanks!

Strangely, when I apply this patch to Drupal 8.6.3 using composer.json, I get a new directory named "b/core/..." and "core/core/...". Has anyone else seen a problem like that? I have other core patches which get applied correctly. Only this one creates these 2 weird directories.

I'm talking about this patch: https://www.drupal.org/files/issues/2018-11-12/3000573-8-6-51_0.patch

@jigarius OMG that has been happening to me with a few patches once in a while -- also...
...this one: #2856967-62: Allow admins to select a default entity moderation state
...and this one: #2797583-62: Dynamically provide action plugins for every moderation state change

>> So far, I've only experienced it with core patches -- but, it did *not* happen with this one: #2693675-22: fix system_get_info(), do not discard info about the current installation profile

It's so freaking weird!!!!!!! But I figure it's me somehow, not Drupal -- but also, I have no clue how to troubleshoot.

Others on the thread: I know it's out of scope, so I understand if you shoot me down, buuuuuttttt have you seen this before, and/or do you know why/when/how a patch will have this effect or not?

I am getting this error on a site migrated from D7. Site was having both Workbench Moderation and core Content Moderation modules installed. Uninstalling the Workbench Moderation module fixed the issue.

@junaidpv I'm running the same type of migration as you and your solution worked! Thank you so much for posting it!