[PP-1] Entity API bug affecting REST: "last changed" timestamp not updated and "create new revision" setting not respected when PATCHing via REST

I can't seem to find a duplicate for this one. This is borderline major if it "Cause(s) a significant admin- or developer-facing bug with no workaround".

\Drupal\rest\Plugin\rest\resource\EntityResource::patch() needs to update the uid and change date, presumably only if they are not set in the patch request. I've encountered this seeing that the change date wasn't updating when I assumed it would be handled automatically. Going to take a pass at this...

Untested, but this is the crude direction. Not sure if Drupal core wants these kinds of side effects on PATCH.

There is a patch, we may as well test it.

There is a workaround so this is normal priority.

Missed that $time should be $this->time.

(Also miss-filed this patch into another issue weeks ago.)

Also, note the Time service is only available in 8.3, this issue is filed against 8.2 right now, so we can expect this patch to fail.

1. First: note that the D8 REST API does not support interacting with revisions. Yes, it's possible that when you're PATCHing a Node, that we're automatically creating new revisions under the hood. But we don't allow you to explicitly create new revisions, nor do we expose revisions (listings, metadata) in any way.
2. Second: this is specifically about updating the changed, revision_timestamp and revision_uid fields. The changed field is expected to always be updated automatically whenever \Drupal\Core\Entity\EntityChangedInterface is implemented by an entity type. And the revision_timestamp and revision_uid fields are expected to always be updated automatically whenever an entity type's Create new revision setting is enabled.

This issue is not about point 1 (revisions), but point 2 (automatically setting "last changed" time and automatically creating "new revision" if the entity type or bundle setting is configured to do that).

