Remove JSON API's use of $context['cacheable_metadata']

Now only #2940342 is a blocker.

#2940342: Cacheability metadata on an entity fields' properties is lost landed some time ago, this is no longer blocked!

Yoink, taking this one.

(Worked on this on the flight to DrupalCon Nashville!)

This is basically the goal; the code this issue should remove.

#2952714: Group module's GroupAccessResult::allowedIfHasGroupPermission(s)() does not include cacheability may end up blocking this.

#2952714: Group module's GroupAccessResult::allowedIfHasGroupPermission(s)() does not include cacheability was derailed, and is now a group issue. #2966384: JSON API's LinkManager does not handle cacheability correctly, just like its origin (HAL's LinkManager) is the successor.

And while related, AFAICT it won't block this.

Rebased #8.

All the failures in JsonApiDocumentTopLevelNormalizerTest are due to

    $response = new ResourceResponse();
    $normalized = $this
      ->getNormalizer()
      ->normalize($document_wrapper->reveal(), 'api_json', [
        'request' => $request,
        'resource_type' => $resource_type,
        'cacheable_metadata' => $response->getCacheableMetadata(),
      ]);

i.e. the test was relying on the fact that it is okay to call the normalizer directly, but since #2940342: Cacheability metadata on an entity fields' properties is lost, the cacheability is being bubbled as intended, via normalizer value objects. And this patch is removing JsonApiDocumentTopLevelNormalizer's duplicate/direct bubbling onto the response object's cacheability ('cacheable_metadata' => $response->getCacheableMetadata(),), hence the assertions are failing, even though the bubbling is definitely still happening.

All content entity type integration tests are passing just fine. Besides JsonApiDocumentTopLevelNormalizerTest, only the config entity type integration tests are failing, and they're all failing for the same reason: the tested config entity's cache tag is missing.

Unfortunately, the Symfony serializaton system is what gets in the way here: in principle it requires that an array is returned. But we're already not doing that for EntityNormalizer (returns EntityNormalizerValue), FieldNormalizer (returns FieldNormalizerValue), and so on. The only place where we are doing this, is in \Drupal\jsonapi\Normalizer\JsonApiDocumentTopLevelNormalizer::normalize(). That first constructs a JsonApiDocumentTopLevelNormalizerValue object (similar to the aforementioned ones), but then it still converts it to an array. In the process of converting it to an array, the cacheability is lost. That's why

+++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
@@ -170,9 +170,6 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
-    if (!empty($context['cacheable_metadata'])) {
-      $context['cacheable_metadata']->addCacheableDependency($value_extractor);
-    }

existed, but this patch wants to remove it.

So, how to proceed?

Well, by making JsonApiDocumentTopLevelNormalizer behave like all other normalizers in JSON API and returning a value object, so that the cacheability can be returned! It's up to the caller to convert this to an array prior to encoding.

Ahhh, much better! Down to 3 failures!

Those 3 failures are actually 4 errors and 1 failure in JsonApiDocumentTopLevelNormalizerTest . Specifically, ::testNormalize(), ::testNormalizeUuid(), ::testNormalizeConfig() and ::testCacheableMetadata(). All of those failures are happening because that kernel test still is expecting JsonApiDocumentTopLevelNormalizer to return arrays rather than value objects. Easy to fix.

(Plus a failure in EntityToJsonApiTest. But we'll deal with that later. Since that service is going away in the 2.x branch, it's the lowest priority.)

The remaining failures are 2 errors in JsonApiDocumentTopLevelNormalizerTest::testNormalize() and a failure in ::testNormalizeUuid(). Because includes aren't being rasterized. Because that logic was moved to \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber::toArray() in #14.

But in the case of the top-level JSON API document's normalizer value object, it makes sense for its rasterization to include includes, since there's no next level to bubble it to! So we can simplify this code, and simultaneously fix things.

Now JsonApiDocumentTopLevelNormalizerTest is passing! But JsonApiFunctionalTest::testRead() is failing:

1) Drupal\Tests\jsonapi\Functional\JsonApiFunctionalTest::testRead
reset() expects parameter 1 to be array, null given

First fixing EntityToJsonApiTest. Single line fix.

That last failure, in JsonApiFunctionalTest::testRead() was triggered by the changes in #19's patch.

And it actually is related to one of the problems I found earlier. This is what HEAD outputs for /jsonapi/node/article/2ba2f3ba-6625-4874-8ec8-6e4327055f84/relationships/uid in HEAD:

{
  "data": {
    "type": "user--user",
    "id": "6ca85db5-4c1e-43a0-8651-a66270985246"
  },
  "links": {
    "self": "http://d8/jsonapi/node/article/2ba2f3ba-6625-4874-8ec8-6e4327055f84/relationships/uid",
    "related": "http://d8/jsonapi/node/article/2ba2f3ba-6625-4874-8ec8-6e4327055f84/uid"
  }
}

Note how the top-level "jsonapi" key is missing. We actually have an issue for this: #2949807: Spec Compliance: Error responses are missing the `jsonapi` top-level member!

The root cause of this bug is made obvious by this patch:

