JSON:API is typehinting LatestRevisionCheck because there is no Entity Revision Access API yet, remove that typehint once such an API exists

Created on 22 February 2019, over 5 years ago
Updated 4 May 2023, about 1 year ago

This module causes a fatal error on cache rebuild when the patch from https://www.drupal.org/project/group/issues/2906085 → is applied. This happens because the method in Drupal\jsonapi\Access\EntityAccessChecker::setLatestRevisionCheck() uses a Drupal\content_moderation\Access\LatestRevisionCheck object instead of the corresponding interface. This makes the method much more difficult to override. At a glance, it looks like using AccessInterface here should work fine. So I'm going to attach a patch that does that.

✨ Feature request
Status

Needs work

Version

10.1 ✨

Component
JSON API  →

Last updated 2 days ago

Created by

🇨🇦Canada Dylan Donkersgoed London, Ontario

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇮🇳India Nikhil_110

    Attached patch against Drupal 10.1.x

  • 🇮🇳India sahil.goyal

    Patch #36 is conflicting to apply to D10.1.x, so Updated the patch to make it compatible and trying to Fix the errors.

  • 🇳🇱Netherlands bbrala Netherlands

    Let me get a review in :)

    1. +++ b/core/modules/jsonapi/src/Access/EntityAccessChecker.php
      @@ -67,9 +67,9 @@ class EntityAccessChecker {
      +  protected $AccessInterface = NULL;
      

      This feels weird, the variable name shouldn't start with a capital letter.

    2. +++ b/core/modules/jsonapi/src/Access/EntityAccessChecker.php
      @@ -95,14 +95,14 @@ public function __construct(ResourceTypeRepositoryInterface $resource_type_repos
      +  public function setLatestRevisionCheck(AccessInterface $latest_revision_check) {
      

      Since we are broadening the hint, i think this is not bc-breaking. So this is fine.

    3. +++ b/core/modules/jsonapi/src/Access/EntityAccessChecker.php
      @@ -196,8 +196,8 @@ protected function checkRevisionViewAccess(EntityInterface $entity, AccountInter
      +    if ($entity_type->getLinkTemplate('latest-version') && $entity->isLatestRevision() && isset($this->AccessInterface)) {
      

      I wonder, if you already have access to view all revisions, do we even need to check for the specific revision? Or could you have permissions to see all revisions and then get denied for some specific revision? Although technically possible i kinda wonder why that would be the case.

    Summarizing; some small changes needed, the change seems reasonable now that the blocking issue is merged.

  • 🇨🇭Switzerland amermod

    We are still talking about revision checks while it should be the role of the EntityAccessControlHandler to do the revision access check.

    And it actually does that already, so why is there still a revision access check in the jsonAPI module ? Because the EntityAccessControlHandler can not be trusted for revisions, as said in #3, four years ago ?

    I just attached a patch that removes simply the revisions check, to see what will fail.

  • last update about 1 year ago
    29,374 pass
  • last update about 1 year ago
    30,325 pass
Production build 0.69.0 2024