Funny enough… I encountered this very problem before, while working on the Quick Edit module. Quick Edit introduced (when it was added in #1824500: In-place editing for Fields) code like this:

    // @todo Rather than special-casing $node->revision, invoke prepareEdit()
    //   once https://www.drupal.org/node/1863258 lands.
    if ($entity->getEntityTypeId() == 'node') {
      $node_type = $this->nodeTypeStorage->load($entity->bundle());
      $entity->setNewRevision($node_type->isNewRevision());
      $entity->revision_log = NULL;
    }

So, about 4 years ago, this was already a known problem. The solution was deferred to #1863258: Move entity preparation from form controller to entity class: "last changed" timestamp not updated and "create new revision" setting not respected when updating entities via Entity API. That issue didn't move forward at all in the time that has passed. And it even explicitly mentioned REST PATCHing!

The problem is again the same: do we add work-around in the module whose behavior is perceived as broken (REST today, Quick Edit then), or do we fix the root cause (in Entity API)? We opted for the former then. But since REST shipped with this problem ever since Drupal 8.0.0 was shipped more than 16 months ago, I think there's no point in rushing in a quick fix that increases the technical debt in the REST module and leaves the technical debt in Entity API unaddressed. We should fix the root cause, and reduce our technical debt.

So, postponing this on #1863258: Move entity preparation from form controller to entity class: "last changed" timestamp not updated and "create new revision" setting not respected when updating entities via Entity API.

Per #1863258-13: Move entity preparation from form controller to entity class: "last changed" timestamp not updated and "create new revision" setting not respected when updating entities via Entity API, this is PP-2, since #1863258 is blocked on #2544790.

So, I discussed this on IRC with @tstoeckler and @berdir:

15:06:51 <tstoeckler_> WimLeers: interesting, thanks
15:29:31 <tstoeckler_> WimLeers: I think I disagree with your assessment a bit, to be honest. Let me know if I'm getting this wrong, but while you're arguing for "form" === "rest" (which I agree with), moving stuff to the entity system itself would result in "rest" === "$entity->save()" and I'm not sure if a simple save should trigger a whole bunch of magic (i.e. changed/revision_uid/...)
15:30:50 <tstoeckler_> WimLeers: So I'm not sure that putting stuff e.g. in RestResource (or wherever) would actually be that much of a workaround.
15:31:34 <tstoeckler_> WimLeers: I mean we could make it e.g. so that when you call ->setNewRevision(TRUE) that the revision_uid and revision_timestamp magic is invoked, but then RestResource would still call setNewRevision(TRUE) explicitly. Do you see what I'm getting at?
16:00:12 <WimLeers> tstoeckler_: I'm arguing that the special logic belongs in either Entity API in general or in specific entity types' ::preSave(), NOT in forms, NOT in Quick Edit, NOT in REST. Because it's logic tied to entities in general or a particular entity type in particular
16:01:01 <tstoeckler_> WimLeers: Right, and I'm not sure I agree ;-)
16:01:06 <WimLeers> tstoeckler_: I'd rather continue to have things work "incorrectly" in REST than adding a work-around, because otherwise we're again duplicating (so triplicating…) code that we then need to test again separately, all because it's a leaky abstraction
16:01:44 <WimLeers> tstoeckler_: why could Node::preSave() not do $node->setNewRevision(NodeType::isNewRevision()) ?
16:01:58 <berdir1> WimLeers: how would you not save a new revision then?
16:02:13 <WimLeers> berdir1: ahaaaa
16:02:32 <WimLeers> berdir1: tstoeckler_ So "opt out of default" is a good point
16:02:47 <berdir1> WimLeers: while we're discussion to enforce it more, right now, that setting is *only* a default value for the checkbox in the UI, nothing more
16:02:49 <WimLeers> berdir1: tstoeckler_ That then seems like a confirmation of "leaky abstraction"
16:02:52 berdir1 → berdir
16:02:59 <WimLeers> berdir: tstoeckler_ ok
16:03:26 <WimLeers> berdir: tstoeckler_ I can see that reasoning. But if we apply that reasoning to REST, then this is a "Closed (works as designed)" in REST
16:03:38 <WimLeers> berdir: tstoeckler_ because then the burden is on the API client to set the appropriate fields
16:03:44 <WimLeers> berdir: tstoeckler_ I can live with that
16:03:53 <WimLeers> berdir++
16:04:03 <WimLeers> tstoeckler_: sorry, berdir's explanation/question just "clicked" for me, yours did not :)
16:04:20 <berdir> WimLeers: except "save as new revision" is not a field, so rest can't really set it?
16:04:36 <tstoeckler_> WimLeers: I do think we can improve things. Like I said above, I think we should do the revision_uid/timestamp magic when someone does ->setNewRevision
16:04:42 <tstoeckler_> WimLeers: no worries :-)
16:04:59 <WimLeers> berdir: but REST can set revision_timestamp and revision_log
16:04:59 <berdir> WimLeers: and it's also a permission thing. it's just a default *but* only users with  "administer nodes" can change that default
16:05:12 <WimLeers> berdir: oh but yes basically this is "you need a separate resource to create a new revision"
16:05:13 <tstoeckler_> WimLeers, berdir: but e.g. automatically calling setNewRevision() is tricky I think
16:05:19 <tstoeckler_> WimLeers, berdir: changed I could see either way
16:05:30 <WimLeers> yeah, "changed" I think totally should happen by default 
16:05:58 <berdir> tstoeckler_: as I commented, I think changed is only in the form because the automatic code has all kinds of bugs, that we're slowly fixing? and that some point, it shouldn't matter anymore if we call it or not.. ?
16:06:03 <pwolanin> jacobembree:  in  function drupal_retrieve_form() in D7 it's set to an empty array - I'm not sure why the param is there unless someoen wants to basically extend an existing form builder
16:06:03 → claudiu_cristea joined (~claudiu_c@86.120.24.124)
16:06:15 <berdir> tstoeckler_: well I guess except when you save without actually making a change :)
16:06:25 <tstoeckler_> berdir, WimLeers: I tend to generally agree.
16:06:27 <WimLeers> berdir: tstoeckler_ So I think we're agreeing that "auto create new revision" does NOT make sense for REST. No blame on Entity API there.
16:06:48 <WimLeers> berdir: tstoeckler_ But we're also agreeing that we think 'changed' _should_ happen automatically. It's a computed field that's recomputed on every save
16:06:51 <berdir> not sure we agree :)
16:07:00 <WimLeers> So which part do you disagree with?
16:07:12 <berdir> that making or not making sense for rest
16:07:24 <berdir> I'm just saying it is a lot more complicated than "entity API should just do it automatically"
16:07:27 <tstoeckler_> berdir, WimLeers: yeah, I actually I don't understand why that's currently not happening
16:07:36 <tstoeckler_> (I mean changed)
16:07:41 → saketkumar joined  ⇐ roderik and juampynr quit  
16:08:51 <berdir> WimLeers: if you want feature parity with the UI, you need a way to override the default for users with the necessary permission (not sure how, a fake field in the serialized array?) and probably respect that setting for non-admins..
16:08:53 → greggles and prestonso joined  
16:11:08 <jacobembree> pwolanin: Thanks.
16:11:31 <WimLeers> berdir: well I do think it doesn't make sense at this point
16:11:47 <WimLeers> berdir: because "create new revision" is a _default_ that you can change the value for in the UI
16:11:56 <WimLeers> berdir: so what this is about, is choosing to create a new revision or not
16:12:05 <WimLeers> berdir: but there's NO WAY to make that decision via REST
16:12:11 <WimLeers> berdir: because there's non field
16:12:18 ⇐ saketkumar quit (~saketkuma@14.141.151.206) Ping timeout: 260 seconds
16:12:20 <berdir> WimLeers: .. that you can change.. *IF* you are an admin
16:12:21 <WimLeers> *no field to choose that, since there's no field to represent that
16:12:34 <berdir> if you want to enforce revisions on a decoupled site, then we need to dd *something*
16:12:44 <WimLeers> right
16:12:50 <WimLeers> that's the angle I was first looking from
16:12:54 <WimLeers> tstoeckler_: ^^ -> thoughts?
16:13:13 → rho joined (~rho@drupal.org/user/114058/view)
16:14:03 <pwolanin> jacobembree:  probably people setting it to an empty array copied or upgraded some D6 code where you did not get a $form variable passed in
16:14:04 <berdir> WimLeers: we're also adding non-field properties for example to file and image fields, technically, we can add whatever we want to a serialized document and then the serializer can call setNewRevision()
16:14:14 <tstoeckler_> tstoeckler_: I agree, but conceptually I think would be OK if the Resource plugin did that call. The question is just based on what decision (i.e. what Berdir is referencing)
16:14:30 <WimLeers> tstoeckler_: sorry, can you rephrase? I can't parse that
16:15:04 <WimLeers> berdir: adding new properties to fields when normalizing or denormalizing !== adding new fields entirely — that's much harder to support
16:15:04 → coltrane_ joined  ⇐ coltrane quit  
16:15:29 <WimLeers> (not to mention to document)
16:15:46 <berdir> WimLeers: I'd argue it's trivial to support in \Drupal\hal\Normalizer\ContentEntityNormalizer::denormalize
16:15:46 <tstoeckler_> WimLeers: I.e. for me it would make sense that if you wanted (auto-)revisions via Rest, then I would expect the Resource plugin to make a call to ->setNewRevision()
16:15:53 <berdir> WimLeers: and other similar normalizers
16:16:35 <WimLeers> berdir: but it won't work for _every_ entity type. And there'd be no way to let https://www.drupal.org/project/schemata expose this new pseudo-field.
16:16:38 <tstoeckler_> WimLeers: But the question is if we do not want two different plugins just because of that we need some way to configure whether or not to call ->setNewRevision().
16:22:07 <berdir> WimLeers: schemata will also not be able to generically provide schema for the image style urls? how is that different?
16:22:40 <berdir> WimLeers: and another option would be to make it a http header or so that is outside of the serialized string
16:23:19 <berdir> WimLeers: and any entity type should work with that as long as it hasn't replaced the normalizer/uses a supported format
16:23:53 <berdir> ok, permission handling is a problem, we have no generic handling for that yet, then it would have to be node specific for now. but so is the UI
16:24:34 <berdir> actually that's not true
16:24:41 <berdir> we solved that problem when adding the generic revision UI
16:25:12 <berdir> WimLeers: '#access' => !$this->entity->isNew() && $this->entity->get($entity_type->getKey('revision'))->access('update'),, you can control revision if you edit the revision id field

