Moderation state field cannot be updated via REST, because special handling in ModerationStateFieldItemList

If ModerationStateFieldItemList does special stuff on removing a item maybe it should also override appendItem() so that things continues work as expected?

Per the discussion, it looks like this is more of a content_moderation problem than a rest problem. Or, to be more specific: the problem is with how it uses the Entity/Field API in a special way.

So moving there for now. I'm betting @amateescu will have thoughts on this :)

We explicitly have test cases which assert we can't empty out the moderation_state field, ModerationStateFieldItemListTest::testSetEmptyState. This was a behavior of ModerationStateFieldItemList which predated ComputedItemListTrait, so I don't believe this is necessarily a problem with the trait. We had to override parts of it to keep that working.

For context, I believe there were some nasty implications down the call-stack if the moderation_state field was ever completely emptied (EntityFormDisplay attempts this when the moderation widget is hidden I think), hence the nuanced and unusual behavior.

I do believe there would be some implementation of FieldNormalizer::denormalize which doesn't rely on an empty FieldItemList which would satisfy regular FieldItemLists and the content_moderation one, but I don't know if we'd want to make such a concession for CM in an "upstream" subsystem. The alternative would be a custom normalizer. Testing this with the attached patch.

Whatever the implementation a full integration test using moderation_state with rest would probably be valuable. Rest has been a selling-point/consideration of a few implementation decisions along the way for CM, so not supporting it over something silly would be a shame. EntityResourceTestBase looks like possibly a good starting point?

This should fix some test cases. This was failing because of deltas not being sequential in this test case:

    // Multi-value field: remove item 0. Then item 1 becomes item 0.
    $normalization_multi_value_tests = $this->getNormalizedPatchEntity();
    $normalization_multi_value_tests['field_rest_test_multivalue'] = $this->entity->get('field_rest_test_multivalue')->getValue();
    $normalization_remove_item = $normalization_multi_value_tests;
    unset($normalization_remove_item['field_rest_test_multivalue'][0]);

Re-keying the field values when using set instead of appendItem should fix a few things.

Stepping back from a solution, here is an approach to testing this. This adds the same suite of REST tests each entity type has for a moderated node. It currently fails with the same error reported in the issue summary:

{"message":"Unprocessable Entity: validation failed.\nmoderation_state: Moderation state: this field cannot hold more than 1 values.\n"}

I suppose one thing ModeratedNodeResourceTestBase could become in the future is a place to add more coverage around REST requests that interact with a moderated entity. The other aspect is: we'd essentially lose coverage for the Node entity under conditions where content_moderation is disabled. How important that is, I'm not sure.

I'm still not 100% on this approach, but I'll try fix the remaining tests so it can be properly evaluated. One strange semantic is: FieldItem classes will notify the parent FieldItemList of a change, even if that FieldItem is "detached" and not a member of the FieldItemList yet.

Another approach to fixing this might be to override ItemList::appendItem in ModerationStateFieldItemList to simply return the computed 0 delta, that feels a bit awkward too though.

@Sam152, actually, that might be the correct thing to do since ModerationStateFieldItemList goes out of its way to always have a value at delta 0 :)

Realising that was mentioned all the way back in #2 :)

On the other hand: could removing appendItem in rest provide better support for fields which generally aren't emptyable, especially if the fix isn't really that ugly (this might be a uniquely CM thing)? Overriding a method in an already tricky/complex class to behave in opposition to its name and purpose seems like a bit of a "gotcha" that could bite us in the future.

Overriding a method in an already tricky/complex class to behave in opposition to its name and purpose seems like a bit of a "gotcha" that could bite us in the future.

That's a very good point. Do you know if it's documented anywhere why exactly are we trying to always keep the moderation_state field not empty? Maybe this is just for historical reasons or because of missing core functionalities when the original code of CM was written, and then we could try to make it behave like every other field.

The unemptiableness of the field was discussed here in the context of preserving the behavior when migrating to ComputedItemListTrait. Here are the test cases that failed when ComputedItemListTrait implicitly removed that behaviour. We added an explicit test case for it in that issue:

  /**
   * Tests the computed field when it is unset or set to an empty value.
   */
  public function testSetEmptyState() {
    $this->testNode->moderation_state->value = '';
    $this->assertEquals('draft', $this->testNode->moderation_state->value);

    $this->testNode->moderation_state = '';
    $this->assertEquals('draft', $this->testNode->moderation_state->value);

    unset($this->testNode->moderation_state);
    $this->assertEquals('draft', $this->testNode->moderation_state->value);
  }

