Add a version negotiation to revisionable resource types

Thanks for filing this! This would be the first and only PHP API that JSON API has. (See jsonapi.api.php.) If we add this API, then it'd definitely have to be marked @internal. At least initially.

That still would allow you on projects to add your own plugins. But the JSON API module would have no responsibility to retain BC for this PHP API.

I've started experimenting with this locally and have something that could apply.

Questions that I ran into while doing so:

1. In Drupal terms, what is a working copy?
2. Simarlarly, what is a latest revision?
   1. Without content_moderation installed and referencing the definitions given in RFC 5829, I think these are one and the same.
3. What should the self link be for a resource? Should it include the revision ID negotiator? Perhaps only when it is not the default revision.

Re: the proposed solution:

Can we change current_version to resource_version?

Can we not make this a plugin right now? Rather, let's have code that will make it easy to add different negotiators tomorrow, but hardcode this for now.

Finally, must this be a 1.x issue? This certainly feels like a 2.1 kind of thing. If not, I think we'll need parallel development tracks for 1.x and 2.x. I don't know that the 1.x version should then ever be committed, but instead just be a patch that will continue to apply since the 1.x is not receiving anything but critical support from at least 2 of the 3 most active maintainers.

Most importantly! Thanks for opening an issue! I'm super excited to see this progress!

I've started experimenting with this locally and have something that could apply.

Here's that code if it'll help. I got this started off 2.x and haven't tried it on 1.x.

UPDATE: I hadn't seen #2992836: Provide links to resource versions (entity revisions) when I posted this. This patch includes that feature but it's easily extracted.

since the 1.x is not receiving anything but critical support from at least 2 of the 3 most active maintainers.

🙃

This is what we plan on running for our codebase based on 1.x. Reviews will be greatly appreciated.

My current plan is to release and support this feature in 1.x, unless there is a strong reason not to.

Let's see how many of those we fix with these simple updates.

This patch negotiates the revision on a param enhancer. This is inspired on @gabesullice's patch in #6.

PROS of this approach: It's a very well scoped service that responds to URL input and modifies a parameter that will be used later on. It loads the revision for all places that it may be used now and the future.
CONS of this approach: It is a bit hacky to replace the value of a route parameter in the route. It uses a parameter enhancer when there is not a route parameter (we have a query string parameter).

Overall I like the new approach, but I'm willing to switch back to #10 if anyone has concerns about this.

Fixed DCS.

This adds support for collections when asking for rel:latest-version. This even works with filtering on future revisions thanks to Entity Query API!

Some additional minor fixes.

Maybe this will fix the tests.

#12: thanks for explaining PROs and CONs! I was going to ask about collection support, and how the two approaches compare when you add support for that.

But I see that you added that in #14, splendid! 🤘

Attached is a small contribution towards making this pass tests. 😊

#12: thanks for explaining PROs and CONs! I was going to ask about collection support, and how the two approaches compare when you add support for that.

Actually, I'd still like to get your thoughts on this.

In the case of collections, we need to perform an entity query, and hence in that scenario, the param enhancer does not help us at all.

Initial review:

