Convert EntityAccessDeniedHttpException into cacheable exception

Should we check that there is cache context per user?

I don't see the harm in adding it in the controller though. So I did that along with asserting the user cache context.

I do — it's duplicating logic in multiple places, which then is confusing. The controller's job is to create a ResourceResponse object containing a JsonApiDocumentTopLevel object that contains the entities/entity collection/exceptions to normalize. The normalization happens in ResourceResponseSubcriber, and that is when cacheability is determined and associated.

At the time of the controller execution, there is no need to worry about cacheability of the values about to be normalized, because values haven't yet been normalized.

I think I see several scope creep things, sorry :( Those bits of scope creep make this patch much complex than it would've been if it did only what the title indicates it does.

1. +++ b/src/Exception/EntityAccessDeniedHttpException.php
   @@ -4,16 +4,17 @@ namespace Drupal\jsonapi\Exception;
   -class EntityAccessDeniedHttpException extends HttpException {
   +class EntityAccessDeniedHttpException extends CacheableAccessDeniedHttpException {
   

   This is the change that the issue title is talking about! 👌

2. +++ b/src/Controller/EntityResource.php
   @@ -134,20 +130,12 @@ class EntityResource {
   -  public function getIndividual(EntityInterface $entity, Request $request, $response_code = 200) {
   ...
   +  public function getIndividual(EntityInterface $entity) {
   
   @@ -312,13 +300,11 @@ class EntityResource {
   -  public function deleteIndividual(EntityInterface $entity, Request $request) {
   +  public function deleteIndividual(EntityInterface $entity) {
   
   +++ b/src/Controller/RequestHandler.php
   @@ -131,8 +131,17 @@ class RequestHandler {
   +    $extra_parameters = [];
   +
        // Only add the unserialized data if there is something there.
   -    $extra_parameters = $unserialized ? [$unserialized, $request] : [$request];
   +    if ($unserialized) {
   +      $extra_parameters[] = $unserialized;
   +    }
   +
   +    // Only add the request object for the methods that require it.
   +    if (in_array($action, ['createIndividual', 'patchIndividual', 'getCollection', 'getRelated', 'deleteRelationship'])) {
   +      $extra_parameters[] = $request;
   +    }
   

   I like this tightening! But very much out of scope AFAICT. Let's do this in a separate issue.

   Also: I am concerned about hardcoding those "extra parameters" in RequestHandler. I think a better solution is to use the arguments resolver like REST started doing in #2720233: Use arguments resolver in RequestHandler to have consistent parameter ordering — thanks to @dawehner for that one! :)

3. +++ b/src/Controller/EntityResource.php
   @@ -939,37 +912,29 @@ class EntityResource {
   +   * @return \Drupal\Core\Entity\EntityInterface|\Drupal\jsonapi\LabelOnlyEntity|\Drupal\jsonapi\Exception\EntityAccessDeniedHttpException
   +   *   The loaded entity, a label only version of that entity or an
   +   *   EntityAccessDeniedHttpException object if neither is accessible.
       */
   -  public static function getEntityAndAccess(EntityInterface $entity) {
   +  public static function getAccessCheckedEntity(EntityInterface $entity) {
   ...
   -    $output = [
   -      'access' => $access,
   -      'entity' => $entity,
   -    ];
   ...
   +      $entity->addCacheableDependency($label_access);
   ...
   -        $output['entity'] = new EntityAccessDeniedHttpException($entity, $access, '/data', 'The current user is not allowed to GET the selected resource.');
   +        return new EntityAccessDeniedHttpException($entity, $access->orIf($label_access), '/data', 'The current user is not allowed to GET the selected resource.');
   

   The issue title suggests this is only about throwing the appropriate exception.

   This goes quite a bit further: it refactors the existing logic from returning a pair (entity + access) to returning an access-checked entity object (entity object with access result cacheability, IF accessible), or an access-checked label, or an exception object.

   AFAICT this is out of scope? Or am I not seeing why this is necessary for point 1?

Waiting for tests to pass, then I'll commit this!

+++ b/src/LabelOnlyEntity.php
@@ -2,6 +2,8 @@
+use Drupal\Core\Cache\CacheableDependencyInterface;
+use Drupal\Core\Cache\CacheableDependencyTrait;

Unnecessary. I'll remove these on commit.