Problem/Motivation
Splitting this into a two parter:
Setting up the state of the entity:
- 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
. - The only way I can reliably setup this state is via a serialize/unserialize.
- Various things seem to depend on serializing entities (from states, persistent caches).
Triggering the error with an entity in this state:
- If you access a field on an entity,
__get
kicks in and proxies you through to the right FieldItemList class. - 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. - 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). - One of these checks is
$content_moderation_info->isDefaultRevisionPublished
. - One of the things this method does is load the default revision of the entity and then access
$default_revision->moderation_state->value
- 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!
- 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:
- If the moderation_state doesn't exist in
$values
(which it doesn't by default because it's computed), when instantiating theFieldItemList
class, when accessingmoderation_state
no value is set. - When no value is set,
updateModeratedEntity
isn't called. - 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):
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.Write an implementation ofisDefaultRevisionPublished
that doesn't require accessing$entity->moderation_state
.UseEntityPublishedInterface::isPublished
instead (doesn't support entities with custom moderation handlers that don't implement this interface).Load the state fromContentModerationState::loadFromModeratedEntity
instead (probably doesn't work very well for newly enabled moderation on existing content, the default state wouldn't kick in).
Write an implementation ofupdateModeratedEntity
which doesn't need to callisDefaultRevisionPublished
.- Don't make any updates to the moderated entity if
$notify
isFALSE
, thus avoiding a loop.
Remaining tasks
Commit the patch!
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#22 | content-moderation-notify-default-revision-2940513-22.patch | 1.82 KB | Berdir |
#22 | content-moderation-notify-default-revision-2940513-22-test-only.patch | 940 bytes | Berdir |
#11 | test.patch | 883 bytes | Sam152 |
#4 | 2940513-4.patch | 1.9 KB | Sam152 |
#3 | 2940513-3.patch | 940 bytes | Sam152 |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #3
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedTest case to illustrate this.
Comment #4
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI still think the right fix should be to remove
isDefaultRevisionPublished
fromModerationStateFieldItemList
somehow, but this does seem to fix the test case. Not exactly sure why right now.Comment #5
jibranI had the same issue with DER once #2407123: Can't access autocreated entity after serialization . Thank you so much for looking into it.
Comment #8
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #9
timmillwoodIt seems weird we have to reset the whole cache for a
isDefaultRevisionPublished()
check. I guess we should looking into your suggestion of not relying onModerationInformationInterface::isDefaultRevisionPublished
inModerationStateFieldItemList::updateModeratedEntity
.Comment #10
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI can confirm the implementation in #2930139: Content moderation - error after trying to publish archived node also fixes this test case.
Comment #11
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI'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! :)
Comment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedUnless there are any objections, lets block behind some better coverage. #2940830: Provide explicit test coverage for isDefaultRevisionPublished and its usage within ModerationStateFieldItemList.
Comment #13
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedUpdating 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.
Comment #14
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #15
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #16
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #17
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #18
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAdding some options to the proposed resolutions section.
Comment #19
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #20
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #21
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI agree with #4, something smells fishy in
isDefaultRevisionPublished()
. Commented in #2940830: Provide explicit test coverage for isDefaultRevisionPublished and its usage within ModerationStateFieldItemList. for now.Comment #22
BerdirI 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.
Comment #23
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThat makes perfect sense to me :)
Comment #24
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAlso, as discussed with @Berdir, this issue doesn't need to be postponed anymore with the new fix.
Comment #25
jibranOh! The old notify nugget always catch us. Nice fix @Berdir. Let me test the patch locally as well.
Comment #26
jibranYup, this patch fixes my problem.
Comment #28
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedTIL what
$notify
is all about. Fix looks great, thanks @Berdir.Comment #29
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedUpdating IS with the new approach.
Comment #30
alexpottCommitted 86746fc and pushed to 8.6.x. Thanks!
Setting back to 8.5.x for cherry-pick once 8.5.0 is out.
Comment #32
alexpottSetting 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.
Comment #33
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedIs 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.
Comment #34
alexpott@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!
Comment #37
jhedstromSeeing 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.