1. +++ b/src/JsonApiSpec.php
   @@ -96,6 +96,13 @@ class JsonApiSpec {
   +  const VERSION_QUERY_PARAMETER = 'resource_version';
   
   +++ b/src/ResourceType/ResourceType.php
   @@ -196,6 +203,16 @@ class ResourceType {
   +   * Whether resources of this resource type are revisionable.
   +   *
   +   * @return bool
   +   *   TRUE if the resource type's resources are revisionable. FALSE otherwise.
   +   */
   +  public function isVersionable() {
   +    return $this->isVersionable;
   +  }
   
   +++ b/src/Revisions/ResourceVersionRouteEnhancer.php
   @@ -0,0 +1,121 @@
   +class ResourceVersionRouteEnhancer implements EnhancerInterface {
   
   +++ b/src/Revisions/RevisionIdNegotiationInterface.php
   @@ -0,0 +1,33 @@
   +interface RevisionIdNegotiationInterface {
   
   +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -456,6 +456,7 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      'url.query_args:resource_version',
   

   "versionable" and "revisionable" are being mixed up many times here.

   If we want "version" to be Drupal-agnostic, that's fine by me. But then we need to very clearly document that, and be consistent about it.

2. +++ b/src/Plugin/RevisionIdNegotiation/IdNegotiation.php
   @@ -0,0 +1,29 @@
   +class IdNegotiation extends PluginNegotiationBase implements RevisionIdNegotiationInterface {
   

   VersionById

3. +++ b/src/Plugin/RevisionIdNegotiation/RelNegotiation.php
   @@ -0,0 +1,76 @@
   +class RelNegotiation extends PluginNegotiationBase implements ContainerFactoryPluginInterface, RevisionIdNegotiationInterface {
   

   VersionByRel

4. +++ b/src/Plugin/RevisionIdNegotiation/RelNegotiation.php
   @@ -0,0 +1,76 @@
   +  protected function getRevisionId(EntityInterface $entity, $input_data) {
   ...
   +      case static::CURRENT:
   ...
   +      case static::LATEST:
   

   This looks quite nice!

5. +++ b/src/Plugin/RevisionIdNegotiation/RelNegotiation.php
   @@ -0,0 +1,76 @@
   +    if (!($entity instanceof RevisionableInterface)) {
   

   This is insufficient. This needs to call \Drupal\Core\Entity\EntityTypeInterface::isRevisionable() instead. See the docs for \Drupal\Core\Entity\RevisionableInterface.

6. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -176,6 +178,20 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +    return !empty($interfaces[RevisionableStorageInterface::class]) && $entity_type->isRevisionable();
   

   That call to isRevisionable() is essential, glad to see it here!

   (And yes, that's another Entity API WTF, but alas, nothing we can fix… 🙃)

7. +++ b/src/Revisions/Annotation/RevisionIdNegotiation.php
   @@ -0,0 +1,27 @@
   +class RevisionIdNegotiation extends Plugin {
   

   Either VersionNegotiation or RevisionNegotiation.

   Right now, this seems to be related to the IdNegotiation class. Very confusing.

8. +++ b/src/Revisions/PluginNegotiationBase.php
   @@ -0,0 +1,93 @@
   +  public function getRevision(EntityInterface $entity, $input_data) {
   +    return $this
   +      ->entityTypeManager
   +      ->getStorage($entity->getEntityTypeId())
   +      ->loadRevision($this->getRevisionId($entity, $input_data));
   +  }
   +
   +  /**
   +   * Gets the revision id.
   +   *
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity.
   +   * @param string $input_data
   +   *   A value used to derive a revision id for the given entity.
   +   *
   +   * @return int
   +   *   The revision id.
   +   *
   +   * @throws \InvalidArgumentException
   +   *   When the revision ID cannot be negotiated.
   +   */
   +  abstract protected function getRevisionId(EntityInterface $entity, $input_data);
   

   Why not make getRevisionId() the plugin interface/API? Then we don't need this logic at all. Then this base class can disappear!

   If there's a reason to have this logic here, I'd love to learn more :)

9. +++ b/src/Revisions/ResourceVersionRouteEnhancer.php
   @@ -0,0 +1,121 @@
   +      || !isset($defaults[Routes::RESOURCE_TYPE_KEY])
   +      // This is a collection request, there is nothing to enhance.
   +      || !isset($defaults[$defaults[Routes::RESOURCE_TYPE_KEY]->getEntityTypeId()])
   ...
   +    /* @var \Drupal\jsonapi\ResourceType\ResourceType $resource_type */
   +    $resource_type = $defaults[Routes::RESOURCE_TYPE_KEY];
   

   Use \Drupal\jsonapi\Routing\Routes::getResourceTypeNameFromParameters() instead.

10. +++ b/src/Revisions/ResourceVersionRouteEnhancer.php
    @@ -0,0 +1,121 @@
    +    if (is_null($resolved_revision)) {
    +      $cacheability = (new CacheableMetadata())
    +        ->addCacheContexts(['url.query_args:' . JsonApiSpec::VERSION_QUERY_PARAMETER])
    +        ->addCacheableDependency($entity);
    +      throw new CacheableNotFoundHttpException(
    +        $cacheability,
    +        sprintf(
    +          'The requested resource version, identified by `%s`, could not be found.',
    +          $resource_version_identifier
    +        )
    +      );
    +    }
    

    👏

11. +++ b/src/Revisions/RevisionIdNegotiationManager.php
    @@ -0,0 +1,68 @@
    +    $this->alterInfo('revision_id_info');
    

    This is probably a force of habit, so probably this is here unintentionally.

    I'd really like this to not have an alter hook. To not create API surface.

12. +++ b/src/Revisions/RevisionIdNegotiationManager.php
    @@ -0,0 +1,68 @@
    +  public static function throwBadRequestHttpException($resource_version_identifier) {
    

    I … don't like this, but let's wait and see if it disappears automatically as this patch gets improved :)

Two general questions for now:

1. Mutation support?
2. Ability to list all revisions? version-history

In the case of collections, we need to perform an entity query, and hence in that scenario, the param enhancer does not help us at all.

I think that may be OK since in collections we can only support a very limited array of negotiation options. So it makes sense for it to be its own thing. But I don't feel attached to any solution.

Addressing feedback from #21

1. That was semi-intentional. Throwing the terms at the wall to see what sticks. I think I like "versionable" a bit better. You?
2. 👌🏽
3. 👌🏽
4. 🙂
5. Ah, yest. We're are doing it in the isVersionable logic but missed it here. Good catch.
6. Yup.
7. I'm going with VersionNegotiation.
8. We had that prior to #14. Since we will need to load the revision we can either do it in the plugin or as the immediate statement after getting the revision ID. If we do it here we simplify the calling code since we don't have to inject entity type manager everywhere we inject the negotiator plugin manager.
9. 👌🏽
10. Fast errors FTW.
11. Agreed. Let's remove it.
12. I found myself repeating this code in several places and I wanted to abstract into a static method. Do you have suggestions on where to put this? Maybe rename to throwBadRevisionInput?

Mutation support?

I think that GET is a good place to start. My gut feeling is that we'll be able to use the same qs negotiation for mutations. I don't see an obvious problem, but I haven't thought about it much.

Ability to list all revisions?

For now all the version discovery happens via hypermedia links. However this version-history implies the creation of a new endpoint. I don't think we are ready to tackle that. Maybe a follow up? In any case I think this could follow the same structure as our entry point.

However this version-history implies the creation of a new endpoint.

To me, it feels like a collection-like endpoint, that does not return resource objects, but resource identifier objects.

Having written that, it seems like version-history ought to be a virtual relationship on any versionable resource type? What do you think?

_{P.S.: d'oh, I wanted to remove my points 2 and 3 because I wasn't entirely sure about them. But I forgot.}

I just noticed that when using IdNegotiation, you're able to request the revision of another entity that the one your pointing at, because there's no validation that the passed revision id is indeed part of the requested entity revision history.
I'll try to write a test case which exhibits this behavior.

I added initial test coverage for IdNegotiation & RelNegotiation and also added a fix to prevent the use of a revision id of another entity.

Forgot to make my test base class abstract.
Also fixed some code styling issues reported by the bot.

you're able to request the revision of another entity that the one your pointing at, because there's no validation that the passed revision id is indeed part of the requested entity revision history.

Woah, great find! 🤯 And one with security implications too … Thanks for adding test coverage! Will review in more detail tomorrow.

Woah, great find! 🤯 And one with security implications too

I agree that this fixes some DX, but I don't see the security implications? While serial IDs are frowned upon because they allow for discoverability of unlisted content, that can hardly be considered insecure (otherwise REST core would be in trouble).

@garphy thanks for the contribution!

In case anyone else is curious, this is the interdiff for the patch above.

Fixes some DCS.

This addresses @Wim Leers' feedback in #21. This is mostly renaming stuff. Let's settle with these new names now.

Some renaming woes. I'm expecting more.

#31: If entity access checks happen first and then you end up loading a revision … but it's of a different entity, then you would've bypassed entity access!

#31: If entity access checks happen first and then you end up loading a revision … but it's of a different entity, then you would've bypassed entity access!

I also wondered about security implications of this "bug" but I don't think that the entity access would be bypassed because all of this happen in a route enhancer, thus the wrongly selected entity would be subsequently checked by JSON API request controller afterwards (unless I'm mistaken here).

Everything here rely upon $entity->access() which is called in EntityResource::getIndividual(), after the revision id is resolved.

Having written that, it seems like version-history ought to be a virtual relationship on any versionable resource type? What do you think?

I disagree. For now, I would like to keep version navigation as a hypermedia-only thing and not try to confuse it with resource fields. That gives us a lot of flexibility and makes it easier to write a profile extension for.

To me, it feels like a collection-like endpoint, that does not return resource objects, but resource identifier objects.

That would be a "relationship-like" route. Again, I have to disagree. I do believe this should be a collection of fully fledged resource objects, w/ support for sparse field sets (and perhaps includes after a v1). This would be far more useful for a revision history type view in a client and diff illustrations.

If you're thinking of version-history as a relationship field, I understand why resource identifiers would feel natural. However, I think making it a relationship field gets us into a conceptually weird recursive self-referential mess and would behave very bizarrely with includes (even though it's "virtual").

A relationship-like endpoint would need to consider how to deal with a PATCH and POST (it would be 404 of course, but considering what it would mean illustrates why it doesn't "fit").

My 3 initial questions in #5 are still open:

1. In Drupal terms, what is a working copy?
2. Simarlarly, what is a latest revision?
   1. Without content_moderation installed and using the definitions given in RFC 5829, I think working-copy and latest-version should actually be the same thing.
3. What should the self link be for a resource? Should it include the revision ID negotiator? Perhaps only when it is not the default revision.

I'd like to descope version-history for now. It's unclear atm and I believe that it will require a new endpoint. I'll open up a new issue to discuss it. Please post your thoughts there.

In Drupal terms, what is a working copy?

The working copy is the default revision.

Without content_moderation installed and using the definitions given in RFC 5829, I think working-copy and latest-version should actually be the same thing.

Agreed. This is the current behavior.

What should the self link be for a resource? Should it include the revision ID negotiator? Perhaps only when it is not the default revision.

It should reflect the currently loaded resource, hence it should it include the revision ID negotiator. I concur that we should not add the revision dimension if it's the default.

This is not currently the case, so I'll follow up.

version-history

I disagree. For now, I would like to keep version navigation as a hypermedia-only thing and not try to confuse it with resource fields. That gives us a lot of flexibility and makes it easier to write a profile extension for.

I … am not sure how that would work, given that revisions aren't linear: they're a graph. A graph that can't be reconstructed even. You can't even do previous and next links.
And yes, you're right in That would be a "relationship-like" route. :) You convince me that this is a bad idea when you say that I think making it a relationship field gets us into a conceptually weird recursive self-referential mess and would behave very bizarrely with includes (even though it's "virtual").

I'd like to descope version-history for now. It's unclear atm and I believe that it will require a new endpoint. I'll open up a new issue to discuss it. Please post your thoughts there.

It seems you'd both like to descope it, but shouldn't we first have an idea of how to achieve that while achieving the other things, to ensure we don't make it impossible to fit in later?

My concern is that we make it possible to load a specific revision by ID, but how can you even figure out what that revision ID would be without having some listing of available revisions, which version-history would provide?

I'd really appreciate it if either or both of you could share slightly more of your thoughts about how you think this ought to work. That would probably address my concerns about descoping that to a separate issue ☺️🙏

The #5 questions

Asked at the bottom of #40, answered in #41: that all sounds great! 🤘

revisions aren't linear: they're a graph

How so ? I thought it was pretty linear. AFAIK, we don't store the "parent" revision of a given revision so we can't really build that graph. Unless I'm mistaken, revisions are only list.

revisions aren't linear: they're a graph.

This isn't the first time you've said that, like @garphy, I'm a little confused though. I don't know that to be true, even with content_moderation installed. Could you explain how they're a DAG? AFAICT, revisions form a linear history. Certain revisions are marked "default" and with CM installed, the latest version can be in a different state. IOW, there's no concept of merging or forking.

You can't even do previous and next links. ... My concern is that we make it possible to load a specific revision by ID, but how can you even figure out what that revision ID would be without having some listing of available revisions, which version-history would provide?

This comes back to the linear history thing... if it is a linear history, then it's the self, predecessor-version and successor-version links that would reveal the revision ID specific links. Also, self links alone need them.

Regardless, even a DAG would have predecessor-version and successor-version links (plural). RFC5829§3.5 even takes this into account specifically: "Some systems may allow more than one of these link relations in order to support branching."

It seems you'd both like to descope it, but shouldn't we first have an idea of how to achieve that while achieving the other things, to ensure we don't make it impossible to fit in later?

I didn't say that ;) But I agree with Mateu that it doesn't need to be in the patch, but I agree with you that we need to understand how it will work whether or not we implement it. I don't see the point of moving the discussion to a different issue though.

The working copy is the default revision.

I disagree. It's easy to confuse when latest-version and working-copy are the same thing though. So what's my reasoning?

A "working copy" is a resource at a server-defined URL that can be used to create a new version of a versioned resource."

New revisions always proceed from the tip of the version history. So, I think the "working-copy" is the revision with the highest revision ID.

OTOH, "the latest version is defined by the system." Which makes me think that latest-version should be the default version.

Here is how I think about it:

a---b---c---d---e---f---g---h

d is the default version
h is the version with the highest revision ID
h may be published or unpublished
with CM installed, h may be in one of many workflow states
with CM installed any rev a-h may be the default version

latest-version: d
working-copy: h
working-copy-of: g
predecessor-version:latest-version: c
successor-version:latest-version: e
predecessor-version:working-copy: g
successor-version:latest-version: 404

Something subtle that I suggested in #2795279-29: [PP-2] [META] Revisions support that I don't want to lose is the idea of "chained" rel negotiators. I gave the example of /jsonapi/node/article?resource_version=rel:predecessor-version:working-copy. You can see how those would work and be different in my example above. Perhaps we need a special delimiter for chained rels.

I don't want to lose that ^ but it doesn't need to be implemented here. We just shouldn't do anything to preclude it.

X-posting this from IRC to the issue. This is a refinement of #44.

"working history":   __b___c__   __e___f___g___h
                    /         \ /
"version history": a           d

a->b is a "check out" in the spec
c->d is a "check in" in the spec
d->e is a "check out" in the spec

a->wasDefault()            === TRUE
d->isDefault()             === TRUE
a->isDefault()             === FALSE
(b,c)->wasDefault()        === FALSE
(e,f,g,h)->wasDefault()    === FALSE
h->isLatestVersion()       === TRUE
(all except h)->isLatestVersion() === FALSE

a === predecessor-version of (b,c,d,e,f,g,h)
d === latest-version of (all, including d)
d === successor-version of (a,b,c)
h === working-copy of (all, this is the only working-copy)
h === working-copy-of of (g)
g === working-copy-of of (f)
b === working-copy-of of (c)

The only missing link relation type is then successor-copy for forward navigation in a "working history" as there is successor-version for the "version history". I don't think that's terribly important though and we can live without it.

How so ? I thought it was pretty linear. AFAIK, we don't store the "parent" revision of a given revision so we can't really build that graph. Unless I'm mistaken, revisions are only list.

Correct, we don't store a reference to the "parent" revision.

This isn't the first time you've said that, like @garphy, I'm a little confused though. I don't know that to be true, even with content_moderation installed. Could you explain how they're a DAG? AFAICT, revisions form a linear history. Certain revisions are marked "default" and with CM installed, the latest version can be in a different state. IOW, there's no concept of merging or forking.

Time to dig in and figure out if I'm misremembering correctly. It definitely sounds like that's the case.

I think I may be confusing what's currently implemented with what the Workflow Initiative plans to do. See #2786133: WI: Phase B: Extend the revision API with support for parents and specifically #2725523: Add a revision_parent field to revisionable entities. "nonlinear revisions" is explicitly mentioned in #2721129: Workflow Initiative for phase B. So yeah … I'm betting that's why I was convinced that revisions were a DAG, sorry!

I found beautiful confirmations of intended linearness in the UI improvement proposals at #1776796-94: Provide a better UX for creating, editing & managing draft revisions., even though that's 6 years old :P

So, yes, you guys are right, revisions are linear. But only in the sense that each revision is created at a certain time. Because if you restore an older revision, an older revisions suddenly reappears, even though it is not actually a successor to the revision that precedes it timewise. So our revisions are "linear but not really". Linear timewise, not linear contentwise. Of course, we lack the necessary metadata to track revision history *contentwise*. That's exactly what the Workflow Initiative aims to fix.

In doing that research, I also identified the following issues as impacting this feature at some point:

1. #1812202-146: Add UUID support for entity revisions
2. #2727511: WI: Add revision hash base field to all revisionable entities
3. #2786133: WI: Phase B: Extend the revision API with support for parents

… since all of that is planned revision-related work in Drupal core

From what I see in the patch in #46 that @justageek merged #2992833: Add a version negotiation to revisionable resource types in here. I'll try to separate the two again.

This fixes the tests from #36. More updates to come.

Something subtle that I suggested in #2795279-29: [META] Revisions support that I don't want to lose is the idea of "chained" rel negotiators. I gave the example of /jsonapi/node/article?resource_version=rel:predecessor-version:working-copy. You can see how those would work and be different in my example above. Perhaps we need a special delimiter for chained rels.

I'm not ignoring this. I'm just putting that thought on pause. I think we may need to define that feature a bit better. For instance you may want to request:

pipe(
  'rel:predecessor-version',
  'rel:working-copy-of',
  'id:17'
)

Note the cross-negotiators interaction. While it is unlikely that this level of flexibility will be useful and pragmatic, I think that with the current implementation (modular and pluggable) makes it "easy" to implement.

This patch changes the nomenclature according to #47. It also adds support for working-copy-of and predecessor-version but throws a cacheable 501 (Not Implemented) for now. We should address that in a follow up.

I still think that default revision == rel:latest-version and latest revision == rel:working-copy will be a huge source of confusion. However, at this point I'm willing to make this concession to bring this patch closer to merge.

RFC5829§3.5 even takes this into account specifically: "Some systems may allow more than one of these link relations in order to support branching."

Woah, fascinating! 👌

I agree with Mateu that it doesn't need to be in the patch, but I agree with you that we need to understand how it will work whether or not we implement it.

That's fine by me. Is @e0ipso also +1?

The working copy is the default revision.

I disagree. It's easy to confuse when latest-version and working-copy are the same thing though. So what's my reasoning?

A "working copy" is a resource at a server-defined URL that can be used to create a new version of a versioned resource."

New revisions always proceed from the tip of the version history. So, I think the "working-copy" is the revision with the highest revision ID.

OTOH, "the latest version is defined by the system." Which makes me think that latest-version should be the default version.

Wow, this is very very confusing!

It doesn't help that https://tools.ietf.org/html/rfc5829#section-3.2 says When included on a versioned resource, this link points to a resource containing the latest (e.g., current) version., while Drupal has both QueryInterface::currentRevision() and QueryInterface::latestRevision() and the docs for the latter say The latest revision is the most recent revision of an entity. This will be either the default revision, or a pending revision if one exists and it is newer than the default..
This means that also in the case of Drupal, just like in the case of RFC5829, latest is considered a level of indirection whose behavior follows certain rules (and could be customized).

As far as I can tell, that last emphasized statement (latest is default, or pending if newer than default) maps 1:1 to concepts in RFC5829. Especially if we factor in the concept of working-copy, which is something that cannot exist in "plain" Drupal, without Content Moderation. For working-copy to make sense, you at least need to distinguish between "draft" vs "live", which you cannot do in Drupal without Content Moderation.

With Content Moderation

*If* there is a pending revision, then working-copy == latest-revision. If there is *no* pending revision, then working-copy == canonical.
Drupal has no direct equivalent for working-copy-of. This can only be reliably implemented once #2725523: Add a revision_parent field to revisionable entities is done. Because when you restore a prior revision in Drupal, it bears no relation at all to the revision directly preceding it in time or revision ID. Note that once this exists, predecessor-version and successor-version technically also need to change their behavior. Which is why I would err on the side of caution and omit those too _{(This is why I was saying #42 that you can't even reliably do previous/next links.)}

Drupal concept	RFC5829 concept
Default/current revision	canonical
Pending revision	working-copy
Latest revision	latest-version
∅	working-copy-of
Previous revision by ID, if it exists	predecessor-version
Next revision by ID, if it exists	successor-version

Without Content Moderation

Drupal concept	RFC5829 concept
Default/current revision	canonical and latest-version
∅	working-copy
∅	working-copy-of
Previous revision by ID, if it exists	predecessor-version
Next revision by ID, if it exists	successor-version

The one thing that's in my proposal that hasn't been mentioned before on this issue, is the notion of using canonical to link to the default/current/live version.

I also think it's essential that latest-version always points to the actual latest, no matter if it's pending/draft (non-default) or default (current/live).

Looking forward to your feedback :)

That's a very different interpretation from what @gabesullice suggests. I honestly don't care what we end up deciding, although I have a slight preference. I only care about the feature being there. In other words, whatever interpretation we go with we should be able to load the revision we want with relations that point to current revision and latest revision.

However, I would love to get everyone on the same page relatively quickly if that is possible.

For the sake of discussion let's keep the 2 scenarios in #53 (with and without content moderation). And let's have in mind the revision history in #47.

Also, when we refer to revisions in Drupal lets talk about:

First revision

(regardless of moderation state). In our revision history: a

First default revision

(the first revision by revision ID that returns true to wasDefaultRevision). In our revision history: a

Previous revision (needs a revision anchor as reference)

(the previous revision when sorting by revision ID). In our revision history: Previous revision of f = e

Previous default revision (needs a revision anchor as reference)

(the previous revision when sorting by revision ID that returns true to wasDefaultRevision). In our revision history: Previous default revision of f = d

Next revision (needs a revision anchor as reference)

Analogous to previous revision

Next default revision (needs a revision anchor as reference)

Analogous to previous default revision

Current default revision

(returns true to isDefaultRevision). In our revision history: d

Last revision

(regardless of moderation state). In our revision history: h

Last default revision

(same as current default revision)

Maybe we can have a table of the relation types in RFC5829 according to both interpretations.

I don't think it's very different. I think there is a significant risk of future BC breaks if we go with #44/#47.

Thanks for outlining the definitions. That's in line with what #47 says. But it results in two version histories. How do we plan to have two version-history exposed? The definition titles in #54 clearly show that there are two parallel version histories: there's "first/previous/next/last revision" and "first/previous/next/last DEFAULT revision", plus one special case: current default revision, which is effectively the live revision.

But RFC5829 clearly says:

A "version history" resource is a resource that contains all the versions of a particular versioned resource.

IOW: a single history.

I hope you're seeing that I'm concerned about going in a direction that will prevent us from exposing certain things in the future, because we go with a less-than-ideal mapping of RFC5829 concepts onto Drupal's implementation, which then means we can't improve that mapping in the future, which means we'll have to forever support this pattern.
I don't even have a strong opinion about any of this! I'm merely concerned.

Maybe we can have a table of the relation types in RFC5829 according to both interpretations.

I wanted to start doing this, but there are a few obstacles:

1. #54 doesn't list pending revision, which is any non-default revision newer than the current default revision. This is a concept added by the Content Moderation module, but expressed using metadata present even without it installed. A consequence of this is that in #47's example, not only h is a working-copy … but for d, probably e, f, g and h are all working copies! (The spec literally says there can be multiple working copies.) I say "probably", because actually we don't know if e is a working copy started from d — it could also have been started from a, b or c. This is why we need #2725523: Add a revision_parent field to revisionable entities for working-copy-of.
2. #47 and #54 make the IMHO dangerous assumption that each revision is based on the one preceding it from a time or ID perspective. But that is not at all guaranteed. Again, we don't know which revision e was started from. Which is why I don't think we can implement predecessor-version or successor-version. I would be okay with previous and next, because e.g. previous Refers to the previous resource in an ordered series of resources. Synonym for "prev".. We do have a single ordered list of all revisions, regardless of the actual predecessor version of each revision: based on revision ID we have an ordered listing, and hence previous and next are fine.

So, let me refine what I wrote in #53 (now there is no distinction between with/without Content Moderation anymore), and apply it to the diagram from #47:

"working history":   __b___c__   __e___f___g___h
                    /         \ /
"version history": a           d

P.S.: I left a comment at #2725523-10: Add a revision_parent field to revisionable entities to indicate that that is probably a blocker to this.

Per #2725523-13: Add a revision_parent field to revisionable entities, we actually do know the strategy that will be used. Which means we can do working-copy, working-copy-of, predecessor-version and successor-version today!

That means that previous == predecessor-version and next == successor-version, until #2725523: Add a revision_parent field to revisionable entities lands. Once it lands, they'll start to diverge for any revision created after the site updates to a Drupal core version that contains #2725523, which means there will NOT be a BC break.

I'm not ignoring this. I'm just putting that thought on pause. I think we may need to define that feature a bit better.

👍We're on the same page :)

I still think that default revision == rel:latest-version and latest revision == rel:working-copy will be a huge source of confusion.

I don't disagree with that. We'll need to communicate this well and consider the DX. A target attribute could help. I'm consoled by the fact that I think it's simple to make the point that rel:latest-version gets the latest "live" revision and rel:working-copy gets the latest draft revision. "draft" sounds a lot like "working-copy".

Wow, this is very very confusing!

I completely agree.

I think that a distinction that I made implicitly but should have called out is that I treat the word "version" to be distinct from and not synonymous with "revision". A "version" in the RFC language is a "revision" that was/is a default.

Especially if we factor in the concept of working-copy, which is something that cannot exist in "plain" Drupal, without Content Moderation. For working-copy to make sense, you at least need to distinguish between "draft" vs "live", which you cannot do in Drupal without Content Moderation.

All my examples presume the "draft" vs "live" distinction. Let's not forget that Content Moderation is stable in core.

I'd like to float the idea that JSON API revision support should require that CM be enabled. Or, at the very least, that without CM enabled we only provide a very spartan implementation, e.g. only having the id negotiator and a version-history collection, specifically excluding the X-version and working-copy links and the rel negotiator from use. I see very little practical benefit to this feature without CM installed. IOW, I think providing a content preview mechanism is far more valuable than a mechanism for displaying a revision log. And preview is only possible with CM enabled.

@e0ipso I think you'd agree with the relative priority there? ^

*If* there is a pending revision, then working-copy == latest-revision. If there is *no* pending revision, then working-copy == canonical.

I think this interpretation goes back to my assumption that version != revision. I think latest-version = isDefaultRevision() = TRUE and working-copy = isPendingRevision() = TRUE.

Why do I think version != revision? Let me pull some quotes:

A "checkin" is an operation on a working copy that creates a new version of its corresponding versioned resource.

When a versioned resource is checked out and then subsequently checked in, the version that was checked out becomes a "predecessor" of the version created by the checkin.

A "checkout" ... creates a working copy, or changes the versioned resource to be a working copy as well

A "checkin" is an operation on a working copy that creates a new version of its corresponding versioned resource.

Each of these examples seem to indicate that a "working copy" is not a version. Then what is it? It must be a draft. With CM, we have drafts. Drafts are pending revisions. All versions are revisions, but not all revisions are versions.

To call it out even more strongly: "A "checkin" is an operation on a working copy that creates a new version of its corresponding versioned resource."

"[A working copy's] corresponding versioned resource" really strongly indicates that a working copy is a derivative of a version and not a version itself.

If the latest revision also happens to be a default revision, then latest-version == canonical

I think it's highly informative that RFC 5829 makes no reference to canonical. To me, the need for the canonical link to identify d makes me a little skeptical of that interpretation. If it were necessary part, it would have been mentioned at least in contrast.

With my interpretation, we can identify every significant revision with only the link relation types mentioned. The ones we struggle with are prior revisions that were not default (b, c, e, f & g). These are the least significant in the system and would be identifiable with a parent-revision link relation type. I.e., we'd still need to have an link relation type outside the scope of the spec, but it's still very useful without it.

To summarize:

All revisions are not versions. If not, then which revisions are versions? Those that were/are default. What then is a revision that was/is not default? It is a revision that is/was a "working copy". What is a working-copy-of? The working copy's "corresponding versioned resource." IOW. the last revision that was a default revision in that revision's ancestry.

What is missing from that summary?

@Wim Leers and my own interpretations have both identified a need for forward and backward revision navigation and additional link relation types. I think the problem is that the spec was not written to account for multiple revisions of a draft. To fill that gap, I propose we have our own links then: parent-revision and child-revision in future. This is an additive addition that leaves a very functional implementation using only spec-defined relation types (without canonical #56 is not very functional).

What about version-history? I think it's entirely reasonable to include revisions that are not versioned in the version history. The "versioned" revisions will be identifiable by their links. For example, self will be equal to latest-version or self will be equal to working-copy. I don't think it's necessary to have separate collections for a "default" history and another collection for a different history. It can be a superset of just the official "versioned" revisions that includes "working copy" revisions.

👏🏽 @gabesullice. As I pointed out the other day in IRC what it makes more sense to me is the correspondence of "version" with "default revision" and "working copy" with "in-progress draft revision". I'm glad to see your articulation of it here.

I am ready to buy #57.

Random thoughts:

To put things in the language of the spec, a "check in" operation would be (∅ | Draft) ⇒ Published. A "check out" would be Published ⇒ (Archived | Draft). With Content Moderation we can create a new version by either checking out the version (hence making a working-copy) and then checking it back in (by publishing it). We can also create a new version by updating the content entity keeping it published (this workflow is also explicitly mentioned in the spec).

What about version-history? I think it's entirely reasonable to include revisions that are not versioned in the version history.

While I buy most of #57 I'm not ready for this. For me it brings down all the cohesiveness of the terminology. version-history is a history of versions. If versions are "default revisions" then I don't think it's entirely reasonable to include working copies there. Because working-copies != versions according to #57.

I think it makes sense to have a watered down versioning negotiation & discovery feature if CM is not enabled. However, I think that having our code paths split depending on other modules being enabled will be hard to maintain. If the VersionByRel was provided by Content Moderation (or we mocked that somehow in JSON API), I'd be open to that solution. In any case I'd like to see a very clear separation for the negotiation and linking.

FWIW the patch in #52 is compatible with the proposal in #57.

While I buy most of #57 I'm not ready for this. For me it brings down all the cohesiveness of the terminology. version-history is a history of versions.

I think I can convince you with one question: Is a draft not part of a version's history?

I think it makes sense to have a watered down versioning negotiation & discovery feature if CM is not enabled.

Fine with me.

However, I think that having our code paths split depending on other modules being enabled will be hard to maintain.

Agreed.

If the VersionByRel was provided by Content Moderation (or we mocked that somehow in JSON API), I'd be open to that solution.

I'm open to that too :) Let's see what it looks like. Perhaps we can put it under modules/content_moderation without an info.yml and force the plugin manager to discover it if the real content_moderation is enabled.

In any case I'd like to see a very clear separation for the negotiation and linking.

This x 💯. I was really pleased to see that you even made two separate issues for this :) I honestly can't articulate why this should be, but my spidey-sense tells me that it's important :P

#57:

I think that a distinction that I made implicitly but should have called out is that I treat the word "version" to be distinct from and not synonymous with "revision". A "version" in the RFC language is a "revision" that was/is a default.

We had a call yesterday evening where we discussed our interpretations of RFC5829 and how its concepts map to Drupal. This was indeed a key thing that came up!

I think RFC5829 basically intends that the different working copies, which can be created off one another, do NOT have an accessible version history. They're "ephemeral" as far as the spec is concerned.

Or, at the very least, that without CM enabled we only provide a very spartan implementation, e.g. only […]I see very little practical benefit to this feature without CM installed.

I … am not sure I agree with that. Drupal has not had Content Moderation for >1.5 decades and got by just fine. I got stuck on the next step of what I was going to write. I tried using the revision functionality on a D8 site without Content Moderation, and concluded that this proposal of yours makes perfect sense :)

Each of these examples seem to indicate that a "working copy" is not a version.

Thanks for extracting those specific examples. Consider me convinced that this is indeed the intention of the RFC authors. a working copy is a derivative of a version and not a version itself. — if only the RFC authors had included this in their RFC! 😞 Would have saved us a lot of time!

I think it's highly informative that RFC 5829 makes no reference to canonical.

Fair.

With my interpretation, we can identify every significant revision with only the link relation types mentioned.

I'm still concerned that this means that it's going to be impossible to create a client that talks to JSON API and is able to provide a complete authoring experience, because without access to all revisions (Drupal terminology) or working copies (RFC5829 terminology), it cannot be considered complete.
This is in part why I never even would have considered that perhaps "working copies" are not "versions": because then how could the "version history" be considered complete?!

So that's my key question then: how do you propose we handle that?

To summarize:

All revisions are not versions. If not, then which revisions are versions? Those that were/are default. What then is a revision that was/is not default? It is a revision that is/was a "working copy". What is a working-copy-of? The working copy's "corresponding versioned resource." IOW. the last revision that was a default revision in that revision's ancestry.

👍 I think this correctly interprets the spec. My one concern still stands though.

EDIT: Perhaps I do still see one bit of ambiguity: does d have a working-copy relation to c or to e? I could see it argued either way… because the question is whether it means "this version originates from this working copy" or "there is a working copy initiated from this version"?)

parent-revision and child-revision

Did you mean version instead of revision?

#58:

While I buy most of #57 I'm not ready for this. For me it brings down all the cohesiveness of the terminology. version-history is a history of versions. If versions are "default revisions" then I don't think it's entirely reasonable to include working copies there. Because working-copies != versions according to #57.

Exactly this! ➕➕➕

I think it makes sense to have a watered down versioning negotiation & discovery feature if CM is not enabled. However, I think that having our code paths split depending on other modules being enabled will be hard to maintain.

Agreed with keeping "maintainability" very high in the list of priorities. I'm not yet sure that it'll necessarily be very complex though: to know that for certain, I'd need to see a patch that does it.

#59:

I think I can convince you with one question: Is a draft not part of a version's history?

But … version-history is intended to list all versions of a resource. And the spec defines "versions" to be different from "working copies". So to then include working copies in the version history seems contradictory.

AFAICT there are two remaining question:

1. How can one access the history of working copies (draft revisions)? @gabesullice proposed to list them in version-history but @e0ipso and I feel that this is contradictory/undermines cohesiveness.
2. Does d have a working-copy relation to c or to e? I could see it argued either way… because the question is whether it means "this version originates from this working copy" or "there is a working copy initiated from this version"?)

To really grok this, I think we need to understand the RFC authors' point of view better. Quoting RFC5829:

   Checkin

      A "checkin" is an operation on a working copy that creates a new
      version of its corresponding versioned resource.

      Note: the operations for putting a resource under version control
      and for checking in and checking out depend on the protocol in use
      and are beyond the scope of this document; see [CMIS], [RFC3253],
      and [JSR-283] for examples.

RFC3253 is WebDAV.

JSR-283 is the spec for the Java Content Repository, which contains https://docs.adobe.com/content/docs/en/spec/jcr/2.0/3_Repository_Model.h.... I'm 99% certain that RFC5829 was written not with multiple systems in mind, but only JCR. That is … a faux pas. But here we are. All terminology in RFC5829 is identical in here: check in, check out, predecessor, successor, version history — but it's only a subset: "working copy" is missing.

CMIS is the "Content Management Interoperability Services (CMIS)" 1.0 spec: https://docs.oasis-open.org/cmis/CMIS/v1.0/cmis-spec-v1.0.html — it contains a different subset more of the terminology: check in, check out, version history, working copy (no "predecessor" or "successor" this time). This spec dates back to 2010, was updated in 2011 with errata. And in 2015, a 1.1 version was released: https://docs.oasis-open.org/cmis/CMIS/v1.1/CMIS-v1.1.html.
Of specific interest to me is how they define "working copy" (given my second remaining question in #60). https://docs.oasis-open.org/cmis/CMIS/v1.0/cs01/cmis-spec-v1.0.html#_Toc... explains their "Version Series" concept:

2.1.9.1 Version Series

A version series for a Document object is a transitively closed collection of all Document objects that have been created from an original Document in the Repository. Each version series has a unique, system-assigned, and immutable version series ID.
The version series has transitive closure -- that is, if object B is a version of object A, and object C is a version of object B, then object C is also a version of object A.

Interestingly, https://docs.oasis-open.org/cmis/CMIS/v1.0/cs01/cmis-spec-v1.0.html#_Toc... says:

2.1.9.4.1 Checkout

A new version of a versionable Document object is created when the checkIn service is invoked on the Private Working copy (PWC) of this object
[…]
After a successful checkout operation is completed, and […], the effects on other Documents in the Version Series MUST be as follows:
- The repository MUST throw an exception if the checkout service is invoked on any Document in the Version Series. (I.e. there can only be one PWC for a version series at a time.)
- The value of the cmis:isVersionSeriesCheckedOut property MUST be TRUE.

[…]

2.1.9.4.4 Checkin

[…]

The effects of the checkIn service MUST be as follows for successful checkins:
- […]
- For all Documents in the Version Series:
o The value of the cmis:isVersionSeriesCheckedOut property MUST be FALSE.
o […]

This literally says there can only ever be ONE working copy per document/resource/entity! 😂

6.  Acknowledgments

   Thanks to the members of Content Management Interoperability Services
   (CMIS) Technical Committee (TC) at OASIS for the initial proposal,
   and to Jan Algermissen for feedback during IETF review.

Thanks, CMIS… 😭 … for imposing your limited world view on HTTP link relation RFCs that aim to be generic.

I dug into the CMIS and JSR-283 specs to figure out an answer to #60.2. In doing so, I essentially managed to answer both questions.

Everything indicates that RFC5829 was proposed by CMIS, and to achieve a semblance of broad market support, they also involved JCR authors. A quick search leads to https://en.wikipedia.org/wiki/Content_Management_Interoperability_Servic..., which points to for example its document-centricness. Which is a similar problem as page-centric CMSes. Seen in that light, it's understandable that they chose to KISS and have only a single working copy.

In conclusion, working-copy in RFC5829 == "private working copy" in CMIS1.0 == a single forward/pending revision in Drupal that all authors must edit at the same time. Except that in RFC5829, multiple of these are allowed, yet they're not allowed to show up in the version history. Seemingly because neither CMIS nor RFC5829 consider it important that working copies (drafts) are listable/navigatable, because they're ephemeral.

Based on #61, I'd say it is impossible to perfectly map RFC5829 to Drupal's capabilities. We just have to make an informed choice.

Whoa! Thank you for going so deep! That's incredibly useful.

Note: I responded to different parts of your comments as I read them, but I think it's important to read my replies below all as a whole because they all inform one another.

EDIT: Perhaps I do still see one bit of ambiguity: does d have a working-copy relation to c or to e?

Neither?

I think d should have a link of type working-copy to h.

h should have a link of type working-copy-of to d.

Quoting myself: "I think the problem is that the spec was not written to account for multiple revisions of a draft." Which is why there should not be a working-copy link from d to e, because e is not the latest draft, h is.

Why do I know that d has a link of type working-copy to h and not of type working-copy-of? Because sections 3.3 and 3.4 has "when included on a versioned resource" for working-copy and "when included on a working copy" for working-copy-of. d is our "versioned resource" so it must have the working-copy link pointing to h.

Did you mean version instead of revision?

No. Very much because of what I just said above. Since d has a link to h. There would be no way to navigate to e, f or g. parent-revision and child-revision fill in the gaps.

version-history is intended to list all versions of a resource.... So to then include working copies in the version history seems contradictory.

Not quite. It says it's "a resource which contains all versions of a versioned resource".

I think it's okay if it contains all versions, but also contains working copies.

Is that a little clever? Yes. Is it in violation of the spec? No.

Are drafts or working copies a part of a version's history? Yes.

Is it less than perfect? Yes. Do we have another proposal that would be more intuitive? No.

Do I believe one exists? Not really, I think that's the best we can do.

Could someone come up with a more intuitive approach? If anyone could, it would be @e0ipso or @Wim Leers ;)

This literally says there can only ever be ONE working copy per document/resource/entity!

That makes sense. It confirms my earlier intuition: "I think the problem is that the spec was not written to account for multiple revisions of a draft."

Based on #61, I'd say it is impossible to perfectly map RFC5829 to Drupal's capabilities. We just have to make an informed choice.

I completely agree with this.

"To fill that gap [between RFC5829 and Drupal's capabilities], I propose we have our own [link relation types]: parent-revision and child-revision in future. This is an additive addition that leaves a very functional implementation using only spec-defined relation types."

If working-copy points from version d to the most recent pending revision h, that's the most useful interpretation.

Why? If you're viewing the default version d wouldn't you want an "edit" button to start from h? And from the edit page, wouldn't you want the "cancel" button to send you back to d?

IOW, since working-copy must point to "ONE working copy," it should point to the single most logical revision, the last one. It should skip previous revisions of that draft because they've been superseded (i.e. e, f & g are superseded by h).

That approach ensures that a spec-only implementation is still quite useful. It can be "enriched" to match Drupal's capability of "a history for working copies" by adding my proposed link relation types, parent-revision and child-revision to access e, f, & g from d or h.

I love the symmetry of our conclusions:

#60

AFAICT there are two remaining question:

1. How can one access the history of working copies (draft revisions)? @gabesullice proposed to list them in version-history but @e0ipso and I feel that this is contradictory/undermines cohesiveness.
2. Does d have a working-copy relation to c or to e? I could see it argued either way… because the question is whether it means "this version originates from this working copy" or "there is a working copy initiated from this version"?)

#57

What is missing from that summary?

@Wim Leers and my own interpretations have both identified a need for forward and backward revision navigation and additional link relation types. I think the problem is that the spec was not written to account for multiple revisions of a draft. To fill that gap, I propose we have our own links then: parent-revision and child-revision in future. This is an additive addition that leaves a very functional implementation using only spec-defined relation types (without canonical #56 is not very functional).

What about version-history? I think it's entirely reasonable to include revisions that are not versioned in the version history. The "versioned" revisions will be identifiable by their links. For example, self will be equal to latest-version or self will be equal to working-copy. I don't think it's necessary to have separate collections for a "default" history and another collection for a different history. It can be a superset of just the official "versioned" revisions that includes "working copy" revisions.

What that tells me is that we're on the right track :)

Hopefully #63 satisfactorily addressed #1 for you. While I'm still hopeful you can come up with something better for #2, I'm willing to settle for my own proposal ofc :P

Fair warning: I'm still reading through the last updates.

Does d have a working-copy relation to c or to e? I could see it argued either way… because the question is whether it means "this version originates from this working copy" or "there is a working copy initiated from this version"?)

I read the link relations as: The ${link-rel-type-name} link of the current resource is ${link-value}

Examples:

• The canonical link of the current resource is https://…/1234
• The latest-version link of the current resource is https://…/1234
• …

working-copy reads well with that. working-copy-of not so much, but I think that the RFC definition helps in that regard. However the definition of working-copy of the spec seems to contradict this /sigh.

/me continues reading.

I think parent-revision and child-revision would be pretty confusing, since there's then 3 concepts in link relations alone: "working copies", "versions" and "revisions". Only a Drupalist will be able to understand it.

Is that a little clever? Yes. Is it in violation of the spec? No.

Are drafts or working copies a part of a version's history? Yes.

Is it less than perfect? Yes. Do we have another proposal that would be more intuitive? No.

It's hard to argue with this after having written #62.

Why? If you're viewing the default version d wouldn't you want an "edit" button to start from h? And from the edit page, wouldn't you want the "cancel" button to send you back to d?

IOW, since working-copy must point to "ONE working copy," it should point to the single most logical revision, the last one.

Hard to argue with this too.

So … it's like I have to agree with Gabe's proposal for lack of better alternatives 😲😂 As far as I can see, it's the least contradicting and most usable mapping of RFC5829 to Drupal's entity revision system out of all possible mappings.

Eagerly awaiting @e0ipso's feedback! 🙂 Rooting for him to do better, but not sure that's possible.

#65

I read the link relations as: The ${link-rel-type-name} link of the current resource is ${link-value}

Perhaps we should all just use the "official" way of saying it:

A link can be viewed as a statement of the form "{context IRI} has a {relation type} resource at {target IRI}, which has {target attributes}".

So, your examples would be:

• https://.../one-two-three-four has a canonical resource at https://.../1234
• https://.../v/a has a latest-version resource at https://../v/d

So:

• https://.../v/d has a working-copy resource at https://.../v/h
• https://.../v/h has a working-copy-of resource at https://.../v/d

Also, props for truly using HTTPS everywhere :P

#66

I think parent-revision and

child-revision

would be pretty confusing, since there's then 3 concepts in link relations alone: "working copies", "versions" and "revisions". Only a Drupalist will be able to understand it.

This is true. What about prior-working-copy and subsequent-working-copy?

What about prior-working-copy and subsequent-working-copy?

Works for me!

@Wim Leers, awesome!

FTR, @e0ipso has also voiced his approval of the HTTP API in various conversations.

Which means... I think we've got consensus on the HTTP API!

Now that we have that, here's my initial review of the patch. FWIW, this is actually the first time I've read it.

1. +++ b/src/Controller/EntityResource.php
   @@ -99,14 +109,25 @@ class EntityResource {
   +  public function __construct(
   ...
   +    VersionNegotiationManager $version_negotiation_manager
   
   @@ -347,6 +368,13 @@ class EntityResource {
   +    // If the request is for the latest revision, toggle it on entity query.
   +    $resource_version_identifier = $request->query->get(JsonApiSpec::VERSION_QUERY_PARAMETER);
   +    $wants_latest_version = $this->wantsLatestRevisionOnCollection($resource_version_identifier);
   +    if ($wants_latest_version) {
   +      $query->latestRevision();
   +    }
   
   @@ -397,6 +428,36 @@ class EntityResource {
   +  protected function wantsLatestRevisionOnCollection($resource_version_identifier) {
   
   +++ b/src/Revisions/ResourceVersionRouteEnhancer.php
   @@ -0,0 +1,120 @@
   +      // This is a collection request, there is nothing to enhance.
   +      || !isset($defaults[$resource_type->getEntityTypeId()])
   

   This adds a lot of complexity to EntityResource and requires us to inject yet another dependency into it.

   Instead of parsing the parameter in the controller, let's move this logic into ResourceVersionRouteEnhancer and do $defaults['wants_latest_versions'] = TRUE|FALSE in its enhance method.

   As you can see, we already have the logic in place to identify a collection route inside the enhance method because we're explicitly saying "there's nothing to do!"

2. +++ b/src/Plugin/VersionNegotiation/VersionById.php
   @@ -0,0 +1,38 @@
   + * Defines a revision id implementation for entity revision id values.
   

   s/id/ID/g

3. +++ b/src/Plugin/VersionNegotiation/VersionByRel.php
   @@ -0,0 +1,127 @@
   + * Revision id implementation for the current or latest revisions.
   

   same.

4. +++ b/src/Plugin/VersionNegotiation/VersionByRel.php
   @@ -0,0 +1,127 @@
   +  const HEAD = 'working-copy';
   ...
   +  const CURRENT = 'latest-version';
   

   I think it's confusing that these constants have different names than their values and that those names don't map to any concepts in the HTTP API. IOW, I don't think it aids understanding.

   I'd rather that we have constants matching the terms we've agreed upon above with very verbose commentary on each of them explaining their meanings how each of them relates to Drupal's revisions concepts.

5. +++ b/src/Plugin/VersionNegotiation/VersionByRel.php
   @@ -0,0 +1,127 @@
   +        $this->checkVersionHistorySupport($storage);
   +        $message = sprintf('The link relation type %s is not implemented yet.');
   

   ❤️

6. +++ b/src/Plugin/VersionNegotiation/VersionByRel.php
   @@ -0,0 +1,127 @@
   +   * Checks if the storage supports listing all the revisions of an entity.
   ...
   +  protected function checkVersionHistorySupport(EntityStorageInterface $storage) {
   +    if (!method_exists($storage, 'revisionIds')) {
   

   Whoa! We really don't have a better way than method_exists?! I'm not surprised and not surprised at the same time.

7. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -175,6 +177,22 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +    $is_versionable_storage = is_subclass_of(
   +      $entity_type->getStorageClass(),
   +      RevisionableStorageInterface::class);
   +    return $is_versionable_storage && $entity_type->isRevisionable();
   

   Again, this seems really clunky. Is there no better way?

8. +++ b/src/Revisions/ResourceVersionRouteEnhancer.php
   @@ -0,0 +1,120 @@
   +    try {
   +      /** @var \Drupal\jsonapi\Revisions\VersionNegotiationInterface $negotiator */
   +      $negotiator = $this->revisionNegotiatorManager->createInstance($negotiator_name);
   +      $resolved_revision = $negotiator->getRevision($entity, $input_data);
   +    }
   +    catch (\InvalidArgumentException $exception) {
   +      VersionNegotiationManager::throwBadRequestHttpException($resource_version_identifier);
   +    }
   +    catch (PluginException $exception) {
   +      VersionNegotiationManager::throwBadRequestHttpException($resource_version_identifier);
   +    }
   

   If we keep plugins, let's just make a getRevision($negotiator_name, $entity, $negotiator_value) on the plugin manager itself.

   That eliminates the need to have the clunky createInstance call and public statics for these exceptions.

9. +++ b/src/Revisions/VersionNegotiationManager.php
   @@ -0,0 +1,67 @@
   +/**
   + * Provides an Revision ID negoation plugin manager.
   ...
   + * @internal
   + */
   +class VersionNegotiationManager extends DefaultPluginManager {
   

   s/negoation/negotiation/

   ---

   Even though this is @internal, I still feel very uneasy about having a plugin system for version negotiators.

   It seems like it introduces a lot of boilerplate and infrastructure code when we don't actually have any identified need for the modularity that it provides.

   Getting rid of the plugin manager and making everything more static would reduce the size and scope of this patch quite a bit.

10. +++ b/src/Revisions/VersionNegotiationManager.php
    @@ -0,0 +1,67 @@
    +  /**
    +   * The separator between the negotiator name and the negotiation ID.
    +   *
    +   * @var string
    +   */
    +  const SEPARATOR = ':';
    

    I think this belongs on the ResourceVersionRouteEnhancer since that is what is actually exploding the query parameter.

11. +++ b/tests/src/Kernel/Plugin/VersionNegotiation/VersionByIdTest.php
    @@ -0,0 +1,32 @@
    +class VersionByIdTest extends VersionNegotiationTestBase {
    
    +++ b/tests/src/Kernel/Plugin/VersionNegotiation/VersionByRelTest.php
    @@ -0,0 +1,44 @@
    +class VersionByRelTest extends VersionNegotiationTestBase {
    
    +++ b/tests/src/Kernel/Plugin/VersionNegotiation/VersionNegotiationTestBase.php
    @@ -0,0 +1,115 @@
    +abstract class VersionNegotiationTestBase extends JsonapiKernelTestBase {
    

    This definitely needs some Functional tests.

I've rerolled the patch for the 2.x branch (I have not yet ported changes to the Functional tests).

I have not made any of the changes I asked for in #69.

This moves the version query parameter name constant from JsonApiSpec to ResourceVersionRouteEnhancer because it is not part of the spec.

This addresses #69.1 by moving the logic that determines if the collection should load latest revisions or not to the route enhancer.

The logic was also not consistent with our agreed upon definitions. I updated the logic to match those.

Finally, the pattern used to load the latest version could cause lots of unnecessary db loads when most under certain circumstances. The new logic ensures that no unnecessary db loads are made.

This addresses #69.2

This begins to address #69.9 by making it impossible for 3rd party modules to implement version negotiation plugins. I still haven't seen the argument for plugins over static code in this issue yet, but I'll wait to remove it for now since it would be a somewhat dramatic change of direction.

This addresses #69.4.

This addresses #69.8, making the route enhancer much smaller and easier to read. In doing this, I realized we should probably have a new exception for a "revision not found" so we can issue a 404 instead of a 400 bad request.

More tomorrow.

I still haven't seen the argument for plugins over static code in this issue yet

We've held this conversation in other forums, but not here. I think that it boils down to extensibility. Eventually we want revision negotiators to be extensible. The best approach for now is to have locked down plugins (plugins with unsupported opt-in). When things WRT revisions are stable we remove the lock.

Eventually we want revision negotiators to be extensible. The best approach for now is to have locked down plugins (plugins with unsupported opt-in). When things WRT revisions are stable we remove the lock.

This sounds reasonable.

Any reason not to do this, @gabesullice?

I still haven't seen the argument for plugins over static code in this issue yet

We've held this conversation in other forums, but not here. I think that it boils down to extensibility. Eventually we want revision negotiators to be extensible. The best approach for now is to have locked down plugins (plugins with unsupported opt-in). When things WRT revisions are stable we remove the lock.

Yep. And I wanted it to be documented :)

Personally, I feel like this is a good case for applying the yagni principle. But I'm not going to die on that hill. It's not that important.

I may experiment with a service collector vs. a plugin manager. That will likely result in much less code while providing the same future benefit.

Follows up on #76. I introduced two more articulate exceptions so we can better differentiate between a revision which is missing and version identifier which is malformed.

After doing that, I realized the patch had many duplicated exceptions for impossible states. For example, there were conditions asserting that the entity/storage is revisionable, but the code in question would never be executed if that weren't already true.

Finally, the cacheability of the exceptions was incorrect in a couple places. For example, when the resource type is not revisionable, the cache context should be url.path, not url.query_args:resource_version because the resource type differs per route, not by the version identifier.

I also realized that the patch that I rerolled from did not include some of @e0ipso's recent changes. I'll roll those in in a minute.

Just fixing a couple typos and readability things.

In #69.7 I questioned why the code used is_subclass_of. Now I understand why...

It's because that avoids calling $this->entityTypeManager->getStorage(), which would instantiate a storage class for no other reason than to do an instanceof check. IOW, the is_subclass_of way uses less memory and takes less time.

Okay, this rolls in @e0ipso's changes from #52.

There was some more exception clean-up in VersionByRel to be done as in #81 too.

I looked into #69.6, where the patch was doing if (!method_exists($storage, 'revisionIds')) {. I had hoped to find a cleaner pattern, but it seems that it's a Drupal core shortcoming. See #2986027: Deprecate NodeStorageInterface::revisionIds in favor of entity query. IOW, there isn't a better way to check for that method.

BUT, #2986027: Deprecate NodeStorageInterface::revisionIds in favor of entity query does provide a pattern that we can copy. Doing so will enable us to support the full range of link relation types for all revisionable entity types (the current code will only work for nodes).

Then, when #2986027 lands, we can remove that workaround in favor of the core solution.

That will be next.

this is a good case for applying the yagni principle

With Workspaces in core, I actually think it's pretty clear that we are going to need it? (Or at least that the probability for needing it is very high.)

#84: Let's consider it settled by majority rule then. Like I said, I'm not gonna choose that hill to die on :P

I've implemented the remaining link rels. There's an extensive comment in the interdiff that's worth reading.

Setting to NR for tests, this is still a WIP though.

Reworded that long comment.

Doing so made me realize something. Maybe we shouldn't support predecessor-version, workig-copy-of, prior-working-copy and subsequent-working-copy at in the VersionByRel negotiator.

My thinking was that this would be useful for navigating forward and backward through a version history, but that's what the version-history resource will be for. #2992836: Provide links to resource versions (entity revisions) will also make it possible to navigate forward and backward via hyperlinks.

Shall we remove support for those rels entirely? @e0ipso / @Wim Leers?

I'm fine with going with a MVP in this issue.

Done.

FWIW, I don't think that it's even a feature we want to add later. I think it's just a needless feature altogether. Since by using a version history or hyperlink, you'll be able to get to all those revisions by using the VersionById negotiator. I did not restore the 501 response code for that reason.

I'm fine with going with a MVP in this issue as well.

Code standards.

Gonna work on tests next.

This should make the existing tests pass.

Next patch will add functional tests.

Heh, it would have been nicer if I had included the patch to test.

Heh, AND if the patch would apply.

That last patch did not include the latest on 2.x... rerolled.

Here is a suite of Functional tests. I've only tested Node locally. It does not yet cover content_moderation use cases.

Tests will not pass. I'll post the fixes gradually to better explain the change sets.

Rerolled onto the latest 2.x

+++ b/tests/src/Functional/ResourceTestBase.php
@@ -2594,6 +2595,131 @@ abstract class ResourceTestBase extends BrowserTestBase {
+    $expected_document['links']['self']['href'] = $working_copy_url->setAbsolute()->toString();
+    $expected_document['data']['links']['self']['href'] = $latest_url->setAbsolute()->toString();

While tests are running, let me call this decision out ^

This means that resource objects' self links will always link to their specific revision's URL. This ensures that the link functions like a permalink for that resource object.

OTOH, resource top-level objects' self link will always link to the URL requested.

For example, GET /jsonapi/node/article?resource_version=rel:latest-version will have a top-level self link as /jsonapi/node/article?resource_version=rel:latest-version and a resource object self link as /jsonapi/node/article?resource_version=id:2.

Notably, that also means that GET /jsonapi/node/article (no query parameter) will have a top-level object's self link as /jsonapi/node/article (no query parameter) and a resource object's self link as /jsonapi/node/article?resource_version=id:2.

Testing Drupal\Tests\jsonapi\Functional\NodeTest
.F......F                                                           9 / 9 (100%)

Time: 1.91 minutes, Memory: 4.00MB

There were 2 failures:

1) Drupal\Tests\jsonapi\Functional\NodeTest::testGetIndividual
Failed asserting that Array &0 (
    0 => 'user.permissions'
) is identical to Array &0 (
    0 => 'url.query_args:resource_version'
    1 => 'user.permissions'
).

/var/www/html/modules/contrib/jsonapi/tests/src/Functional/ResourceTestBase.php:698
/var/www/html/modules/contrib/jsonapi/tests/src/Functional/ResourceTestBase.php:932
/var/www/html/modules/contrib/jsonapi/tests/src/Functional/NodeTest.php:286

2) Drupal\Tests\jsonapi\Functional\NodeTest::testRevisions
Failed asserting that Array &0 (
    0 => 'user.permissions'
) is identical to Array &0 (
    0 => 'url.query_args:resource_version'
    1 => 'user.permissions'
).

This is the first failure to address.

Obviously, the cache needs to consider the resource version requested... for two reasons:

1. To make sure that you don't receive the wrong version
2. To make sure that access is respected between revisions. When content_moderation is enabled, that will be more important. For example, if rev 1 is published and rev 2 is unpublished, I shouldn't be able to request ?resource_version=id:1 get an access allowed, then request ?resource_version=id:2 and get access allowed if I'm not allowed to view unpublished content.

Cool, next failure:

Testing Drupal\Tests\jsonapi\Functional\NodeTest
.F......E                                                           9 / 9 (100%)

Time: 1.98 minutes, Memory: 4.00MB

There was 1 error:

1) Drupal\Tests\jsonapi\Functional\NodeTest::testRevisions
Exception: Notice: Undefined index: node
Drupal\jsonapi\Revisions\ResourceVersionRouteEnhancer->enhance()() (Line: 109)

This is just a leftover from the 1.x version of this patch when we renamed the entity parameter.

Testing Drupal\Tests\jsonapi\Functional\NodeTest
.F......F                                                           9 / 9 (100%)

Time: 1.96 minutes, Memory: 4.00MB

There were 2 failures:

1) Drupal\Tests\jsonapi\Functional\NodeTest::testGetIndividual
// I'll fix this later.

2) Drupal\Tests\jsonapi\Functional\NodeTest::testRevisions
The 'errors' member was not as expected.
Failed asserting that Array &0 (
    0 => Array &1 (
        'detail' => 'The current user is not allowed to GET the selected resource. The 'access content' permission is required.'
        'links' => Array &2 (
            'info' => Array &3 (
                'href' => 'http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4'
            )
            'via' => Array &4 (
                'href' => 'http://php-apache-jenkins-drupal8-contrib-patches-39733/subdirectory/jsonapi/node/camelids/3652083c-0b7b-4fb4-99a9-7e607f34797c'
            )
        )
        'source' => Array &5 (
            'pointer' => '/data'
        )
        'status' => 403
        'title' => 'Forbidden'
    )
) is identical to Array &0 (
    0 => Array &1 (
        'detail' => 'The current user is not allowed to GET the selected resource. The 'access content' permission is required.'
        'links' => Array &2 (
            'info' => Array &3 (
                'href' => 'http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4'
            )
            'via' => Array &4 (
                'href' => 'http://php-apache-jenkins-drupal8-contrib-patches-39733/subdirectory/jsonapi/node/camelids/3652083c-0b7b-4fb4-99a9-7e607f34797c?resource_version=id%3A1'
            )
        )
        'source' => Array &5 (
            'pointer' => '/data'
        )
        'status' => 403
        'title' => 'Forbidden'
    )
).

This failure is caused by access denied errors that should have a via link that includes a resource version ID.

Code standards.

And this patch makes sure that resource objects have the right links too.

Rerolled after: #3006270: Add ResourceTypeRepository::createResourceType() for easier JSON API Extras support and simpler code

Whew, that messed things up :P

Easy fix. It was a copy/paste thing. Of course one can't call getRevisionId() on an entity which does not implement RevisionableInterface.

After going down the path of adding the "correct" self links to error and resource objects. I think that self links actually belong in a #2992836: Provide links to resource versions (entity revisions) too. They raise a new set of questions that could derail this issue and greatly expand the patch size.

Okay, the last patch succeeded except for new CS violations. This resolves those.

My next patches should start to make assertions for when content_moderation is installed.

#115++

This should fix many of the errors like this one:

1) Drupal\Tests\jsonapi\Functional\UserTest::testRevisions
LogicException: Entity type user does not support revisions.

This makes the assertions more generic, ie, not Node specific.

I'm doing this to fix MediaTest, which will mean we have at least two revisionable entity types tested thus far.

Removing the unused use statement.

And now, here are the tests for revisions + content_moderation. They should pass for at least Node and Media.

Correct interdiff for #124.

Oh look, it's our old friend MessageTest.

Easy fix for BlockTest.