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?

Proposed resolution

Move this logic to the entity class itself. But first, #2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously must be fixed.

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

CommentFileSizeAuthor
#3 entity_prepare.patch6.65 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity_prepare_0.patch. Unable to apply patch. See the log in the details link for more information. View
entity_prepare.patch6.03 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 48,741 pass(es), 453 fail(s), and 174 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

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
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity_prepare_0.patch. Unable to apply patch. See the log in the details link for more information. View

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-2] 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-2] 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 node form, 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.