Remove access to the Request object in the normalization process

1. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -111,10 +113,7 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
          // First normalize the data.
   ...
   -      ] + JsonApiDocumentTopLevelNormalizer::expandContext($request, $request->get('resource_type')));
   +      $jsonapi_doc_object = $serializer->normalize($data, $format, static::generateContext($request));
   

   😍

2. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -127,6 +126,53 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
   +   * Generates the context to pass to normalizers.
   

   "Generates a top-level JSON API normalization context."

3. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -127,6 +126,53 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
   +   *   The request to get the URL params from to expand the context.
   

   "The request from which the context can be derived."

4. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -127,6 +126,53 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
   +      'request' => $request,
   

   I don't think we can call this done until this is removed. Ofc, that's what you're asking about in #11... see below.

[Problems] in \Drupal\jsonapi\Normalizer\Value\JsonApiDocumentTopLevelNormalizerValue::rasterizeValue().

Thoughts?

I think this is just the kind of information that should be carried by the JsonApiDocumentTopLevel object. I'd add a $links argument to its constructor which receives an array of Url objects keyed by link relation type.

In that constructor: assert(empty(array_diff(array_keys[$links], ['self', 'first', 'next', 'prev', 'last', 'related'])))

Then, do all that work with the link manager in the controller rather than in the normalization process.

^{This will make it easier to capture that pesky cache data too!}

Initial review below. I made a few nits that I know you said were not yet done, but /shrug :P

Overall, I love the direction of this patch!

that requires extensive changes to LinkManager.

What kind of changes? Perhaps we should make a prerequisite issue for it? My dream would to see LinkManager simply removed, or made into an internal class with public static helper methods, or a really solid API for adding custom links to various levels of the document introduced. Or some combination of those things.

