Problem/Motivation

When content_translation is enabled, it uses the BaseFieldOverride entity in order to mark specific fields as translatable/untranslatable. When the moderation_state field has an override for anything, not just translation status, it stops working properly. Instead of an instance ModerationStateFieldItemList, we are given a FieldItemList for the moderation_state field.

Proposed resolution

Fix BaseFieldOverride so that it returns the correct list class.

Remaining tasks

Commit!

User interface changes

None.

API changes

Updated return value from BaseFieldOverride::getClass to match return value of the BaseFieldDefinition the BaseFieldOverride was created for.

Data model changes

None.

Steps to reproduce

Original issue manifested in conflict between content_moderation and content_translation:

  1. Install Drupal with the standard profile
  2. Enable Content Moderation and Content Translation modules
  3. Enable "Editorial workflow" for the Article content type.
  4. Add a new Article. Save it as draft, and then edit it again and save the changes as draft.
    There won't be any errors and the changes will be saved
  5. Add an additional language and enable translation for the Article node, and only for that type.
  6. Add an another test article again, repeat 4th step (above).
    Exception will be thrown
  7. Now repeat the 4th step with the Basic page content type as well
    Exception will be thrown
CommentFileSizeAuthor
#55 basefieldoverride-2848775-55.patch574 bytesSurfinSpirit
#45 2848775-45.patch4.02 KBSam152
#45 2848775-45_TESTS_ONLY.patch3.46 KBSam152
#45 interdiff.txt2.89 KBSam152
#39 2848775-39.patch2.07 KBSam152
#39 2848775-39_TEST_ONLY.patch1.51 KBSam152
#39 interdiff.txt522 bytesSam152
#35 VeryUGLY-JustToMakeItWorksQuickly-2848775-34-8.x.patch3.58 KBDuneBL
#35 content_moderation-EntityStorageException_when_re-saving_translatable_draft_as_draft-2848775-34-8.x.patch2.98 KBDuneBL
#33 2848775-33__TEST-ONLY.patch2.07 KBSam152
#30 2848775-30__TEST-ONLY.patch1.51 KBSam152
#29 Screen Shot 2017-08-11 at 3.14.10 PM.png27.11 KBSam152
#22 Issue-2848775-22-Fix-EntityStorageException_8.4.x.patch2.93 KBeyilmaz
#22 Issue-2848775-22-Fix-EntityStorageException_8.3.x.patch3 KBeyilmaz
#21 Issue-2848775-21-Fix-EntityStorageException_8.4.x.patch2.93 KBeyilmaz
#21 Issue-2848775-21-Fix-EntityStorageException_8.3.x.patch2.93 KBeyilmaz
#16 interdiff.txt1.03 KBeyilmaz
#16 interdiff-8.3.x.txt1.03 KBeyilmaz
#16 Issue-2848775-16-Fix-EntityStorageException_8.3.x.patch2.93 KBeyilmaz
#16 Issue-2848775-16-Fix-EntityStorageException_8.4.x.patch2.94 KBeyilmaz
#11 Issue-2848775-11-Fix-EntityStorageException.patch2.85 KBD34dMan
#10 Issue-2848775-10-Fix-EntityStorageException_8.3.2.patch2.84 KBD34dMan
#9 Issue-2848775-09-Fix-EntityStorageException_8.3.2.patch2.84 KBD34dMan
#8 Issue-2848775-08-Fix-EntityStorageException_8.3.2.patch2.16 KBD34dMan
#7 Issue-2848775-07-Fix-EntityStorageException.patch2.11 KBD34dMan
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

huzooka created an issue. See original summary.

huzooka’s picture

Title: EntityStorageException thrown when trying to re-save draft content as draft again » EntityStorageException thrown when trying to re-save translatable draft content as draft again
asghar’s picture

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

huzooka’s picture

Hi @asghar,

This issue is about Drupal 8.4.x and Drupal 8.3.x. The referred issue targets Drupal 8.2.x, but I'm almost sure that one won't be fixed.

Sam152’s picture

I could not reproduce this in 8.4.x HEAD.

stmh’s picture

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

D34dMan’s picture

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

D34dMan’s picture

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

D34dMan’s picture

This work includes translation into consideration

D34dMan’s picture

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

D34dMan’s picture

D34dMan’s picture

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.

stmh’s picture

Status: Active » Needs review

Patch works on my end

Status: Needs review » Needs work

The last submitted patch, 11: Issue-2848775-11-Fix-EntityStorageException.patch, failed testing. View results

eyilmaz’s picture

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’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
D34dMan’s picture

@eyilmaz that makes sense

eyilmaz’s picture

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

eyilmaz’s picture

Status: Needs work » Needs review

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

timmillwood’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Workflow Initiative

I still can't reproduce the issue.

catch’s picture

Issue tags: +WI critical
catch’s picture

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

eyilmaz’s picture

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.

Sam152’s picture

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
Sam152’s picture

Status: Needs work » Needs review
FileSize
1.51 KB

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.

Sam152’s picture

Status: Needs review » Needs work

The last submitted patch, 30: 2848775-30__TEST-ONLY.patch, failed testing. View results

Sam152’s picture

Status: Needs work » Needs review
FileSize
2.07 KB

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.

Sam152’s picture

Title: EntityStorageException thrown when trying to re-save translatable draft content as draft again » BaseFieldOverride entities created by content_translation break content_moderation because they don't support computed fields
Issue tags: -Needs tests

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.

DuneBL’s picture

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)

DuneBL’s picture

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

Sam152’s picture

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

DuneBL’s picture

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).
Sam152’s picture

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

The last submitted patch, 35: VeryUGLY-JustToMakeItWorksQuickly-2848775-34-8.x.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 39: 2848775-39_TEST_ONLY.patch, failed testing. View results

amateescu’s picture

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 :)

Sam152’s picture

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.

Sam152’s picture

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

The last submitted patch, 45: 2848775-45_TESTS_ONLY.patch, failed testing. View results

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs issue summary update

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.

catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Issue tags: +8.4.0 release notes

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.

plach’s picture

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.

Sam152’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

  • catch committed 82f1a1c on 8.5.x
    Issue #2848775 by Sam152, eyilmaz, D34dMan, DuneBL, huzooka:...

  • catch committed 7c87acf on 8.4.x
    Issue #2848775 by Sam152, eyilmaz, D34dMan, DuneBL, huzooka:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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!

Sam152’s picture

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

SurfinSpirit’s picture

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