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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach created an issue. See original summary.

plach’s picture

@Wim Leers, #126:

This would mean revision_default would no longer behave like a computed field (in the committed patch you can't set it, only read it), even though it is not a computed field. It'd then behave like a "regular" field. Or are there still ways in which it would behave differently?

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.

This sounds good! But there's an edge case: what would happen when REST clients (or PHP code using the Entity API) would not specify a value? Would it then fall back to the behavior that was in the previously committed patch?

Yep, it would basically default to TRUE, which is the default value of $isDefaultRevision.

Sorry if this is a trivial question, but I can't find an obvious answer: how does one load the current default revision? [...]

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.

gabesullice’s picture

Would/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.

plach’s picture

Would/should this new field work for PATCH requests too? If so, then can it ever be changed to 0?

Nope, the revision default status cannot be changed. We would probably need a validator to ensure this.

How would one revert to a previously default revision?

That means creating a new revision, so a regular POST should be used.

[...] I feel like that's a particularly easy detail to get bitten by.

Yep, a validator would be the best choice here, I think.

gabesullice’s picture

That means creating a new revision, so a regular POST should be used.

  1. If this is the case, would that mean default revisions can never be PATCH'ed (effectively immutable)? If so, the only resources which can be PATCH'ed are non-revisionable resources and pending revisions, correct?
  2. I was going to nitpick POST and say that it sounds more like a PUT, which led me to a bigger idea (which doesn't necessarily mean it's a better idea...) that we should probably be using HTTP/URL semantics to define the revision_default behavior instead of a magic computed field.

Example:

GET /entity/<id> // Retrieves the default revision
POST /entity/<id> // Creates a new *default* revision
PATCH /entity/<id> // This would only be for non-revisionable entities. We *could* make this an alias of POST /node/<id>/revisions, but I wonder if that would just cause more headaches in the end.

POST /entity/<id>/revisions // Creates a new *pending* revision

GET /entity/<id>/revisions/<rid> // Gets a specific revisions; may also get the default revision.
PATCH /entity/<id>/revisions/<rid> // May update a *pending* revision. Would it be unacceptable to PATCH the default revision?

DELETE /entity/<id>/revisions/<rid> // Can revisions ever be removed?

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.

Wim Leers’s picture

Priority: Major » Critical
Status: Active » Needs review
FileSize
738 bytes

Per @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.

Status: Needs review » Needs work

The last submitted patch, 6: 2937850-6.patch, failed testing. View results

Wim Leers’s picture

As 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.

Status: Needs review » Needs work

The last submitted patch, 8: 2937850-8.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
5.5 KB

Forgot one bit.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Cool, thanks!

We will have to revisit this once we actually support revisions in Rest, but for now this is a good stop-gap fix.

catch’s picture

Title: Ensure the entity revision default state makes sense to REST API consumers » Mark revision_default as internal for REST consumers

  • catch committed 7eaf9ab on 8.6.x
    Issue #2937850 by Wim Leers, plach: Mark revision_default as internal...
catch’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

  • catch committed ab140c2 on 8.5.x
    Issue #2937850 by Wim Leers, plach: Mark revision_default as internal...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.