Problem/Motivation

While an entity type's form controller class might be the primary form by which to edit an entity, it should be possible to create additional forms: for example, an in-place editing form, as in #1824500: In-place editing for Fields. Also, entities can be edited outside of forms entirely, via RESTful web services, as in #1826688: REST module: PATCH/update.

What is currently done by \Drupal\Core\Entity\ContentEntityForm::buildEntity(), \Drupal\Core\Entity\ContentEntityForm::updateChangedTime() and friends has nothing to do with forms: it's just about doing further manipulation of $entity that wasn't done during entity object creation and entity loading for … legacy reasons?

The problem is that all these "things that should happen automatically when the entity is modified" only happen when the entity is modified via the entity edit form. But those things should also happen when:

  1. using in-place editing (proposed work-around: #2925846: Quick Edit only creates revision for Node entities, should do it for all content entity types that support it)
  2. using the core REST module (proposed work-around: #2838395: [PP-1] Entity API bug affecting REST: "last changed" timestamp not updated and "create new revision" setting not respected when PATCHing via REST)
  3. using the contrib JSON API module
  4. using the contrib GraphQL module

Proposed resolution

Move this logic to the entity classes themselves, or wherever appropriate. Basically, the following should happen automatically at entity save time:

  1. if the entity is revisionable, and it has a bundle entity type which implements \Drupal\Core\Entity\RevisionableEntityBundleInterface, respect the setting stored in the bundle entity
  2. if the entity has a new revision, automatically set the revision creation time
  3. if the entity is being created in a request where a user is authenticated, that user should be the author of the revision
  4. if the entity implements EntityChangedInterface, set the updated time

Remaining tasks

  1. #2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously
  2. TBD

User interface changes

TBD

API changes

TBD

Data model changes

TBD

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Note that the above patch doesn't move the implementation of ViewFormControllerBase::prepareEntity(), because there seems to be a little more coupling there to the form itself. For example, the "add" and "clone" forms skip that preparation.

Status: Needs review » Needs work

The last submitted patch, entity_prepare.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
6.65 KB

ViewUI is a strange class.

Wim Leers’s picture

#3: entity_prepare.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, entity_prepare.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Title: Move entity preparation from form controller to entity class » 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
Category: Task » Bug report
Priority: Normal » Major
Issue summary: View changes
Status: Needs work » Active
Issue tags: +Contributed project blocker, +blocker

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

This technical debt in Entity API already has introduced technical debt in the Quick Edit module. And now it risks introducing technical debt in the REST module (and hence also the JSON API contrib module). And in fact, this impacts anybody doing entity updates using the Entity API.

Given the impact of this, and the "soft data loss" (not creating revisions when users expect new revisions to be created…), I think this is definitely major.

Berdir’s picture

Not sure, for one thing, I'm not sure if an load + save on the API should default to a new revision, that would be a behavior change for existing code, not convinced that's a bugfix for every situation.

And doing that would also require us to fix #2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously first, as you can't undo a setNewRevision() currently.

The forced setting of changed is AFAIK just a workaround because the API isn't working properly, hopefully that will eventually work as expected once all those bugs around translated and non-translated fields are resolved.

tstoeckler’s picture

hook_node_prepare() no longer exists. I assume you mean various #entity_builder's, but not sure. Will look at #2838395: [PP-1] Entity API bug affecting REST: "last changed" timestamp not updated and "create new revision" setting not respected when PATCHing via REST now, but in any case the issue summary needs a refresher.

Wim Leers’s picture

Title: 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 » [PP-1] 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

#11:

Not sure, for one thing, I'm not sure if an load + save on the API should default to a new revision, that would be a behavior change for existing code, not convinced that's a bugfix for every situation.

Only if the entity type (or bundle if it's an entity type with bundles) has a setting that says "create new revision by default". i.e. \Drupal\node\NodeTypeInterface::isNewRevision() or its successor \Drupal\Core\Entity\RevisionableEntityBundleInterface::shouldCreateNewRevision().

And doing that would also require us to fix #2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously first, as you can't undo a setNewRevision() currently.

Okay, this issue is PP-1 then. And #2838395 is PP-2.


#12: the IS definitely needs an update after 4 years…

Wim Leers’s picture

Issue tags: +API-First Initiative
catch’s picture

fwiw I do think load and save on the API should default to a new revision, even though it's a behaviour change. #2696555: On the entity form of revisionable entities, make "create new revision" and "revision log" configurable is related for the UI.

Wim Leers’s picture

Issue tags: +Entity Field API
Wim Leers’s picture

Wim Leers’s picture

I think #2669802: Add a content entity form which allows to set revisions actually made this issue a whole lot easier already! 🎉

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes

Update IS to clarify where updating of "last changed" timestamp happens today: \Drupal\Core\Entity\ContentEntityForm::updateChangedTime().

Wim Leers’s picture

Status: Active » Postponed

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Title: [PP-1] 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 » 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
Version: 8.4.x-dev » 8.5.x-dev
Status: Postponed » Needs review
Issue tags: +Needs tests
Wim Leers’s picture

In working on this, I ended up creating #2925846: Quick Edit only creates revision for Node entities, should do it for all content entity types that support it, to already make Quick Edit entity type-agnostic (rather than coupled to Node). We should be able to remove that code once we're done here though.

This issue was originally created against the entity system component, and was a blocker to improved quickedit.module functionality (and maintainability) as well as rest.module. But the patch in #3 is against code that is A) no longer recognizable as D8 code, B) no longer in D8, because it dates back to a time very early in the D8 cycle.
In #17 and #20, I already updated the IS to reflect today's reality.

The problem is that all these "things that should happen automatically when the entity is modified" only happen when the entity is modified via the entity edit form. But those things should also happen when:

  1. using in-place editing (proposed work-around: #2925846: Quick Edit only creates revision for Node entities, should do it for all content entity types that support it)
  2. using the core REST module (proposed work-around: #2838395: [PP-1] Entity API bug affecting REST: "last changed" timestamp not updated and "create new revision" setting not respected when PATCHing via REST)
  3. using the contrib JSON API module
  4. using the contrib GraphQL module

The attached patch should fail (hard): it removes that logic that is currently hardcoded in the "full entity edit form", and which should be part of Entity API/Field API/Typed Data API, i.e. at a lower level.

Wim Leers’s picture

Issue summary: View changes
Related issues:

Status: Needs review » Needs work

The last submitted patch, 24: 1863258-24-FAIL.patch, failed testing. View results

hchonov’s picture

The problem is that all these "things that should happen automatically when the entity is modified" only happen when the entity is modified via the entity edit form.

We've added the logic for updating the changed timestamp to ContentEntityForm, so that the timestamp is being updated even if the entity hasn't been changed. It is there only for this case, for all other cases even if it has been modified in the form we have ChangedItem::preSave(), which should take care of updating the timestamp when the entity is being saved with changes.

We've made two decisions:

  1. If an entity is saved over its form without changes, the changed timestamp has to be updated.
  2. If the entity is being saved over the Entity API without changes, the changed timestamp should not be updated.
Wim Leers’s picture

What's the reasoning behind those two decisions?

Because if want to stick with that reasoning, this effectively becomes Closed (works as designed), which would of course be wonderful (one less entity system major!).

hchonov’s picture

We've introduced and defined this behaviour in #2506213: Update content entity changed timestamp on UI save. Both @alexpott and @webchick have agreed on it in #4 and #26 on the referenced issue - #2506213-4: Update content entity changed timestamp on UI save and #2506213-26: Update content entity changed timestamp on UI save.

dawehner’s picture

At least for me I agree with the comments linked in #29. Just by the introducing of user interaction in forms we introduce the concept of actual time. Given that we should have the same behaviour in REST. Saving an entity via REST should do the same as the UI, but not necessarily the same as the PHP API.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

AaronMcHale’s picture

Removed duplicate related issue, added #2524250: RevisionableInterface::preSaveRevision() should move to the storage layer since it appears to be relevant in some way, maybe even just a duplicate in that this issue might fix that issue?

I ended up here because it bothered me that every Core module and any contrib/custom module that has revisionable entities needs to implement RevisionableInterface::preSaveRevision to prevent the revision log message from being erased if the user chooses not to create a new revision. Figured this issue was probably the best place to fix that rather than open a new issue for it.

xjm’s picture

Priority: Major » Critical

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.

hchonov’s picture

Not sure, for one thing, I'm not sure if an load + save on the API should default to a new revision, that would be a behavior change for existing code, not convinced that's a bugfix for every situation.

I agree with @Berdir here and

Just by the introducing of user interaction in forms we introduce the concept of actual time. Given that we should have the same behaviour in REST. Saving an entity via REST should do the same as the UI, but not necessarily the same as the PHP API.

with @dawehner here.

If a REST call should result into a new revision then this should be an explicit and not an implicit action.

Wim Leers’s picture

If a REST call should result into a new revision then this should be an explicit and not an implicit action.

Remember, it's not just about REST, but also Quick Edit. That's the original reason this issue was created: #1824500: In-place editing for Fields + #2925846: Quick Edit only creates revision for Node entities, should do it for all content entity types that support it.

Also: when Content Moderation is installed, @effulgentsia's testing shows that creating a new revision does happen implicitly (automatically).

hchonov’s picture

Also: when Content Moderation is installed, @effulgentsia's testing shows that creating a new revision does happen implicitly (automatically).

CM makes explicit use of the entity API and also explicitly requests a new revision on save for moderated entities in \Drupal\content_moderation\Entity\Handler\ModerationHandler::onPresave(). At least to my understanding this is an explicit action - it is not placed in the core entity API and happens in the pre-saving process of entities maintained by the module content moderation.

This means that if a REST call updates a moderated entity then it will be saved in a new revision. This however is not a decision of the entity API but of content moderation.

hchonov’s picture

REST can do something similar by default, but should make it possible to opt-out.

Wim Leers’s picture

#37 is indeed a very important nuance and a crucial difference. Thanks for pointing that out so clearly! 🙏

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

stephencamilo’s picture

Status: Needs work » Closed (won't fix)
hestenet’s picture

Status: Closed (won't fix) » Needs work

Reset issue status.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

This is a bug smash fortnightly triage target.

Reading through the issue it looks like the Issue Summary needs to be updated. Adding tag.

quietone’s picture

Issue tags: +Bug Smash Initiative

Sorry for the noise, I forgot a tag.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.