Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
- the "last changed" timestamp is not updated when updating entities via REST
- the "create new revision" setting on node types is not respected when updating entities via REST
Proposed resolution
Fix this, and add test coverage.
Remaining tasks
- Fix the underlying root cause, in #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
- Add test coverage in this issue
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#27 | 2838395-27.patch | 8.29 KB | Wim Leers |
#12 | patch-revision-user-and-change-date-2838395-12.patch | 3.92 KB | Grayside |
#7 | patch-revision-user-and-change-date-2838395-7.patch | 3.9 KB | Grayside |
Comments
Comment #2
mpastas CreditAttribution: mpastas at Globant commentedI'm patching the node like this:
{{host}}/node/1?_format=json using basic auth and switching the users that edit the node.
Comment #3
mpastas CreditAttribution: mpastas at Globant commentedI solved it temporary forcing the revision creation from the hook_node_presave
Comment #4
cilefen CreditAttribution: cilefen commentedI 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".
Comment #5
cilefen CreditAttribution: cilefen commentedComment #6
Grayside CreditAttribution: Grayside at Phase2 for Norwegian Cruise Line commented\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...Comment #7
Grayside CreditAttribution: Grayside at Phase2 for Norwegian Cruise Line commentedUntested, but this is the crude direction. Not sure if Drupal core wants these kinds of side effects on PATCH.
Comment #8
Grayside CreditAttribution: Grayside at Phase2 for Norwegian Cruise Line commentedComment #9
cilefen CreditAttribution: cilefen commentedThere is a patch, we may as well test it.
Comment #10
cilefen CreditAttribution: cilefen commentedThere is a workaround so this is normal priority.
Comment #12
Grayside CreditAttribution: Grayside at Phase2 commentedMissed that $time should be $this->time.
(Also miss-filed this patch into another issue weeks ago.)
Comment #13
Grayside CreditAttribution: Grayside at Phase2 commentedAlso, 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.
Comment #15
Wim LeersPATCH
ing aNode
, 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.changed
,revision_timestamp
andrevision_uid
fields. Thechanged
field is expected to always be updated automatically whenever\Drupal\Core\Entity\EntityChangedInterface
is implemented by an entity type. And therevision_timestamp
andrevision_uid
fields are expected to always be updated automatically whenever an entity type's 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:
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
PATCH
ing!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.
Comment #16
Wim LeersPer #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.
Comment #17
Wim LeersSo, I discussed this on IRC with @tstoeckler and @berdir:
So I'm now thinking that we can deal with the "create new revision" aspect here. Patch upcoming.
Comment #18
Wim LeersRerolled #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.)Comment #20
Wim LeersRandom CI fail. Retesting.
Comment #22
Berdirthis is on NodeType and NodeTypeInterface does not document a return value. The method has the same name but it has a different meaning.
I guess this fails because not all entity types have a bundle entity type.
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.
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.
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.
Comment #23
Wim Leersrevision_uid
andrevision_timestamp
are read-only perNodeAccessControlHandler::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 theNodeType
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 forisRevisionable()
instead, and moved theRevisionLogInterface
further down, to be the condition/requirement to set the owner plus creation time.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?Comment #25
Wim Leers#22.2 wasn't fixed correctly in #23.
Comment #27
Wim LeersThe 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.
Comment #29
Wim LeersCI crashed super hard: https://dispatcher.drupalci.org/job/drupal_patches/8845/console. Retesting.
Comment #30
Wim LeersGreen! :)
Curious to hear @berdir's and @tstoeckler's thoughts!
Comment #31
tstoecklerYay, 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!
As far as I can tell this will fatal for revisionable entities without bundle entities?
This is something that I think
setNewRevision()
could trigger itself at some point, but definitely out of scope here, just wanted to note that.Comment #32
Wim Leers\Drupal\Tests\rest\Functional\EntityResource\User\UserJsonBasicAuthTest
— see the annotation of
\Drupal\user\Entity\User
Comment #33
tstoecklerUser
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.Comment #34
Wim LeersOh right, sorry — thanks! Do we have an entity type that is revisionable but does not have a bundle?
Comment #35
tstoecklerEntityRevTest
is revisionable and doesn't have a bundle entity (though it does have bundles).Comment #36
Wim LeersSo I'll need to add
EntityRevTestJsonBasicAuthTest
etc, then that will have the necessary test coverage. Will do.Comment #37
Wim LeersComment #38
Wim LeersIS rewritten.
Comment #39
Wim LeersComment #41
Wim Leers#2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously landed in 8.5!
Comment #44
jibranAlmost 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. :)
The
$original_entity
should be passed to\Drupal\Core\Entity\TranslatableRevisionableStorageInterface::createRevision()
before updating the field values.Something like this:
And this should be done before
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.
Comment #45
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 #52
stephencamilo CreditAttribution: stephencamilo as a volunteer commentedComment #53
hestenetReset issue status.