BaseFieldOverride entities created by content_translation break content_moderation because they don't support computed fields

Hi @huzooka

I have repeated the same steps for Drupal version 2.5 and 8.2.6 on local and simplytest.me respectively but could not produce the error. In your 3rd step you mentioned *Editorial workflow* it meant *Manage moderation* Dropdown right?. Could you produce the same error on simplytest.me ?. Thanks

I could not reproduce this in 8.4.x HEAD.

I can reproduce this issue with 8.3.2, enabling content_translation and content_moderation. Saving a second, unpublished draft will throw this error.

The patch attempts to check if the moderation state value exists before trying to use them.

Just in case anybody wants to try the patch with 8.3.2, attaching for the sake of convenience.

This work includes translation into consideration

Sorry, about the previous patch, fixed the error in above patch int his one.

Attaching fix for 8.4.x-dev

In my local install based on 8.3.2, there were lots of revisions for a particular node and its translation, before the content moderation and workflow as enabled. Also, none of the nodes were in published state either. After enabling content moderation and workflow, I assigned the editorial workflow to these contents. After this, attempting to save the nodes started giving Exceptions. Applying the patch solves the issue for me.

Patch works on my end

The getTranslation() method throws an exception when there is no translation.
Check for hasTranslation before executing getTranslation().

Patches for 8.3.x & 8.4.x attached.

@eyilmaz that makes sense

Updating patch again.
The last return in isDefaultRevisionPublished is checking for the wrong revision.

Fixing another issue where original_id was always set.

I still can't reproduce the issue.

I think this might be a duplicate of an older issue that was since fixed, but can't find that issue at the moment.

Root cause for this may be that there is a fieldoverride configuration for the moderation_state field. Then ModerationStateFieldItemList is not used when trying to get the value for moderation_state (since type is string). This is for 8.3. did not checked for 8.4.

I tried to reproduce this, but couldn't trigger the exact exception. I did run into some weird scenarios which I think could be the same issue reported here, but manifesting in different ways.

I was able to reproduce something similar with:

1. drush si standard -y && drush en content_moderation -y && drush en content_translation -y
2. Add a second language. /admin/config/regional/language
3. Add content translation for "article". /admin/config/regional/content-language
4. Create a basic page /node/add/page. The widget isApplicable method returns FALSE and a textfield widget is shown:
   
5. From there, enabling moderation works on article, but never on page again. The textfield sticks around and can be used to enter invalid states which trigger a very similar error message in the IS. The state 'foo' does not exist in workflow

I spent some more time debugging this. I think #28 is correct and any field config override breaks moderation in the exact way the issue summary describes.

Content translation simple exposes this in content_translation_form_language_content_settings_submit:

          $translatable = based on UI checkboxes.
          if ($field_config->isTranslatable() != $translatable) {
            $field_config
              ->setTranslatable($translatable)
              ->save();
          }

The callstack I get with this test-only patch is:

 Caused by
 InvalidArgumentException: The state '' does not exist in workflow.'
 
/core/modules/workflows/src/Plugin/WorkflowTypeBase.php:167
/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php:110
/core/modules/content_moderation/src/ModerationInformation.php:180
/core/modules/content_moderation/src/EntityOperations.php:113
/core/modules/content_moderation/content_moderation.module:73
/core/lib/Drupal/Core/Extension/ModuleHandler.php:402
/core/lib/Drupal/Core/Entity/EntityStorageBase.php:169
/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:441
/core/lib/Drupal/Core/Entity/EntityStorageBase.php:435
/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:298
/core/lib/Drupal/Core/Entity/EntityStorageBase.php:389
/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:796
/core/lib/Drupal/Core/Entity/Entity.php:377
/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php:427

Note: I don't think the translation status of the field matters in any of these test cases, it's irrelevant given it's a computed field with an always-translatable entity backing it.

#29 is a seperate issue which I'll open shortly, with the same cause, a field config override.

I've logged #2901478: Test ModerationStateWidget::isApplicable with a BaseFieldOverride for #29. Looking at a fix for ##30.

Edit: This patch name is incorrect, this is the test and fix ^

Tracked the instantiation of the FieldListItem class through TypedDataManager to arrive at BaseFieldOverride returning the wrong class for ::getClass().

I think this possibly points to a broader problem, the BaseFieldOverride entity does not work properly with computed fields with overriden list classes. The parent implementation of ::getClass this was falling back to looks like:

  // \Drupal\Core\Field\FieldConfigBase
  /**
   * {@inheritdoc}
   */
  public function getClass() {
    // Derive list class from the field type.
    $type_definition = \Drupal::service('plugin.manager.field.field_type')
      ->getDefinition($this->getType());
    return $type_definition['list_class'];
  }

