Problem/Motivation

Splitting this into a two parter:

Setting up the state of the entity:

  1. The internal state of a content entity is required to have a moderation_state set in $values but with no field item list class loaded in $fields.
  2. The only way I can reliably setup this state is via a serialize/unserialize.
  3. Various things seem to depend on serializing entities (from states, persistent caches).

Triggering the error with an entity in this state:

  1. If you access a field on an entity, __get kicks in and proxies you through to the right FieldItemList class.
  2. In the case of ->moderation_state since our entity has $values as per the above, TypedDataManager::getPropertyInstance will set this value on our list class when creating it.
  3. Inside our list class when this value is being set, in updateModeratedEntity we do some calculations to see what kind of metadata we should set on the host entity (published status/defaultness).
  4. One of these checks is $content_moderation_info->isDefaultRevisionPublished.
  5. One of the things this method does is load the default revision of the entity and then access $default_revision->moderation_state->value
  6. The issue is, if we started this whole process using the default revision of the entity, the version that is loaded out of the static cache is the same entity from step 2!
  7. The result is Undefined property: Drupal\node\Entity\Node::$moderation_state because inside a __get we are trying to access the very same property from the very same object.

Here is a super stripped down version of what is going on which breaks with the very same error:

<?php
class Node {
    public function __get($prop) {
        $version_of_the_entity_from_the_static_cache = $this;
        return $version_of_the_entity_from_the_static_cache->{$prop};
    }
}
$foo = new Node();
print $foo->moderation_state;

The reason this is usually not a problem is:

  1. If the moderation_state doesn't exist in $values (which it doesn't by default because it's computed), when instantiating the FieldItemList class, when accessing moderation_state no value is set.
  2. When no value is set, updateModeratedEntity isn't called.
  3. Only getters on the list class, the moderation state is lazily loaded from there.

For discoverability:

If you have notices disabled, this manifests itself as:

InvalidArgumentException: The state '' does not exist in workflow.' in Drupal\workflows\Plugin\WorkflowTypeBase->getState() (line 154 of core/modules/workflows/src/Plugin/WorkflowTypeBase.php

If you have notices enabled, this manifests itself as:

Undefined property: Drupal\node\Entity\Node::$moderation_state.

Proposed resolution(s):

  1. Do what @validoll suggested in #2930139 and load a fresh copy of the entity, instead of one from the static cache, avoid the conflict above.
  2. Write an implementation of isDefaultRevisionPublished that doesn't require accessing $entity->moderation_state.
    1. Use EntityPublishedInterface::isPublished instead (doesn't support entities with custom moderation handlers that don't implement this interface).
    2. Load the state from ContentModerationState::loadFromModeratedEntity instead (probably doesn't work very well for newly enabled moderation on existing content, the default state wouldn't kick in).
  3. Write an implementation of updateModeratedEntity which doesn't need to call isDefaultRevisionPublished.
  4. Don't make any updates to the moderated entity if $notify is FALSE, thus avoiding a loop.

Remaining tasks

Commit the patch!

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Issue summary: View changes
Sam152’s picture

Status: Active » Needs review
FileSize
940 bytes

Test case to illustrate this.

Sam152’s picture

I still think the right fix should be to remove isDefaultRevisionPublished from ModerationStateFieldItemList somehow, but this does seem to fix the test case. Not exactly sure why right now.

jibran’s picture

I had the same issue with DER once #2407123: Can't access autocreated entity after serialization . Thank you so much for looking into it.

The last submitted patch, 3: 2940513-3.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 4: 2940513-4.patch, failed testing. View results

Sam152’s picture

timmillwood’s picture

It seems weird we have to reset the whole cache for a isDefaultRevisionPublished() check. I guess we should looking into your suggestion of not relying on ModerationInformationInterface::isDefaultRevisionPublished in ModerationStateFieldItemList::updateModeratedEntity.

Sam152’s picture

I can confirm the implementation in #2930139: Content moderation - error after trying to publish archived node also fixes this test case.

Sam152’s picture

Status: Needs work » Needs review
FileSize
883 bytes

