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:
- 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)
- 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)
- using the contrib JSON API module
- 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:
- 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 - if the entity has a new revision, automatically set the revision creation time
- if the entity is being created in a request where a user is authenticated, that user should be the author of the revision
- if the entity implements
EntityChangedInterface
, set the updated time
Remaining tasks
User interface changes
TBD
API changes
TBD
Data model changes
TBD
Comment | File | Size | Author |
---|---|---|---|
#24 | 1863258-24-FAIL.patch | 2.3 KB | Wim Leers |
#3 | entity_prepare.patch | 6.65 KB | effulgentsia |
entity_prepare.patch | 6.03 KB | effulgentsia | |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedNote 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.
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedViewUI is a strange class.
Comment #4
Wim Leers#3: entity_prepare.patch queued for re-testing.
Comment #9
Wim LeersThis 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.
Comment #10
Wim LeersComment #11
BerdirNot 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.
Comment #12
tstoecklerhook_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.Comment #13
Wim Leers#11:
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()
.Okay, this issue is PP-1 then. And #2838395 is PP-2.
#12: the IS definitely needs an update after 4 years…
Comment #14
Wim LeersComment #15
catchfwiw 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.
Comment #16
Wim LeersComment #17
Wim LeersIS updated.
#2238077: Rename EntityFormController to EntityForm, #2669802: Add a content entity form which allows to set revisions and others significantly changed this code, which is why the original patches no longer make sense at all.
Comment #18
Wim LeersI think #2669802: Add a content entity form which allows to set revisions actually made this issue a whole lot easier already! 🎉
Comment #19
Wim LeersComment #20
Wim LeersUpdate IS to clarify where updating of "last changed" timestamp happens today:
\Drupal\Core\Entity\ContentEntityForm::updateChangedTime()
.Comment #21
Wim LeersIf this is blocked on #2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously, then let's mark this .
Comment #23
Wim Leers#2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously landed in 8.5, this is now unblocked!
Comment #24
Wim LeersIn 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 improvedquickedit.module
functionality (and maintainability) as well asrest.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:
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.
Comment #25
Wim LeersComment #27
hchonovWe'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 haveChangedItem::preSave()
, which should take care of updating the timestamp when the entity is being saved with changes.We've made two decisions:
Comment #28
Wim LeersWhat's the reasoning behind those two decisions?
Because if want to stick with that reasoning, this effectively becomes
, which would of course be wonderful (one less entity system major!).Comment #29
hchonovWe'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.
Comment #30
dawehnerAt 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.
Comment #33
AaronMcHaleRemoved 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.
Comment #34
xjmDiscussed 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.
Comment #35
hchonovI agree with @Berdir here and
with @dawehner here.
If a REST call should result into a new revision then this should be an explicit and not an implicit action.
Comment #36
Wim LeersRemember, 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).
Comment #37
hchonovCM 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.
Comment #38
hchonovREST can do something similar by default, but should make it possible to opt-out.
Comment #39
Wim Leers#37 is indeed a very important nuance and a crucial difference. Thanks for pointing that out so clearly! 🙏
Comment #46
stephencamilo CreditAttribution: stephencamilo as a volunteer commentedComment #47
hestenetReset issue status.
Comment #49
quietone CreditAttribution: quietone commentedThis is a bug smash fortnightly triage target.
Reading through the issue it looks like the Issue Summary needs to be updated. Adding tag.
Comment #50
quietone CreditAttribution: quietone commentedSorry for the noise, I forgot a tag.