Problem/Motivation
Ran into this in #2914839: The current moderation state in the "meta" region on content entity forms is coupled to the moderation_state field widget..
Inside prepareTranslation, the new translation is created like so:
public function prepareTranslation(ContentEntityInterface $entity, LanguageInterface $source, LanguageInterface $target) {
/* @var \Drupal\Core\Entity\ContentEntityInterface $source_translation */
$source_translation = $entity->getTranslation($source->getId());
$target_translation = $entity->addTranslation($target->getId(), $source_translation->toArray());
The key part is $source_translation->toArray()
in order to gather field data to initialise the new translation. This happens like so:
foreach ($this->getFields() as $name => $property) {
$values[$name] = $property->getValue();
}
Which translates roughly into the equivalent of:
$entity->get('moderation_state')->getValue();
This doesn't work for the moderation_state
field and thus a new translation will not be initialised with the moderation state from the previous translation.
Proposed resolution
Lets fix the computed field to handle all scenarios where the field item list can be accessed by making use of the work in #2392845: Add a trait to standardize handling of computed item lists.
Remaining tasks
Commit!
User interface changes
Fixes a user interface bug with entity translations not having the correct initial state.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#66 | 2915398-66.patch | 6.58 KB | Sam152 |
#66 | interdiff.txt | 2.77 KB | Sam152 |
#64 | 2915398-64_REMOVED_NULL_CHECK_IN_COMPUTE_VALUE.patch | 5.54 KB | Sam152 |
#64 | inter-interdiff.txt | 865 bytes | Sam152 |
#64 | 2915398-64.patch | 5.69 KB | Sam152 |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #3
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #4
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFixing method name.
Comment #6
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #7
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #8
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #9
timmillwoodLooks good to me, I tried to see if I could find a better solution, but not sure there is.
I guess as this is a bug it should also be backported to 8.4.x too?
Comment #10
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedYeah, I think a backport makes sense. I also dropped a note in #2392845: Add a trait to standardize handling of computed item lists that
getValue
is another method which should trigger a "calculate" on computed fields.Comment #11
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI'd like to increase the scope of this issue a bit and make
ModerationStateFieldItemList
use the trait added in #2392845: Add a trait to standardize handling of computed item lists so we don't start getting bug reports for each method that is not computing the value :)Comment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedMakes perfect sense, the
content_moderation
test suite might also turn up any issues with the implementation of the trait thus far.Comment #13
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThat's exactly what I'm hoping for :)
Comment #15
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedOne more combined patch now that the other issue is green.
Comment #16
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@Sam152, this should fix all the failures here, which makes me think that the default "caching" added by the trait is not really useful because most computed fields will probably have their own way of checking if the values were already computed or not.
I was working on this for a few hours, so the interdiff is to #11.
Comment #18
timmillwoodLooks pretty nice. I guess this is now postponed on #2392845: Add a trait to standardize handling of computed item lists, but leaving as "Needs review" for now, just going to prepend the title.
Comment #19
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis needs a small update for the latest patch in #2392845-133: Add a trait to standardize handling of computed item lists.
Comment #20
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThat issue circled back around and the trait is again at the typed data level, so re-uploading #16 just so it's the last good patch here.
Comment #21
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe parent issue is in so this is unblocked now.
Comment #23
timmillwoodUpdating
doComputeValue()
toensureComputedValue()
.Comment #24
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedGreat, now who wants to RTBC? :)
Comment #25
jibranCan we please add a comment here to explain why we ware overriding this function?
Comment #26
timmillwoodThis patch removes the need to override the
ensureComputedValue()
method.Calling
$entity->moderation_state->value = "draft"
uses\Drupal\content_moderation\Plugin\Field\ModerationStateFieldItemList::setValue()
to set the value in$this->list
but when fetching the value from$entity->moderation_state->value
this is ignored because$this->valueComputed
is FALSE, so it computes the value rather than using the set value.So in the patch, after calling
setValue()
the property$this->valueComputed
is set to TRUE to prevent computing the value and overwriting the set value.Comment #28
timmillwoodSeems like there are still occasions when
$this->list
is empty, and$this->valueComputed
is set to TRUE, so the values don't get computed.Comment #29
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI tried to debug this today but I didn't get anywhere.. I'll try again tomorrow.
Comment #30
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedEntityStateChangeValidationTest
can be fixed with the attached. The moderation_state field is set to be translateable, so I don't think it's meant to automatically be populated when adding a new translation? This is also how translations are created inContentTranslationController::prepareTranslation
.Still looking at the
ModerationStateNodeTest
fails.Comment #31
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThis is the source of the behavioural change. When a field is hidden, the widget still exists in the form array and the entity is built with
$entity->moderation_state = '';
. Previously if the list item was empty, we would always recompute. Now we don't do that. When '' is set, we consider that the value and we don't attempt to recompute values.I think we should not accept empty field items as
$valueComputed = TRUE
inComputedItemListTrait
.Comment #32
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedHere is the test fix from #30 as well a test to drill down on the exact behavioural change between the two patches. Doubting if this should actually go in
ComputedItemListTrait
, because it does seem like it should be possible to update a computed field to empty, just not a content moderation field it seems.Comment #33
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedSo based on the above analysis, I think overriding
ensureComputedValue
is the correct approach. Lets however use the same approach asComputedItemListTrait::ensureComputedValue
for caching, but just add our one specific$this->list[0]->isEmpty()
check. So we should now have:Comment #34
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedLast question, I have is should we be setting
$this->valueComputed = TRUE;
any time we callupdateModeratedEntity
?Comment #35
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks @Sam152, that really nice detective work :)
The fact that the moderation state field should never be empty is the key here. However, I think the question in #34 should be applied on a general scale: why don't we set the unpublished state value by default in
setValue()
when no other state is given by the form or an API call.This also means we should probably test what happens when we do
unset($node->moderation_state)
. That call should also result in setting the value to 'draft' IMO.Comment #36
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis is the part that maybe should be updated to *always* set a value. Which means that we should change
getModerationStateId()
so it can never return NULL, and return the default unpublished state by default.Comment #38
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks! :)
Doing it in
setValue
would also require the same check inonChange
, so we'd be back to trying to sleuth methods that impact$this->list
inModerationStateFieldItemList
which we're trying to avoid and haveComputedItemListTrait
do for us. But I also don't mind if you feel strongly about it.Nice extra test case + fix.
These parts changed (sorry missed the interdiff).
Comment #39
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented#38 was a cross post with #36, trying that approach now.
Comment #40
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented#36 didn't work (see attempt.patch, applies on top of #38) because it kicks in during the computing of the existing content state, not during the setting of a a new state. Thus I still prefer #38 so far.
Comment #42
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #44
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedReuploading #38 so it's obvious what needs review.
Comment #45
timmillwoodShouldn't this be
{@inheritdoc}
?I'd be tempted to do:
I'm not too fussy, but it would ensure that we use any future changes to the method in the trait, and future changes are tested with Content Moderation.
Comment #46
mstef CreditAttribution: mstef commentedTested out the patch from #44.
I may be misunderstanding the issue but it does not seem to work correctly.
1) Create a page node in English set to a workflow state of "Needs review" rather than "Draft".
2) Add a translation for French.
3) When adding the translation, it says the current state is "Needs review" whereas it should be "Draft". This happens with whatever state the English version is in.
If it were just the "Current state" label that was the issue, it would be okay, but the transitions that are available reflect that invalid state as well.
Before this patch, on 8.4.2, the label and available transitions are correct, but the validation is incorrect as that it assumes the new translation has the same state of the original copy. So it's basically the opposite.
EDIT: By "correct" I mean the new translation starts as a draft. I reread this issue and now I realize that point of this fix is to make the translation start at the state that the original copy is in. That does not make sense to me. If English is in a state such as "Needs review", creating a translation should not automatically mean that is ready for review, etc.
Comment #47
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks for the review.
#45.1: I'm not sure, can we inherit docs from a trait instead of a parent?
#45.2: Same here, tried this and parent::ensureComputedValue() doesn't exist because it comes in via the trait.
@mstef, this issue is mostly focused around making the computed field behave as closely to a normal field as possible. This is the test case that was failing, which just happened to expose itself through the creation of a new translation:
I only hit this from an API level, not via creating translations from the UI. When a translation is initialised, the values from the source translation are copied over, so I think given the old behaviour was only the result of a bug in FieldItemList. To make things even more confusing, the bug probably only exposed itself for states that weren't published/draft, since these become the default based on the publishing status. I haven't tested this from the UI, but a round-about way of testing what the patch is already testing would be:
This fails in HEAD, but passes with the patch applied. If I'm reading your analysis in #46 correctly, this inadvertently fixed a bug with the transition validation but introduced a behavioural change which you previously presumed to be intended?
Tricky stuff, thanks for testing this in any case.
Comment #48
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI reproduced #46 from the UI by:
1. Enabling CM and translation on "article".
2. Deleting the "Restore to Draft" transition.
3. Archiving an item of content.
4. Create a translation of that item of content.
5. Toggle patch, see different behaviors.
No patch
Patch
So from what I can tell, the behavior after the patch is applied is correct and new translations should not revert to draft when created from the UI, given it's how all other translatable fields work.
So the question from here is, are we happy with tests that fix the underlying API or do we want some explicit UI tests for the translation/transition validation stuff, which was fixed implicitly?
Comment #49
mstef CreditAttribution: mstef commentedOkay thanks for the explanation and the patch. I was confused by the issue.
I still personally think the functionality is not correct; but that's not for me to decide. I don't think that workflow states should be copied over like other fields because it's not like other fields. It's a status / not a value. Perhaps I can find a way to reset the workflow state when new translations are initialized.
Comment #50
timmillwoodI think @mstef is correct. When adding a translation we should not show anything in the meta sidebar, and not show a current state. The state for new translation should be assumed to be the initial state, however in ModerationStateFieldItemList
$entity->isNewTranslation()
returns FALSE. So I opened #2927455: When adding a new translation the entity should start from the draft state to fix that. This can be done as a follow up because the current functionality of this patch fixes an already big issue and holding that up would be a real shame.Here's a patch fixing the items in #45.
Think we're good for RTBC?
Comment #51
mstef CreditAttribution: mstef commentedThanks for opening that @timmillwood. I try to attempt a fix as soon as possible.
RTBC works for me.
Comment #52
timmillwoodIf RTBC works for @mstef, lets do it!
Comment #53
plachWhy is this filed against 8.4.x?
Comment #54
plach#49 and #50 make sense to me, but we need to update the IS to reflect the proposed solution.
Aside from that, I have a few nitpicks and questions:
Shouldn't we keep throwing this exception and invoke the parent (trait) method after this check is performed?
"Tests"
Also can we prefix the method name with
::
to improve readability?"Tests"
Comment #55
timmillwoodAdding
get()
back so we can have the exception, and adding a test for it.Comment #56
plachThanks! Can we please update also the IS?
Comment #57
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedUpdated IS.
Comment #58
plachThis will have to go in 8.5.x first.
Comment #59
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedGood point, forgot to update that. Queueing a test against 8.5.
Comment #60
timmillwoodIn the follow up to this (#2927455: When adding a new translation the entity should start from the draft state) I changed direction a little to force all new translations to start from the initial unpublished state (draft). Much like how we force new entities to start from the initial unpublished state. However this may see the return of the invalid transition issue shown in #48.
Comment #62
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFail is an issue with CI.
Comment #63
Wim Leers❤️
This means these two methods are collaborating in a subtle way.
It's only thanks to
computeValue()
conditionally setting$this->list[0]
thatensureComputedValue()
's comment makes sense:always recompute the state
.Also,
when none is is preset the default state is used
is not very clear: where is this default state then set? It's not happening here? (Should it bepresent
btw?)Finally, I wonder if it's be clearer to move this to after the
$this->traitEnsureComputedValue()
call? Because then it'd be clear we'd instantly "forgetting" what we just computed.@covers ::getValue
@covers ::get
@covers ::set
+@dataProvider
would be betterComment #64
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented2. I'm not sure these two methods are that related. Not sure if this explanation clears anything up.
ensureComputedValue
: This exists to preserve some existing idosyncratic behavior with how the computed worked before the trait was introduced, @see this described in #31. I think the key thing is the field item list can be set to an empty value completely outside of "computing" of the field (core does this in entity forms). Ie, I can go$entity->computed = '';
. Normally this would probably be valid, but in the case of CM, things don't place nice.computeValue
: The null check here is purely based around entities belonging to bundles that may not have moderation applied to them. It looks like it was introduced way back in #11 and simply moved from the original compute value method. Testing if we really need it in2915398-64_REMOVED_NULL_CHECK_IN_COMPUTE_VALUE.patch
(inter-interdiff.txt
). Hopefully this indicates why this exists, or gives us the nod to remove it.As for rearranging, the trait method reads from
$this->valueComputed
, so not sure that would work.3. Fixed!
4. Fixed!
5. What would go in the data provider? We're setting the value on two different objects and using an unset. Tried to update the comment to be a bit clearer.
Edit: Also right about preset/present. Will wait for some test runs before addressing that.
Comment #66
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedSo I think what #64 has exposed is:
For entities not under moderation,
$entity->moderation_state
can exist in a state of having zero list items. So it goes a little beyond what the comment indicates, it's not only about the static cache. With inter-interdiff applied, empty and null list items can be assigned during the normal lifecycle of a non-moderated entity and the field will essentially have a value for a non moderated entity.If that's the route everyone is happy to go down (which I think makes the most sense), I've added an explicit test for this, tried to clarify the comment on the method and also fixed preset/present.
I think this addresses everything raised so far.
Comment #67
timmillwood@Wim Leers - thanks for the review
@Sam152 - thanks for fixing these points.
Back to RTBC.
Comment #69
catchCommitted/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!
Comment #72
catchComment #73
penyaskitoSince this commit, adding a new translation via API forces that a new revision is added, instead of adding a new translation to the existing revision, which is making the Lingotek automated tests fail (we were actually ensuring the translation was done in the existing revision).
Is this on purpose?
Comment #74
plach@penyaskito:
We are talking about moderated entities, right? I don't think this was on purpose, but I thought any save of a moderated entity was already triggering the creation of a new revision.