The moderation_state field is not computed during the creation of a new entity translation.

Looks 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?

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

... the content_moderation test suite might also turn up any issues with the implementation of the trait thus far.

That's exactly what I'm hoping for :)

@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.

Looks 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.

This needs a small update for the latest patch in #2392845-133: Add a trait to standardize handling of computed item lists.

That 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.

The parent issue is in so this is unblocked now.

Updating doComputeValue() to ensureComputedValue().

Great, now who wants to RTBC? :)

+++ b/core/modules/content_moderation/src/Plugin/Field/ModerationStateFieldItemList.php
@@ -14,6 +15,29 @@
+    if (!isset($this->list[0]) || $this->list[0]->isEmpty()) {

Can we please add a comment here to explain why we ware overriding this function?

This 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.

Seems like there are still occasions when $this->list is empty, and $this->valueComputed is set to TRUE, so the values don't get computed.

I tried to debug this today but I didn't get anywhere.. I'll try again tomorrow.

Thanks @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.

+++ b/core/modules/content_moderation/src/Plugin/Field/ModerationStateFieldItemList.php
@@ -14,6 +15,33 @@
+    $moderation_state = $this->getModerationStateId();
+    // Do not store NULL values in the static cache.
+    if ($moderation_state) {
+      // An entity can only have a single moderation state.
+      $this->list[0] = $this->createItem(0, $moderation_state);
+    }

This 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.

1. +++ b/core/modules/content_moderation/src/Plugin/Field/ModerationStateFieldItemList.php
   @@ -14,6 +15,33 @@
   +  /**
   +   * Ensures that values are only computed once.
   +   */
   +  protected function ensureComputedValue() {
   

   Shouldn't this be {@inheritdoc}?

2. +++ b/core/modules/content_moderation/src/Plugin/Field/ModerationStateFieldItemList.php
   @@ -14,6 +15,33 @@
   +    if ($this->valueComputed === FALSE || !isset($this->list[0]) || $this->list[0]->isEmpty()) {
   +      $this->computeValue();
   +      $this->valueComputed = TRUE;
   

   I'd be tempted to do:

   if (!isset($this->list[0]) || $this->list[0]->isEmpty()) {
     $this->valueComputed = FALSE;
   }
   parent::ensureComputedValue();
   

   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.

Tested 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.

Okay 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.

I 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?

Thanks for opening that @timmillwood. I try to attempt a fix as soon as possible.

RTBC works for me.

If RTBC works for @mstef, lets do it!

Why is this filed against 8.4.x?

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

1. +++ b/core/modules/content_moderation/src/Plugin/Field/ModerationStateFieldItemList.php
   @@ -83,41 +113,6 @@ protected function loadContentModerationStateRevision(ContentEntityInterface $en
   -  public function get($index) {
   -    if ($index !== 0) {
   -      throw new \InvalidArgumentException('An entity can not have multiple moderation states at the same time.');
   -    }
   

   Shouldn't we keep throwing this exception and invoke the parent (trait) method after this check is performed?

2. +++ b/core/modules/content_moderation/tests/src/Kernel/ModerationStateFieldItemListTest.php
   @@ -79,6 +79,27 @@ public function testArrayIteration() {
   +   * Test getting the moderation_state with getValue.
   

   "Tests"

   Also can we prefix the method name with :: to improve readability?

3. +++ b/core/modules/content_moderation/tests/src/Kernel/ModerationStateFieldItemListTest.php
   @@ -79,6 +79,27 @@ public function testArrayIteration() {
   +   * Test setting an empty value on the moderation_state field.
   

   "Tests"

Adding get() back so we can have the exception, and adding a test for it.

Thanks! Can we please update also the IS?

This will have to go in 8.5.x first.

In 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.

1. +++ b/core/modules/content_moderation/src/Plugin/Field/ModerationStateFieldItemList.php
   @@ -14,6 +15,36 @@
   +    get as traitGet;
   
   @@ -90,32 +121,7 @@ public function get($index) {
        if ($index !== 0) {
          throw new \InvalidArgumentException('An entity can not have multiple moderation states at the same time.');
        }
   ...
   +    return $this->traitGet($index);
   

   ❤️

2. +++ b/core/modules/content_moderation/src/Plugin/Field/ModerationStateFieldItemList.php
   @@ -14,6 +15,36 @@
   +    // Do not store NULL values in the static cache.
   +    if ($moderation_state) {
   ...
   +    // If the moderation state field is set to an empty value, always recompute
   +    // the state. Empty is not a valid moderation state value, when none is
   +    // preset the default state is used.
   +    if (!isset($this->list[0]) || $this->list[0]->isEmpty()) {
   

   This means these two methods are collaborating in a subtle way.

   It's only thanks to computeValue() conditionally setting $this->list[0] that ensureComputedValue()'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 be present 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.

3. +++ b/core/modules/content_moderation/tests/src/Kernel/ModerationStateFieldItemListTest.php
   @@ -79,6 +79,36 @@ public function testArrayIteration() {
   +   * Tests getting the moderation_state with getValue.
   

   @covers ::getValue

4. +++ b/core/modules/content_moderation/tests/src/Kernel/ModerationStateFieldItemListTest.php
   @@ -79,6 +79,36 @@ public function testArrayIteration() {
   +   * Tests getting the moderation_state with get.
   

   @covers ::get

5. +++ b/core/modules/content_moderation/tests/src/Kernel/ModerationStateFieldItemListTest.php
   @@ -79,6 +79,36 @@ public function testArrayIteration() {
   +   * Tests setting an empty value on the moderation_state field.
   

   @covers ::set + @dataProvider would be better

@Wim Leers - thanks for the review
@Sam152 - thanks for fixing these points.

Back to RTBC.

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

Since 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?

@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.