So I'm now thinking that we can deal with the "create new revision" aspect here. Patch upcoming.

Rerolled #12, removed the changed field part of it, added proper solution for the "create new revision" part, and added test coverage.

(Note that "PP-2" still applies and #15 still applies for the changed field portion of this issue. We might want to split this off.)

00:00:00.349 Warning: failed to get default registry endpoint from daemon (Cannot connect to the Docker daemon. Is the docker daemon running on this host?). Using system default: https://index.docker.io/v1/

Random CI fail. Retesting.

1. diff --git a/core/modules/node/src/Entity/NodeType.php b/core/modules/node/src/Entity/NodeType.php
   index e4a4c3b..002beb9 100644
   
   index e4a4c3b..002beb9 100644
   --- a/core/modules/node/src/Entity/NodeType.php
   
   --- a/core/modules/node/src/Entity/NodeType.php
   +++ b/core/modules/node/src/Entity/NodeType.php
   
   +++ b/core/modules/node/src/Entity/NodeType.php
   @@ -126,6 +126,7 @@ public function isNewRevision() {
   
   @@ -126,6 +126,7 @@ public function isNewRevision() {
       */
      public function setNewRevision($new_revision) {
        $this->new_revision = $new_revision;
   +    return $this;
      }
    
   

   this is on NodeType and NodeTypeInterface does not document a return value. The method has the same name but it has a different meaning.

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -84,12 +110,20 @@ class EntityResource extends ResourceBase implements DependentPluginInterface {
        parent::__construct($configuration, $plugin_id, $plugin_definition, $serializer_formats, $logger);
        $this->entityType = $entity_type_manager->getDefinition($plugin_definition['entity_type']);
   +    $bundle_entity_type = $this->entityType->getBundleEntityType();
   +    $this->bundleEntityTypeStorage = $entity_type_manager->getStorage($bundle_entity_type);
        $this->configFactory = $config_factory;
   

   I guess this fails because not all entity types have a bundle entity type.

3. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -262,6 +298,16 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +      if ($bundle_entity instanceof RevisionableEntityBundleInterface) {
   +        $original_entity->setNewRevision($bundle_entity->shouldCreateNewRevision());
   +      }
   

   I guess the question is now how and if we allow to override this if you have the necessary permission.

   Also note that setNewRevision() is not RevisionLogInterface, so I'd argue that should be outside of that, any content entity has that method, but we should check for ->getEntityType()->isRevisionable() instead.

4. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -262,6 +298,16 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +      $original_entity->setRevisionUserId($this->user->id());
   +      $original_entity->setRevisionCreationTime($this->time->getRequestTime());
   

   Yes, this part should possibly be further down, and part of it actually is, for example \Drupal\node\Entity\Node::preSave() + \Drupal\node\Entity\Node::preSaveRevision(). A few other entity types have that duplicated but we can likely generalize that now.

5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php
   @@ -66,7 +68,9 @@ protected function createEntity() {
            'name' => 'Camelids',
            'type' => 'camelids',
   -      ])->save();
   +      ])
   +        ->setNewRevision(FALSE)
   +        ->save();
   

   so that's why you changed it, but as I mentioned, that's not what the interface currently documents anyway and changing that is unrelated.

1. Yep, it doesn't, but IMO that's an oversight. Anyway, I don't want this to be a distraction, so reverted that change, and went to uglier code.
2. Fixed.
3. RE: how to override. Heh, before coming to your review here, that's exactly what I was thinking about :) Except not the "create new revision or not" part, but the "specify revision owner and creation time" portion. But revision_uid and revision_timestamp are read-only per NodeAccessControlHandler::checkFieldAccess(), so that answers my concern. The "create new revision or not" portion can only become overridable once we allow explicit interaction with entity revisions via REST IMO. On most sites, you also cannot customize this at all as a regular content editor. Only site administrators are usually allowed to override this default. In other words: respecting the NodeType setting is what you do 99% of the time. So I think that's also what we should do here.

   RE: RevisionLogInterface: I introduced a check for isRevisionable() instead, and moved the RevisionLogInterface further down, to be the condition/requirement to set the owner plus creation time.

4. Hm… I see what Node::preSave(Revision)() are doing. But I think the current behavior here is correct: we want to log that the current user (as authenticated for this REST request) has created this revision. So I don't think anything needs to change here?
5. Yep, reverted for that reason.

#22.2 wasn't fixed correctly in #23.

The hardcoded normalization was specific to the default normalization, it didn't automatically use the HAL normalization when necessary. That caused the last 3 fails. Fixed now.

CI crashed super hard: https://dispatcher.drupalci.org/job/drupal_patches/8845/console. Retesting.

Green! :)