1. +++ b/src/Controller/EntityResource.php
   @@ -801,12 +801,15 @@ class EntityResource {
   +   * @param \Drupal\Core\Url[]|string[] $links
   +   *   The URLs to which to link. A 'self' link is added automatically.
   ...
   +    $links['self'] = $this->linkManager->getRequestLink(\Drupal::request());
   

   I'd like to see the self link be explicit, unless that would explode the scope of this patch.

2. +++ b/src/Controller/EntityResource.php
   @@ -821,7 +824,14 @@ class EntityResource {
   +        ? $entity_collection->getTotalCount()
   
   +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -110,10 +113,7 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
   -      $jsonapi_doc_object = $serializer->normalize($data, $format, [
   -        'request' => $request,
   -        'resource_type' => $request->get('resource_type'),
   -      ]);
   
   @@ -126,6 +126,52 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
   +    $context = [
   +      'account' => NULL,
   +      'sparse_fieldset' => NULL,
   +      'resource_type' => $resource_type,
   +      'include' => $public_includes,
   +    ];
   

   This shows that the request is no longer added part of the normalization process.

3. +++ b/src/Controller/EntityResource.php
   @@ -821,7 +824,14 @@ class EntityResource {
   +    $collection_links = $this->linkManager->getPagerLinks(\Drupal::request(), $link_context);
   

   Nit: I would call these "pagination_links"

   I like that this extracts the link manager from the normalization process in at least one case. It always felt a little like the old CurrentContext class, in that it weaves its way through everything.

4. +++ b/src/JsonApiResource/JsonApiDocumentTopLevel.php
   @@ -22,14 +25,27 @@ class JsonApiDocumentTopLevel {
   +   * @param \Drupal\Core\Url[]|string[] $links
   +   *   The URLs to which to link.
   

   "The URLs to which the top-level document should link."

5. +++ b/src/JsonApiResource/JsonApiDocumentTopLevel.php
   @@ -22,14 +25,27 @@ class JsonApiDocumentTopLevel {
   +    assert(empty(array_diff(array_keys($links), ['self', 'first', 'next', 'prev', 'last', 'related'])));
   

   Perhaps we should document the allowed link relations in the docblock.

6. +++ b/src/JsonApiResource/JsonApiDocumentTopLevel.php
   @@ -42,4 +58,14 @@ class JsonApiDocumentTopLevel {
   +   *   The document-level links.
   

   s/document/top

7. +++ b/src/Normalizer/RelationshipItemNormalizer.php
   @@ -60,7 +60,7 @@ class RelationshipItemNormalizer extends FieldItemNormalizer {
          $entity_and_access = EntityResource::getEntityAndAccess($target_entity);
   -      $included_normalizer_value = $this->serializer->normalize(new JsonApiDocumentTopLevel($entity_and_access['entity']), $format, $context);
   +      $included_normalizer_value = $this->serializer->normalize(new JsonApiDocumentTopLevel($entity_and_access['entity'], []), $format, $context);
   

   This will need to be adjusted to account for #2987206: Refactor `getEntityAndAccess` to return cacheable objects with access cacheability rather than an entity/access pair.

8. +++ b/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
   @@ -24,18 +26,16 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
   -   * The includes.
   -   *
   -   * @var array
   +   * @var array|\Drupal\Core\Url[]|string[]
   

   Missing a comment.

9. +++ b/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
   @@ -45,34 +45,17 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
   -  public function __construct(array $values, array $context, array $link_context, $is_collection = FALSE) {
   +  public function __construct(array $values, array $links, $is_collection = FALSE) {
   

   This is a much nicer signature for a value object.

Let's mark it @internal here?

That's a first step in the right direction :)

What I was trying to say is I don't think it the LinkManager should have its own state, i.e. if every method is public static, its methods exist only to avoid code duplication. IOW, this would ensure that it's no longer a dependency woven through everything. It would become a simple set of reusable functions. I don't think that should happen here.

OTOH, I've also it at the back of my mind that JSON API needs a public API for customizable HATEOAS links. My last remark and this need are related, but not compatible. I just want it to be on your mind too.

1. I'm not sure what you mean by this. Do you mean that each controller method should generate its own self link?

Exactly. In general, I'd like to see links explicitly passed to JsonApiDocumentTopLevel and to each value object that has links, rather than having value objects generating their own links.

3 & 5. WFM!

It seems impossible to combine the goal of this issue ("normalizers unaware of Request object") with the intent to have normalizer value objects generate their own links? Or what am I missing?

I think you understood the reverse of what I wrote. I do not want any value objects to generate their own links (or call the LinkManager).

Will respond to #27 soon. I'll have to stare at it a bit.

Re: #27

What about something like this? Can't it be an explicit argument to the value object? Am I missing something?

It's not very elegant, but it's an improvement :P

$meta ... in #35 is a good suggestion.

Couple nits, and a q. After that, RTBC I think.

1. +++ b/src/Controller/EntityResource.php
   @@ -792,12 +792,17 @@ class EntityResource {
   +    $links['self'] = $this->linkManager->getRequestLink(\Drupal::request());
   

   Why not inject the request?

2. +++ b/src/Controller/EntityResource.php
   @@ -805,18 +810,28 @@ class EntityResource {
   +      'total_count' => $resource_type->includeCount()
   +        ? $entity_collection->getTotalCount()
   +        : FALSE,
   

   Nit: Can we just add this to the array conditionally, rather than overloading total_count?

3. +++ b/src/Controller/EntityResource.php
   @@ -805,18 +810,28 @@ class EntityResource {
   +    $meta = $link_context['total_count'] === FALSE
   

   Nit: let's use $resource_type->includeCount() instead of checking for FALSE.

Awesome :)

+++ b/src/Controller/EntityResource.php
+++ b/src/Controller/EntityResource.php
      // though the normalizer skips disabled references, we can avoid unnecessary

+++ b/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
+++ b/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
 -    // Flatten the includes.
...
-       if (!$context['request']->get('_on_relationship')) {
++      if (!\Drupal::requestStack()->getCurrentRequest()->get('_on_relationship')) {

This was my fault. Can't we just add a $context['on_relationship'] === TRUE in generateContext? No, because then we'd have to add the $context param back.

+++ b/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
@@ -55,18 +57,11 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
-   *

@@ -74,25 +69,25 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
+      if (!\Drupal::requestStack()->getCurrentRequest()->get('_on_relationship')) {
         // Make sure that different sparse fieldsets are cached differently.
         $this->addCacheContexts(array_map(function ($query_parameter_name) {
           return sprintf('url.query_args:%s', $query_parameter_name);
@@ -100,13 +95,24 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,

@@ -100,13 +95,24 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
       }

Perhaps we can add these cache contexts somewhere else then? E.g. in EntityResource::getCollection()

AND HECK YEAH :D

Bump.

In this particular case I think this issue is a step in the right direction.

❤️

1. IIRC, because value objects probably shouldn't be generating their own links. Also, doing so removes the need to pass in the LinkManager which depends on the request object. I agree with your confusion about accepting string or a Url objects. I'll investigate.

2. I wouldn't call this particular instance "babysitting." This guarantees that the response subscriber is getting an upcasted value, not a string. Given the complexity of the routing system, I'm happy to have this assert there so that we can quickly pinpoint when/if that ever breaks.

3. I agree. In a better language we'd make this an explicit argument using an enum. I did my best to replicate that in the attached patch.

4. The first was my suggestion in #13 and I remember @Wim Leers was actually not entirely sold either. Since then, I've actually come to think about link object keys differently. Therefore, I've removed the first assertion because I agree that it's not helpful. WRT to the second assertion, I guess it comes down to the same arguments that exist between type safe and untyped languages. My preference is on the latter, so I appreciate these kinds of type assertions.

Re: #54.1 I removed the Url stuff. I imagine @Wim Leers was prepping to better handle link cacheability, but that can happen in a separate issue.

2 CS violations and forgot to update a unit test.

IMHO, this is ready.

I lied. This gets rid of some outdated comments since #55.

🎉