Unable to publish node content as admin: "The current user is not allowed to update the field 'changed'" error

@mayur-sose This is random error correct? Or does this happen on all nodes all the time?

Also does it happen for others with less permissions but who are still able to publish

I have seen this regularly in e2e tests - example - https://git.drupalcode.org/project/experience_builder/-/jobs/5966903#L4919

It appears to be random - as doesn't occur on re-test - but that might indicate its a race/timing issue - and being related to 'changed' that feels likely as some time passing may be the trigger.

Seeing this again in CI today https://git.drupalcode.org/project/experience_builder/-/jobs/5979572#L1351

After debugging a bit, I think it's because of this piece of code in ClientDataToEntityConverter

https://git.drupalcode.org/issue/experience_builder-3536247/-/blob/35362...

      if ($entity instanceof EntityChangedInterface && $name === 'changed') {
        $changed_timestamp = $items->first()?->get('value');
        assert($changed_timestamp instanceof Timestamp);
        $changed_timestamp_int = $changed_timestamp->getCastedValue();
        assert(is_int($changed_timestamp_int));
        $form_updated_changed_field = $changed_timestamp_int !== ((int) $entity_form_fields['changed']);
      }
    }

    assert(!is_null($entity->id()));
    $original_entity = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadUnchanged($entity->id());
    assert($original_entity instanceof FieldableEntityInterface);
    // Filter out form_build_id, form_id and form_token.
    $entity_form_fields = array_filter($entity_form_fields, static fn (string|int $key): bool => is_string($key) && $entity->hasField($key), ARRAY_FILTER_USE_KEY);
    // Copied from \Drupal\jsonapi\Controller\EntityResource::updateEntityField()
    // but with the additional special-casing for `changed`.
    foreach ($entity_form_fields as $field_name => $field_value) {
      \assert(\is_string($field_name));
      if ($field_name === 'changed' && $form_updated_changed_field) {
        continue;
      }
      try {
        $original_field = $original_entity->get($field_name);

There is a case when the $form_updated_changed_field value is not TRUE so is set to the original field, and the opposite. I still working on how to reproduce it reliably.

AFAICT \Drupal\Core\Entity\EntityChangedTrait::setChangedTime() is only called by tests, except for in one main spot and 3 spots in total:

1. \Drupal\Core\Entity\ContentEntityForm::updateChangedTime() (which is what we're hitting)
2. \Drupal\Core\Entity\Form\RevisionRevertForm::prepareRevision()
3. \Drupal\Core\Action\Plugin\Action\SaveAction::execute()

Related: shouldn't we remove all our explicit checks for 'changed' the field NAME and switch it over to checking 'changed' the field TYPE? 😅

One thing I noticed in 0.x is that it does not seem that change time is ever updated

With node or page

• create the entity - note the time
• wait a minute
• make an edit in XB
• wait a minute
• Publish the entity

If you look at the entity revisions list and the time is the same for all revisions

I tested a theory in the branch 3536040-ensure-unique-changed and it may have fixed it. I can't be 100% sure as I've only run it ~10 times so far and there's only one e2e running to make things faster, but it has yet to fail.

Although I couldn't reproduce locally, I ran logs on UnpublishedChanges.tsx and the manually created changed value was sometimes only 1 different from the existing one. This had me wondering if there are instances where, the value set could be identical to the prior one - if that were the case then $form_updated_changed_field = $changed_timestamp_int !== ((int) $entity_form_fields['changed']); might be FALSE and an unworthy changed does not get the desired continue; treatment.

  if ($field_name === 'changed' && $form_updated_changed_field) {
        continue;
      }

#9 is probably unrelated. opened another issue #3537709: Revision created timestamp not updated when editing in XB

@bnjmnm thanks for the investigation.

I originally wrote the logic around "changed" I think I made it overly complex to cover edge cases that probably won't happen

1. It assumed that it might be valid at some point to used the "changed" value sent from the client
2. It assumed that some entity types might have overridden entity forms that would not have the logic in \Drupal\Core\Entity\ContentEntityForm::updateChangedTime
3. It didn't consider the implications of allowing the client to send a "changed" value if 1) and 2) actually happened. Basically for the same entity some edits could be made inside XB where different clients would be the authority on revision timestamps and some edits would be made outside of XB where the server would the authority on revision timestamps

I think basically 2) is very unlikely to happen so1) is not needed. Even if 1 was needed 3) means there would probably other implications to consider

So made an MR to ignore "changed" from the client. https://git.drupalcode.org/issue/experience_builder-3536040/-/tree/35360...

Here is little debug module to get around the problem in #3537709: Revision created timestamp not updated when editing in XB so you can see when testing the changed time is actually still updated

I think issue summary needs to be updated. I can do that tomorrow but until then this is where I think it stands

I detailed in the problem and why @bnjmnm's MR could still result in random fails in test in comments here https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

I do think now the proper solution is ignore the "changed" value from the client because was always being ignore except to determine if access was going to be checked. That is done in my MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/1351. I have explained the reasoning of why I think it is ok in #15

It would be possible to write a test by mocking the request time, through a new class like \Drupal\update_test\Datetime\TestTime::getRequestTime and have the client send in the same timestamp. This should result in access error in 0.x but not in https://git.drupalcode.org/project/experience_builder/-/merge_requests/1351

Updated the summary based on this MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/1351

It doesn't require any front-end changes but it would probably be good idea for the front-end to also stop sending changed but it could be in follow-up

After reviewing both MR, I think I feel mor comfortable with MR 1351 (ignoring changed). https://git.drupalcode.org/project/experience_builder/-/merge_requests/1351.

It doesn't require any front-end changes but it would probably be good idea for the front-end to also stop sending changed but it could be in follow-up

Agree, but I think you're covering it well by ignoring changed.

After reviewing and testing, I agree with @isholgueras that for time being MR1351 seems like a better solution.

https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... is … 🤪😄

Glad to see this one done :) Closed https://git.drupalcode.org/project/experience_builder/-/merge_requests/1350.