I'd like to see what tests are currently covering the behavior of making revisions default if the current default is unpublished. It doesn't seem like ContentModerationStateTest covers this as I'd expected. I can think of a few alternative implementations, but I think some explicit test coverage would be best before rewriting it.

edit: the answer was: no tests! :)

Sam152’s picture

Title: ModerationStateFieldItemList fails after an entity has been serialized/unserialized. » [PP-1] ModerationStateFieldItemList fails after an entity has been serialized/unserialized.
Related issues: +#2940830: Provide explicit test coverage for isDefaultRevisionPublished and its usage within ModerationStateFieldItemList.
Sam152’s picture

Issue summary: View changes

Updating the issue summary with what (I hope) is a much more clear explanation of how this happens based on some more digging I did today. It should help inform the options we have for dealing with this problem.

Sam152’s picture

Issue summary: View changes
Sam152’s picture

Issue summary: View changes
Sam152’s picture

Issue summary: View changes
Sam152’s picture

Issue summary: View changes
Sam152’s picture

Issue summary: View changes

Adding some options to the proposed resolutions section.

Sam152’s picture

Issue summary: View changes
Sam152’s picture

Issue summary: View changes
amateescu’s picture

I agree with #4, something smells fishy in isDefaultRevisionPublished(). Commented in #2940830: Provide explicit test coverage for isDefaultRevisionPublished and its usage within ModerationStateFieldItemList. for now.

Berdir’s picture

I think there is another way to fix this.

The entity system has a way to notify fields/properties that they should *not* notifiy the parent when a value is set: $notify. onChange() respects that. I think this custom logic should too. If a field is initialized with the value set on the entity, then $notify is set to FALSE.

So all we need to do is respect that and the loop goes away.

Confirmed that this fixes the test that was uploaded here earlier.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

That makes perfect sense to me :)

amateescu’s picture

Title: [PP-1] ModerationStateFieldItemList fails after an entity has been serialized/unserialized. » ModerationStateFieldItemList fails after an entity has been serialized/unserialized.

Also, as discussed with @Berdir, this issue doesn't need to be postponed anymore with the new fix.

jibran’s picture

+++ b/core/modules/content_moderation/src/Plugin/Field/ModerationStateFieldItemList.php
@@ -141,7 +141,10 @@ public function onChange($delta) {
+    if (isset($this->list[0]) && $notify) {

Oh! The old notify nugget always catch us. Nice fix @Berdir. Let me test the patch locally as well.

jibran’s picture

Yup, this patch fixes my problem.

Sam152’s picture

e.g. this is not initialized with the current value

TIL what $notify is all about. Fix looks great, thanks @Berdir.

Sam152’s picture

Issue summary: View changes

Updating IS with the new approach.

alexpott’s picture

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

Committed 86746fc and pushed to 8.6.x. Thanks!

Setting back to 8.5.x for cherry-pick once 8.5.0 is out.

  • alexpott committed 86746fc on 8.6.x
    Issue #2940513 by Sam152, Berdir: ModerationStateFieldItemList fails...
alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Setting the issue to patch to be ported for cherry-pick once 8.5.0 is out. let's not clog the rtbc queue and ptbp makes them easy to find once 8.5.0 is out.

Sam152’s picture

Is there any criteria for evaluating bug fixes for 8.5.0? This has come up a few times in a few different queues and is quite ambiguous and tricky to track down. Given it's so simple, it would be great to include for the first stable release of content_moderation.

alexpott’s picture

Status: Patch (to be ported) » Fixed

@catch and @xjm discussed the RC policy for experimental modules recently as content moderation is still in beta (until 8.5.0 ships) this is eligible for backport. @catch +1'd this one so in it goes.

Committed 8eb7947 and pushed to 8.5.x. Thanks!

  • alexpott committed 8eb7947 on 8.5.x
    Issue #2940513 by Sam152, Berdir: ModerationStateFieldItemList fails...

Status: Fixed » Closed (fixed)

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

jhedstrom’s picture

Seeing very odd behavior if an entity is serialized and has a non-default moderation state. Opened #2971157: Moderation state set to default after entity is serialized.