Problem/Motivation
In #2891215-107: Add a way to track whether a revision was default when originally created and following it was brought up that the semantics of the newly added revision_default
field may be unclear for REST API consumers, because of its nature of internally-handled read-only field, especially given that entity revisions are not supported yet by the REST module. On top of that it was also brought up that at the moment REST API clients have no way to specify whether a revision should be default or pending, when creating a new one. In fact this is not achieved via a field in the PHP API, but via a runtime property read and populated via the RevisionInterface::isDefaultRevision()
method that is not exposed by the REST API.
Proposed resolution
Use the revision_default
field itself as a "setter": if it's set to 1 when POSTing a new revision, this would be created as default, while 0 would trigger the creation of a pending revision. Achieve this by internally calling ::isDefaultRevision($revision_default)
when the revision_default
field is populated. This would be entirely consistent with the logic that currently populates the revision_default
field on save.
Basically HTTP clients would rely on the revision_default
field to know whether the revision they are handling was default when created, and to set its default status when creating a new one. To determine whether a revision is the current default revision, they would need to load the current default revision and compare its revision ID with the ID of the revision being handled.
It was also suggested to mark revision_default
as internal until REST supports entity revisions, which would make it no longer appear in REST API responses.
Remaining tasks
- Validate the proposed solution
- Write a patch
- Reviews
- Update the related change record
User interface changes
None
API changes
Setting the revision_default
field will update the revision's default state accordingly.
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#10 | 2937850-10.patch | 5.5 KB | Wim Leers |
#10 | interdiff-8-10.txt | 1.22 KB | Wim Leers |
#8 | interdiff-6-8.txt | 4.02 KB | Wim Leers |
#8 | 2937850-8.patch | 4.72 KB | Wim Leers |
#6 | 2937850-6.patch | 738 bytes | Wim Leers |
Comments
Comment #2
plach@Wim Leers, #126:
Yep, it would mostly behave as a regular field, with the side of effect of also setting the internal
$isDefaultRevision
property, that is used by the storage to determine whether a new revision should be recorded as such in the base table.Yep, it would basically default to TRUE, which is the default value of
$isDefaultRevision
.A GET request to the canonical URI should return the default revision. We will need a different URI to load entity revisions, I guess something equivalent to
/node/{node}/revisions/{node_revision}/view
working for every entity.Comment #3
gabesulliceWould/should this new field work for PATCH requests too? If so, then can it ever be changed to 0?
How would one revert to a previously default revision?
The easy answer might be that one should only include the field in the request when one wishes to change the entity's status, but I feel like that's a particularly easy detail to get bitten by.
Comment #4
plachNope, the revision default status cannot be changed. We would probably need a validator to ensure this.
That means creating a new revision, so a regular POST should be used.
Yep, a validator would be the best choice here, I think.
Comment #5
gabesulliceExample:
I may fundamentally be misunderstanding revisions. That confusion is probably centered around whether revisions are mutable or not.
Regardless, I think we should definitely consider that the URL/HTTP semantics instead of including that as a "magic" field.
Comment #6
Wim LeersPer @plach's comment at #2860097-180: Ensure that content translations can be moderated independently yesterday, only now can we start focusing on this issue, because all of the hyper-critical translations + revisions + content moderation patches landed.
A quick recap:
#2891215: Add a way to track whether a revision was default when originally created introduced a data model change that resulted in an API change (in REST, JSON API and GraphQL). That change is adding a field. Adding a field is not a BC break. But this the field does not really make sense to HTTP API consumers. If we end up removing it in a future release, then that would be a BC break.
Hence bumping to critical.
@plach outlines a potential change to how the
revision_default
field behaves, so that it's no longer just a read-only field, but one that has actual purpose. I completely support exploring that. But we only have a few days until RC, at which point this cannot change anymore. And figuring out those semantics in a few days seems impossible (see #2 through #5 for starters), especially because we have zero experience with revision handling via Drupal's HTTP APIs. So there's a huge risk that we get it wrong.@plach also describes the alternative solution: marking
revision_default
as internal. Consequently, there would be no change at all in HTTP APIs. 8.5 would behave exactly as 8.4. Given the extremely constrained time, this is IMHO the only viable approach.Attached is a patch that marks the
revision_default
field as internal, and hence removes all risk. It's a single-line patch.Comment #8
Wim LeersAs expected, #6 causes all tests to pass, except REST tests that have been updated to expect
revision_default
. We can now change those expectations back to not expect that field. And all is well :) (There's only one weird case:EntityTranslationNormalizeTest
, which I'd never even seen until today. It does some very questionable assertions, but fixing that is out of scope here.)Also, no need for change record updates, because https://www.drupal.org/node/2936349 didn't mention that this affected REST responses. Finally, this is not an API change, but prevents an API change. PHP APIs continue to work exactly as they do in HEAD. HTTP APIs continue to work exactly as they did in 8.4.
Comment #10
Wim LeersForgot one bit.
Comment #11
plachCool, thanks!
We will have to revisit this once we actually support revisions in Rest, but for now this is a good stop-gap fix.
Comment #12
catchComment #14
catchCommitted/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!