I'm not totally sure where else this has come up, I feel like it has popped up a few other times but I couldn't say where.

Yep, I remember the recent discussion from the switch to ComputedItemListTrait, but I was wondering about the original/historical reason for that behavior.

The reason for questioning it is that it seems to cause a lot of problems so maybe we should address the root cause instead :)

I suppose the failing test cases can be a reference for what parts broke with an emptiable field. You're right, it might be worth evaluating the effort in addressing those issues instead.

So if we go back to your comment from #2915398-38: The moderation_state field is not computed during the creation of a new entity translation. (first paragraph), we already have special handling in both setValue() and onChange() so we know that whatever change needs to happen to get rid of the "never empty value" behavior will require both of those methods to stay in sync.

I'm sorry that I didn't follow up on that path at the time, but I feel we should restart the investigation based on your "attempt" patch form #40 in that issue..

I stumbled another issue impacted by assigning empty values to moderation_state (#2915384: Alternative moderation_state field widgets do not receive the same access/visibility treatment as the default ModerationStateWidget.), in this case I think the issue would also be helped if rolled back the decision to avoid assigning ''. Given that issue is around the behavior of the field on non moderated entities, I also think #2915383: The moderation_state base field is added to all revisionable entity types even if they do not have moderation enabled is kind of tangentially related too.

Trying to approach suggested by @amateescu to make the moderation_state field emptiable. Lets see what the testbot says. This passes the new rest tests as expected.

The fail is what I mentioned in #5:

For context, I believe there were some nasty implications down the call-stack if the moderation_state field was ever completely emptied (EntityFormDisplay attempts this when the moderation widget is hidden I think), hence the nuanced and unusual behavior.

When the widget is hidden an empty string is assigned to moderation_state. Having a look at this now.

Lets try this: avoid ever making the ModerationStateWidget assign '' as a state, even if it's hidden by access control. Instead make the default the value already provided/computed on $items so WidgetBase::extractFormValues simply reassigns that same value.

@Sam152

Patch #31 works but it's missing permissions while making rest call. That means draft creator can also change moderation state to published even though he do not have publish transition permission.

Patch #31 works!

But problem is user can update moderation state though REST even he/she do not have that transition permission. For eg: User with only create draft permission can update node via REST PATCH to published state event if do not have permission to publish content from draft state.

Reuploading for another test run. Would be good to get this issue back on track.

The patch looks great to me :) I think the only question I have is what happens when we try to save a node with an empty value for the moderation state field. I would expect that the new revision would keep the previous moderation state.

I see we added some assertions to check for assigning empty values, can we expand them a bit to also save the node and check that the moderation state didn't change?

Good point. I think the validation of the entity might fail under those scenarios, is that a good enough level of protection if saving the node goes haywire? In any case, will add these scenarios to the test case and see what happens.

Fixing the test cases and expanding on our new coverage based on #37.

Correct patch for #40.

+++ b/core/modules/content_moderation/tests/src/Kernel/ModerationStateFieldItemListTest.php
@@ -103,29 +104,69 @@ public function testGet() {
+    $violations = $this->testNode->validate();
+    $this->assertCount(1, $violations);
+    $this->assertEquals('State <em class="placeholder"></em> does not exist on <em class="placeholder">Editorial</em> workflow', $violations->get(0)->getMessage());

I'm not sure this is the correct violation that a user should receive. Wouldn't it be better to make the field required instead?

Thanks for the review.

I looked into making the field required (see first interdiff), this breaks down for non-moderated bundles and without #2935932: Add a FieldDefinition class for defining bundle fields in code. it wont be easy to override "required" for each bundle :)

I worked on a second approach for adding a nicer message to our existing validator which respects isModeratedEntity.

I have to say that I would have preferred the first approach, but we can always do it after #2935932: Add a FieldDefinition class for defining bundle fields in code. lands, and the cleanup provided by this patch is worth getting in sooner :)

Per #44 and #47, posted #2935932-110: Add a FieldDefinition class for defining bundle fields in code. to ensure we don't forget to do the indeed much nicer #44.1!

Once we have a field definition and field storage definition, we would actually be able to turn the moderation state field into a bundle field, so the validation message would be even better for non-moderated entities, the message would indicate that the field doesn't exist at all.