Support version negotiation for any entity type (currently only Node & Media are supported)

All correct! Unfortunate, isn't it? 😔

It seems like you're just stating facts in this issue. So … what were you hoping to achieve with this? To have an issue to follow so you know when generic support is added? If so: 👍🙏

If not: please clarify :)

Don't apologize — we should've created this issue ourselves instead of only pointing to a core issue. There already is pretty strong demand for this feature in core thanks to the Workflow Initiative. But it sure helps to have this issue to gauge interest!

So thank you! 🙏

Clarifying issue title.

Per #2795279-66: [PP-2] [META] Revisions support, this is not blocked on #2350939: Implement a generic revision UI anymore, but on #3043321: Use generic access API for node and media revision UI, which captures only the relevant subset of #2350939.

I think it might be worth temporarily updating the change record (https://www.drupal.org/node/3015434) to articulate that this is the case. Not just that only node and media entities are supported but also that if loading node entities with included references to revisioned entities (e.g. entity_reference_revisions / paragraphs), those included entities will always be the most recent version.

It also seems a little regrettable that there is simply no way (at least that I could find) of getting around this without hacking core.

@PQ did you find a solution, even if you hack the core ?

Hi @lois.chabrand. I did end up patching core for our requirements. I've attached the patch file if it helps you out. It only adds support for paragraphs included as part of a node but hopefully if you're not using those then it might serve as a pointer for other entity types.

Hi @PQ,
thanks for your patch, it seems to work for paragraph included in Node, unfortunately we have an other issue with nested paragraph included in Node.

Hi .. thx for work on this, wondering what the status of this is now?

#3043321: Use generic access API for node and media revision UI landed! 😊 Time to re-evaluate this.

@PQ: to make it work as you're hoping for with Paragraph entities does it also need this change?

diff --git a/core/modules/jsonapi/src/Revisions/ResourceVersionRouteEnhancer.php b/core/modules/jsonapi/src/Revisions/ResourceVersionRouteEnhancer.php
index 853261346c..98ac32dfef 100644
--- a/core/modules/jsonapi/src/Revisions/ResourceVersionRouteEnhancer.php
+++ b/core/modules/jsonapi/src/Revisions/ResourceVersionRouteEnhancer.php
@@ -91,7 +91,7 @@ public function enhance(array $defaults, Request $request) {
 
     // If the resource type is not versionable, then nothing needs to be
     // enhanced.
-    if (!$resource_type->isVersionable()) {
+    if (!$resource_type->isVersionable() && !in_array($resource_type->getEntityTypeId(), ['node', 'media', 'paragraph'])) {
       // If the query parameter was provided but the resource type is not
       // versionable, provide a helpful error.
       if ($has_version_param) {

Hi @DamienMcKenna,

I don't really recall why that line was there, it may have been to prevent other entities from having negative effects from the changes.

That said, my patch was fairly bespoke to the requirement of just getting it working for paragraphs so I don't know if it will be a good starting point for more generic entity handling. I'll be delighted if it does help in that endeavour in any way though :)

Updated the issue summary to reflect the current status.

That's all from me for today. I believe the remaining fails are due to \Drupal\Tests\jsonapi\Functional\ResourceTestBase::testRevisions trying to test content_moderation on unmoderatable entity types (MenuLinkContent, PathAlias, Term).

I started review on this and got pretty far, but have ended up out of time unfortunately.

Code looking good. I was just thinking if there is any tests left that don't test revisions.

This seems like overkill, but \Drupal\taxonomy\TermAccessControlHandler::checkAccess doesn't handle the 'view all revisions' case yet.

Also your comment on core/modules/jsonapi/tests/src/Functional/TermTest.php did trigger me, is there an issue for that somewhere on the queue or do we need a child issue for that?

I hope i can resume this sometime soon, but if someone else wants to dive in, please don't hessitate to do so.

Also your comment on core/modules/jsonapi/tests/src/Functional/TermTest.php did trigger me, is there an issue for that somewhere on the queue or do we need a child issue for that?

I couldn't find one, we should create one as a blocker for #2936995: Add taxonomy term revision UI.

That sounds like it is a good idea (tm) :)

One thing that kinda worries me when looking at this that the changes to the tests mean those are only tested while revisionable. Wouldn't it make sense to keep the old unrevisioned tests, but perhaps extend the tests to run once while revisionable and once without?

I think changing all the existing tests like this would probably get called back by committers to be honest. So rather introduce something like BlockContentRevisionableTest I think.

Just saw this issue #3225953: Add generic entity-type test coverage for JSON:API and revisions which seems related.

Patch for composer workflows.

Ok, I went through this issue again. Tried to have a go at the separation, but it doesn't make too much sense to separate. Since these types are revisionable they should be treated as such. Setting RTBC.

We do need a changerecord, so i created one.

Random layout builder fail

Can we get another MR against 10.0.x for this?

Thanks

Adding tag Needs reroll, but only for a 10.0.x MR.

Left a comment on the MR

Update to #13 patch to work with Drupal 9.3.7

I've created the follow-up issue #3269029: Accesscheck for Terms doesn't support the new 'view all revisions' permission. Considering this back to RTBC, we can fix the taxonomy access later.

I don't see any taxonomy changes in #2350939: Implement a generic revision UI so I'm not sure what the approach would be for #3269029: Accesscheck for Terms doesn't support the new 'view all revisions' permission.

Looking at TermAccessControlHandler, its not clear what we'd do.

I suspect rather than defer to either 'administer taxonomy' we'd probably need to add new permissions.

The basis that we need a discussion on how to handle this is one reason not to hold up this issue. But it begs the question, is this patch even useful at all for terms if you need to grant the 'administer taxonomy' permission? I don't think it is. I understand that it also makes it available for media, block_content and a myriad of entity-types in the contrib space. So perhaps that's another vote not to hold it up. I'll raise it on the next committers meeting.

Yeah I would argue that this is quite useful even if terms are not supported.

For anyone using 9.2, here's a reroll of the MR that's working for us on 9.2.11.

I discussed this with other committers at a recent core-committers meeting.

The consensus was (to quote webchick)

ship a useful thing, then later ship an improvement to said useful thing" rather than "wait until all facets of thing are useful before shipping it."

I've rebased both the 9.4.x and 10.0.x MRs via the gitlab UI.

I'll let the tests run on both branches and revisit this in the morning.

Nice, thanks @larowlan

Thanks folks, this is in.

I'll publish the change record.

See you in the followups

https://www.drupal.org/project/drupal/issues/3031271#comment-13706800

@lois.chabrand , @PQ - thank you very much, with small changes that works good to drupal 9.5.2

update to https://www.drupal.org/project/drupal/issues/3031271#comment-14884943 - it was working good, until we deleted some nodes which share the paragraph

afterthat we getting

Error: Call to a member function getEntityType() on null in Drupal\jsonapi\Access\EntityAccessChecker>checkRevisionViewAccess() (line 249 of web/core/modules/jsonapi/src/Access/EntityAccessChecker.php)

    switch ($entity_type->id()) {
      case 'node':
      case 'media':
        $access = $entity->access('view all revisions', $account, TRUE);
        break;
      // Project-specific modification... Allow access to revisions of paragraphs
      // if the current user has access to the parent entity and to this
      // version of this paragraph. Note, this is not a perfect implementation.
      // Ideally we should be checking for access to the specific version of the
      // parent entity but there appears to be no way to do that with
      // information available at this point until
      // https://www.drupal.org/project/paragraphs/issues/2954512 is committed.
      case 'paragraph':
        assert($entity instanceof ParagraphInterface);
        $parent = $entity->getParentEntity();
        // Parent became grand parent
        while ($parent->getEntityType()->id() === 'paragraph') {   // <--------- line 249
          $parent = $parent->getParentEntity();  
        }

adding patch which overcomes https://www.drupal.org/project/drupal/issues/3031271#comment-15257741

It basically ignores parent part of access check if parent is null