Which would make for configuration based fields, you can't change the list class, it's always based on the field type. But in the override use case, calling the storage definitions implementation of getClass is more comprehensive because it checks the definition for a class before calling back off to plugin.manager.field.field_type:

  // \Drupal\Core\TypedData\ListDataDefinition
  public function getClass() {
    $class = isset($this->definition['class']) ? $this->definition['class'] : NULL;
    if (!empty($class)) {
      return $class;
    }
    else {
      // If a list definition is used but no class has been specified, derive
      // the default list class from the item type.
      $item_type_definition = \Drupal::typedDataManager()
        ->getDefinition($this->getItemDefinition()->getDataType());
      if (!$item_type_definition) {
        throw new \LogicException("An invalid data type '{$this->getItemDefinition()->getDataType()}' has been specified for list items");
      }
      return $item_type_definition['list_class'];
    }
  }

The fix in the patch attached is in-line with other methods which presumably are designed to solve similar problems:

  /**
   * {@inheritdoc}
   */
  public function isDisplayConfigurable($context) {
    return $this->getBaseFieldDefinition()->isDisplayConfigurable($context);
  }

  /**
   * {@inheritdoc}
   */
  public function getDisplayOptions($display_context) {
    return $this->getBaseFieldDefinition()->getDisplayOptions($display_context);
  }

  /**
   * {@inheritdoc}
   */
  public function isReadOnly() {
    return $this->getBaseFieldDefinition()->isReadOnly();
  }

  /**
   * {@inheritdoc}
   */
  public function isComputed() {
    return $this->getBaseFieldDefinition()->isComputed();
  }

  /**
   * {@inheritdoc}
   */
  public function getClass() {
    return $this->getFieldStorageDefinition()->getClass();
  }

Lets see what the testbot thinks.

I hope it's not too presumptuous of me to update the IS and title based on what I've found. With the patch in #33 applied, I can no longer reproduce the error which I was seeing consistently following the steps in the issue summary.

I have rerolled #22 for 8.5
But I discover that, in 8.5, workflow::getState() doesn't exists anymore!!!
It has been moved into a WorkflowTypeBase.

This is why I have created a very ugly patch which will work only if the user use the default state of the module.
This is a patch that can be used only to make your site working quickly (and a proof of concept that #22 is doing the job)

@Sam152: sorry I have posted #35 without testing/reading #30->#35

No problem! If you are encountering the problem, some testing of #33 would be greatly appreciated.

I confirm that #33 is working nicely!!!
Many thanks
Just a small offset in 8.5

patching file core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php
patching file core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
Hunk #1 succeeded at 330 (offset -69 lines).

Minor change to bring the method in-line with the others.

The analysis in #33 and the fix makes sense indeed, but I'm concerned that I couldn't find any kernel or unit test for the BaseFieldOverride entity type. Wouldn't it be better to add a generic test for that instead of the Content Moderation one?

But if we decide to keep this test as well, I just one remark for it:

+++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
@@ -399,6 +399,36 @@ public function testMultilingualModeration() {
+      'moderation_state' => 'published',
+    ]);
+    $node->save();
+    $this->assertTrue($node->isPublished());
+
+    $node->moderation_state = 'draft';
+    $node->save();
+    $this->assertFalse($node->isPublished());

How about testing the reverse scenario: don't provide any value for the moderation_state field, check the 'draft' default value has been applied, and then change it to published? I think that's how we usually test our computed moderation_state field :)

I agree, we should have a dedicated test for this. The issue where BaseFieldOverride was introduced included a bunch of unit tests for this stuff, but I can't find where they are in HEAD.

I couldn't find an existing test class where this made sense, I've created a new one specifically for testing the BaseFieldOverride entity.

Can't see any reason to not RTBC.

Yet again, nice investigation from @Sam152!

I guess we should update the issue summary based on #33?
Also, does this need a change record, not that it's a change, but it's a pretty deep and weird bug fix.

I don't think this needs a change record since no-one should have to do anything differently, but definitely the issue summary update, tagging for a release notes mention.

Patch looks very straightforward but didn't do an in-depth review of the tests or anything yet.

RTBC +1

Minor stuff that can be fixed on commit:

1. +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
   @@ -399,6 +399,37 @@ public function testMultilingualModeration() {
   +   * Test moderation when the moderation_state field has a config override.
   

   Tests

2. +++ b/core/tests/Drupal/KernelTests/Core/Field/Entity/BaseFieldOverrideTest.php
   @@ -0,0 +1,65 @@
   +class BaseFieldOverrideTest extends KernelTestBase {
   

   @#43:

   A standalone test works for me, however the original kernel test is in EntityFieldTest.

Given the tests are installing different modules/schema I think the standalone test class is OK.

Fixed the comment issue on commit.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Woohoo! Any WI critical squashed! Thanks for the attention you are giving these issues @catch.

Kernel tests are not applying on 8.3.4 so attaching a patch without tests.