+++ b/src/EventSubscriber/ResourceResponseSubscriber.php
@@ -123,34 +124,32 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
+  protected static function toArray(ValueExtractorInterface $jsonapi_doc_object) {
+    assert($jsonapi_doc_object instanceof JsonApiDocumentTopLevelNormalizerValue || $jsonapi_doc_object instanceof RelationshipNormalizerValue);

Apparently the to-be-rasterized value can be either JsonApiDocumentTopLevelNormalizerValue or RelationshipNormalizerValue. But we'd expect this to always be JsonApiDocumentTopLevelNormalizerValue!

And since #19 moves the "rasterization of includes" logic to \Drupal\jsonapi\Normalizer\Value\JsonApiDocumentTopLevelNormalizerValue::rasterizeValue(), it's now only happening for JsonApiDocumentTopLevelNormalizerValue, and no longer for RelationshipNormalizerValue. (In HEAD, this happens in JsonApiDocumentTopLevelNormalizer, which may be called with a RelationshipNormalizerValue object.)

The failing line is:

$first_include = reset($single_output['included']);

… i.e. the includes are missing from the rasterization. This makes sense given the above analysis!

The solution is pretty simple: always require the to-to be normalized/rasterized value to be a JsonApiDocumentTopLevelNormalizerValue value object. Also when accessing the relationship route. That also allows ResourceResponseSubscriber to be much simpler. The one complication is that RelationshipNormalizerValue also needs to bubble a related link, but that's easy enough to support in JsonApiDocumentTopLevelNormalizerValue::rasterizeValue().

#24 should make JsonApiFunctionalTest::testRead() pass again. But now the ResourceTestBase::testRelationships() tests will fail (expected [], actual FALSE). Slight tweaking of JsonApiDocumentTopLevelNormalizerValue::rasterizeValue()'s logic is sufficient. The line in question only really matters for collections, so the change should be fine. Our extensive test coverage will help (dis)prove the change.

CS violations.

Green! 🎉

Time for clean-up. Fixed the last CS violation. Updated ResourceResponseSubscriber's class docblock per the changes above. And most importantly: removed all the render context shenanigans (which includes capturing of out-of-band bubbled cacheability) from ResourceResponseSubscriber, which makes it much, much simpler.

Self-review:

1. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -121,32 +109,13 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
   +      $jsonapi_doc_object =  $serializer->normalize($data, $format, [
   +        'request' => $request,
   +        'resource_type' => $request->get('resource_type'),
   +      ]);
   +      assert($jsonapi_doc_object instanceof JsonApiDocumentTopLevelNormalizerValue);
   +      $response->addCacheableDependency($jsonapi_doc_object);
   +      $response->setContent($serializer->encode($jsonapi_doc_object, $format));
   

   This could use a comment.

2. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -195,6 +176,8 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   +   * @todo Remove this when \Drupal\jsonapi\Normalizer\RelationshipItemNormalizer::normalize() removes its call to ::buildNormalizerValue()
   

   #2962461: JsonApiDocumentTopLevelNormalizer is SerializerAware but doesn't get the serializer injected is doing just that, and is green!

3. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1582,17 +1582,14 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -      // @todo Uncomment this in https://www.drupal.org/project/jsonapi/issues/2949807
   -      // @codingStandardsIgnoreStart
   -      //'jsonapi' => [
   -      //  'meta' => [
   -      //    'links' => [
   -      //      'self' => 'http://jsonapi.org/format/1.0/',
   -      //    ],
   -      //  ],
   -      //  'version' => '1.0',
   -      //],
   -      // @codingStandardsIgnoreEnd
   +      'jsonapi' => [
   +        'meta' => [
   +          'links' => [
   +            'self' => 'http://jsonapi.org/format/1.0/',
   +          ],
   +        ],
   +        'version' => '1.0',
   +      ],
   

   This is effectively doing half of what #2949807: Spec Compliance: Error responses are missing the `jsonapi` top-level member aims to do: it fixes the problem for relationship responses, which means that #2949807 only still has to take care of error responses.

Great, #31 passed tests too! There is still one more place where we need to remove render context-based cacheability capturing: JSON API's RequestHandler. Let's see if tests still pass with that removed. (I already know they won't.)

One small thing was forgotten in #2966384: JSON API's LinkManager does not handle cacheability correctly, just like its origin (HAL's LinkManager).

Of course Shortcut has to be special. The root cause is \Drupal\link\Plugin\Validation\Constraint\LinkNotExistingInternalConstraintValidator::validate() calling Url::toString(), which bubbles cacheability. Created #2982060: Un-skip skipped JSON:API tests because LinkNotExistingInternalConstraintValidator is no longer incorrectly bubbling cacheability to fix this.

Cleaned up #33 (unused use, but most importantly: removed the injected renderer service).

Also addressed #32.1.

All green. IMHO this is ready to be committed.

#2962461: JsonApiDocumentTopLevelNormalizer is SerializerAware but doesn't get the serializer injected landed, now addressing #32.2…

🎉🚀

1. :D
2. ✔️
3. The second can be, the first can't, because CS violation otherwise.
4. I specifically used += to not overwrite it, since \Drupal\jsonapi\Normalizer\Value\RelationshipNormalizerValue::getLinks() already computes it. There's no point in recomputing it. So, kept this.
5. :)