Curious to hear @berdir's and @tstoeckler's thoughts!

Yay, this patch is really neat in that it actually allows creating revisions via Rest in the first place. Obviously this is not full-featured Rest+Revisions integration, but it's still really, really cool!

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -84,12 +109,21 @@ class EntityResource extends ResourceBase implements DependentPluginInterface {
   +    if ($bundle_entity_type = $this->entityType->getBundleEntityType()) {
   +      $this->bundleEntityTypeStorage = $entity_type_manager->getStorage($bundle_entity_type);
   
   @@ -262,6 +298,18 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +    if ($original_entity->getEntityType()->isRevisionable()) {
   +      $bundle_entity = $this->bundleEntityTypeStorage->load($original_entity->bundle());
   

   As far as I can tell this will fatal for revisionable entities without bundle entities?

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -262,6 +298,18 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +      if ($original_entity instanceof RevisionLogInterface) {
   +        $original_entity->setRevisionUserId($this->user->id());
   +        $original_entity->setRevisionCreationTime($this->time->getRequestTime());
   +      }
   

   This is something that I think setNewRevision() could trigger itself at some point, but definitely out of scope here, just wanted to note that.

1. No, because we're already testing several entity types without bundles. E.g. \Drupal\Tests\rest\Functional\EntityResource\User\UserJsonBasicAuthTest
   — see the annotation of \Drupal\user\Entity\User
2. Cool :)

User is not revisionable. Just looking at the code $this->bundleEntityTypeStorage will not always be set when it's called because the conditions for setting and calling it are not congruent.

Oh right, sorry — thanks! Do we have an entity type that is revisionable but does not have a bundle?

EntityRevTest is revisionable and doesn't have a bundle entity (though it does have bundles).

So I'll need to add EntityRevTestJsonBasicAuthTest etc, then that will have the necessary test coverage. Will do.

IS rewritten.

#2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously landed in 8.5!

Almost repeating myself from #2993557-33: Allow optional creation of new revision when PATCHing revisionable entities to support autosave functionality via JSON:API.. Feel free to correct me if I'm wrong. :)

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -262,6 +299,18 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
+    if ($original_entity->getEntityType()->isRevisionable()) {
+      $bundle_entity = $this->bundleEntityTypeStorage->load($original_entity->bundle());

The $original_entity should be passed to \Drupal\Core\Entity\TranslatableRevisionableStorageInterface::createRevision() before updating the field values.
Something like this:

    if ($original_entity instanceof RevisionableInterface && !$original_entity->isLatestRevision()) {
      /* @var \Drupal\Core\Entity\ContentEntityStorageInterface $storage */
      $storage = $this->entityTypeManager->getStorage($type_id);
      $revision_id = $storage->getLatestRevisionId($entity->id());
      if ($revision = $storage->loadRevision($revision_id)) {
        $original_entity = $revision;
      }
      $original_entity = $storage->createRevision($original_entity, $original_entity->isDefaultRevision());
    }

And this should be done before

 
    // Overwrite the received fields.
    foreach ($entity->_restSubmittedFields as $field_name) {
      $field = $entity->get($field_name);
      // It is not possible to set the language to NULL as it is automatically
      // re-initialized. As it must not be empty, skip it if it is.
      // @todo Remove in https://www.drupal.org/project/drupal/issues/2933408.
      if ($entity->getEntityType()->hasKey('langcode') && $field_name === $entity->getEntityType()->getKey('langcode') && $field->isEmpty()) {
        continue;
      }
      if ($this->checkPatchFieldAccess($original_entity->get($field_name), $field)) {
        $original_entity->set($field_name, $field->getValue());
      }
    }

This will make sure a couple of things:

1. We always create new revision from the latest revision, not from the loaded entity, which is a default revision in this case. This is also Drupal default behavior i.e. node edit page always lets you edit the latest revision.
2. This will take care of translatable entity as well.

Discussed this issue today with @Wim Leers, @gabesullice, and @effulgentsia. I think that this is a data integrity issue and therefore critical.

This will affect the JSON:API module as well as the REST module.

Reset issue status.

In #15 this was marked postponed, presumably on #1863258: Move entity preparation from form controller to entity class: "last changed" timestamp not updated and "create new revision" setting not respected when updating entities via Entity API is blocked on #2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously. One has been fixed and the other Closed as outdated. So, this can be active. But maybe this is also outdated?

Is there work to be done here?