Add a Link and LinkCollection class to support RFC8288 web linking.

High-level review.

I like this a lot. The value objects seem to fit beautifully and naturally simplify things.

1. +++ b/jsonapi.services.yml
   @@ -84,6 +84,14 @@ services:
   +    class: Drupal\jsonapi\Normalizer\LinkCollectionNormalizer
   ...
   +    class: Drupal\jsonapi\Normalizer\WebLinkNormalizer
   

   "Link Collection" and "Web Link". But doesn't that mean it's "Web Link Collection"?

2. +++ b/src/JsonApiResource/LinkCollection.php
   @@ -0,0 +1,134 @@
   +class LinkCollection implements \IteratorAggregate {
   
   +++ b/src/JsonApiResource/WebLink.php
   @@ -0,0 +1,172 @@
   +class WebLink implements CacheableDependencyInterface {
   

   Thinking out loud: LinkRelation and LinkRelations.

   (RFC8288 does not define "web link" as a concept. It's merely titled "web linking" as in "linking on the web". Otherwise I'd +1 WebLink).

3. +++ b/src/JsonApiResource/LinkCollection.php
   @@ -0,0 +1,134 @@
   +   * @return static
   +   *   A new LinkCollection with the given link inserted or merged with the
   +   *   current set of links.
   +   */
   +  public function withLink($key, WebLink $new_link) {
   ...
   +   * @return \Drupal\jsonapi\JsonApiResource\LinkCollection
   +   *   A new, filtered LinkCollection.
   +   */
   +  public function filter(callable $f) {
   ...
   +   * @return \Drupal\jsonapi\JsonApiResource\LinkCollection
   +   *   A new LinkCollection with the links of both inputs.
   +   */
   +  public static function merge(LinkCollection $a, LinkCollection $b) {
   

   Immutable: 👍

4. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -173,12 +173,13 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   +    $links = $this->serializer->normalize($object->getLinks(), $format, $context);
   
   +++ b/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
   @@ -92,12 +91,13 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
   +  public function __construct($document_type, array $values, NormalizedValue $links, $cardinality = FALSE, $includes = FALSE, array $meta = []) {
   ...
   +    $this->links = $links;
   
   +++ b/src/Normalizer/Value/NormalizedValue.php
   @@ -0,0 +1,51 @@
   +class NormalizedValue implements ValueExtractorInterface, CacheableDependencyInterface {
   
   +++ b/src/Normalizer/WebLinkNormalizer.php
   @@ -0,0 +1,40 @@
   +class WebLinkNormalizer extends NormalizerBase {
   

   Why do we even need this? Why can't we pass LinkCollection straight to JsonApiDocumentTopLevelNormalizerValue and let \Drupal\jsonapi\Normalizer\Value\JsonApiDocumentTopLevelNormalizerValue::rasterizeValue() handle this?

5. I expected — perhaps wrongly — \Drupal\jsonapi\LinkManager\LinkManager to become a lot simpler. Why is my expectation wrong?

In any case: 👏👏👏👏 for this first iteration, this is looking fantastic already, and more importantly: it looks promising :)

Sorry I didn't finish the review. But sleep needs caught up w/ me.

I'm liking what I see so far.

1. +++ b/src/Controller/EntityResource.php
   @@ -841,7 +843,7 @@ class EntityResource {
   -   * @param string[] $links
   +   * @param \Drupal\jsonapi\JsonApiResource\LinkCollection $links
   

   I just started and I like this already.

2. +++ b/src/JsonApiResource/Link.php
   @@ -0,0 +1,172 @@
   +   *   A list of valid RFC5988 link relation types.
   

   Valid in what sense? Meaning already registered? If not, we should give an example of a custom relation type.

3. +++ b/src/JsonApiResource/Link.php
   @@ -0,0 +1,172 @@
   +    assert(static::compare($a, $b) === 0);
   

   Should this be enforced outside of the asserts? Does it make sense to merge 2 links with a different href?

#22.4.1: can't this jsonapi_hypermedia module alter links by taking a \Drupal\jsonapi\JsonApiResource\JsonApiDocumentTopLevel object and creating a new one, with a different set of links?

The perspective I'm coming from:

• I'd rather not introduce more normalizers
• if we have Link value objects and JsonApiDocumentTopLevel is another value object that can contain Links, then in principle we shouldn't need much else
• the code I quoted in #21.4 at least seemed to be doing much else — I admittedly did not yet do a deep dive to grok every nuance of this patch

1. +++ b/jsonapi.services.yml
   @@ -84,6 +84,14 @@ services:
   +    class: Drupal\jsonapi\Normalizer\LinkCollectionNormalizer
   ...
   +    class: Drupal\jsonapi\Normalizer\LinkNormalizer
   

   These are now consistent 👍

2. +++ b/src/Controller/EntityResource.php
   @@ -849,8 +851,9 @@ class EntityResource {
   +    $self_link = new Link(new CacheableMetadata(), $this->linkManager->getRequestUrl($request), ['self']);
   +    $links = ($links ?: new LinkCollection([]))->withLink('self', $self_link);
   
   +++ b/src/LinkManager/LinkManager.php
   @@ -121,22 +124,26 @@ class LinkManager {
   +      $pager_links = $pager_links->withLink('next', new Link(new CacheableMetadata(), $next_url, ['next']));
   ...
   +        $pager_links = $pager_links->withLink('last', new Link(new CacheableMetadata(), $last_url, ['last']));
   ...
   +      $pager_links = $pager_links->withLink('first', new Link(new CacheableMetadata(), $first_url, ['first']));
   ...
   +      $pager_links = $pager_links->withLink('prev', new Link(new CacheableMetadata(), $prev_url, ['prev']));
   

   'self' is in here twice, once in the Link invocation, once in the withLink(…) invocation.

   Same for the other code hunks I cited.

3. +++ b/src/Controller/EntryPoint.php
   @@ -83,32 +72,22 @@ class EntryPoint extends ControllerBase {
   +    $urls = array_reduce($resources, function (LinkCollection $carry, ResourceType $resource_type) {
   

   Nit: s/$urls/$links/

   (Fixed!)

4. +++ b/src/JsonApiResource/JsonApiDocumentTopLevel.php
   @@ -52,18 +50,13 @@ class JsonApiDocumentTopLevel {
   -  public function __construct($data, EntityCollection $includes, array $links, array $meta = []) {
   +  public function __construct($data, EntityCollection $includes, LinkCollection $links, array $meta = []) {
        assert(!$data instanceof ErrorCollection || $includes instanceof NullEntityCollection);
   -    assert(Inspector::assertAll(function ($link) {
   -      return is_array($link) || isset($link['href']) && is_string($link['href']);
   -    }, $links));
   

   🎉

5. +++ b/src/JsonApiResource/Link.php
   @@ -0,0 +1,174 @@
   +   * JSON:API Link constructor.
   
   +++ b/src/JsonApiResource/LinkCollection.php
   @@ -0,0 +1,134 @@
   +   * LinkCollection constructor.
   

   Nit: inconsistent.

   (Fixed!)

6. +++ b/src/JsonApiResource/Link.php
   @@ -0,0 +1,174 @@
   +  public function __construct(CacheableMetadata $cacheability, Url $url, array $link_relation_types, array $target_attributes = []) {
   
   +++ b/src/JsonApiResource/LinkCollection.php
   @@ -0,0 +1,134 @@
   +  public function __construct(array $links) {
   

   👌 These both look excellent!

7. +++ b/src/JsonApiResource/Link.php
   @@ -0,0 +1,174 @@
   +   * Compares two links by their href.
   

   I'm missing some documentation explaining why this is sufficient; why is there no need to include target attributes in the comparison?

   EDIT: oh, only later I realized that this is not intended as a callback for sorting. Never mind, this is fine!

8. +++ b/src/JsonApiResource/Link.php
   @@ -0,0 +1,174 @@
   +  public static function merge(Link $a, Link $b) {
   

   Needs test coverage?

9. +++ b/src/JsonApiResource/LinkCollection.php
   @@ -0,0 +1,134 @@
   +  protected static function validKey($key) {
   +    return is_string($key) && !is_numeric($key) && strpos($key, ':') === FALSE;
   +  }
   

   Where are these restrictions defined? Are we imposing them? Or do they originate from RFC8288?

10. +++ b/src/LinkManager/LinkManager.php
    @@ -73,16 +75,16 @@ class LinkManager {
    -   * @return string
    +   * @return \Drupal\Core\Url
    ...
    -      return $request->getUri();
    +      return Url::fromUri($request->getUri());
    

    👌 This is completing the work I started doing a long time ago, yay!

11. +++ b/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
    @@ -79,7 +78,7 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
    -   * @param string[] $links
    +   * @param \Drupal\jsonapi\Normalizer\Value\NormalizedValue $links
    

    I still find this the most confusing thing about this patch. I already brought it up in #21.4. I don't want to rehash it. I'm wondering if we can hold off on introducing this NormalizedValue class until a future issue where there would be multiple things using it, instead of this single one?

12. +++ b/src/Normalizer/LinkNormalizer.php
    @@ -0,0 +1,40 @@
    +  public function normalize($object, $format = NULL, array $context = []) {
    

    I could imagine this just living in JsonApiDocumentTopLevelNormalizerVale?

    That'd avoid creating a new normalizer, because we've been trying to move away from those.

13. +++ b/src/Normalizer/Value/NormalizedValue.php
    @@ -0,0 +1,51 @@
    +  public function rasterizeIncludes() {
    +    return [];
    +  }
    

    Can be removed :)

    (Done!)

14. +++ b/tests/src/Functional/EntryPointTest.php
    @@ -0,0 +1,93 @@
    +    $response = $this->request('GET', Url::fromUri('base://jsonapi'), [RequestOptions::HEADERS => ['Accept' => 'application/vnd.api+json']]);
    

    Nit base:// should be base:/.

    (Fixed!)

15. +++ b/tests/src/Functional/EntryPointTest.php
    @@ -0,0 +1,93 @@
    +  /**
    +   * Copied from \Drupal\Tests\jsonapi\Functional\ResourceTestBase.
    +   *
    +   * @see \Drupal\Tests\jsonapi\Functional\ResourceTestBase::request()
    +   */
    +  protected function request($method, Url $url, array $request_options) {
    ...
    +  /**
    +   * Copied from \Drupal\Tests\jsonapi\Functional\ResourceTestBase.
    +   *
    +   * @see \Drupal\Tests\jsonapi\Functional\ResourceTestBase::decorateWithXdebugCookie()
    +   */
    +  protected function decorateWithXdebugCookie(array $request_options) {
    

    Can we instead move these two methods to a trait that both this test and ResourceTestBase use?

@gabesullice just pinged me, pointing out that wrt points 11+12, https://cgit.drupalcode.org/jsonapi_hypermedia/commit/?id=a4477cf5af177c... is going to be helpful in understanding this approach. I'll need to take another look.

I TOTALLY FORGOT ABOUT THIS I AM SO SORRY WILL REVIEW TOMORROW 😅

I did take another look. I looked at https://cgit.drupalcode.org/jsonapi_hypermedia/commit/?id=a4477cf5af177c... and at JsonApiHypermediaLinkCollectionNormalizer in particular.

I can now see how having the normalizer for the link collection being separate is valuable; because it allows JsonApiHypermediaLinkCollectionNormalizer to do its thing, despite it being a private API, and jsonapi_hypermedia accepting the consequences of it hooking into a private API. Cool. 👍

But that in turn raised more questions 😅 🤓 Sorry 😃

I know jsonapi_hypermedia is just a PoC right now. But JsonApiHypermediaLinkCollectionNormalizer indicates — if I am understanding it correctly — that the only information that a module would be given is a LinkCollection value object. You can then return a new LinkCollection instance: one with additional links. Makes sense, but AFAICT the tricky thing then is: how does a hypermedia link providing module then determine which link collections it wants to add links to? There's no context whatsoever about which resource you're being offered the opportunity to generate additional links for. Well, there's the link_object_key context, but that's just a hashed href.

Perhaps the best way to move this forward is to have a joint call with all maintainers so you can explain this in more detail? I may very well be the only one for who it doesn't "click" how this would work.

Context

I added a context property to LinkCollection. This is conceptually sound even absent of any idea about normalization since links have link contexts.

Good call.

But that RFC specifically says: the context of a link conveyed in the Link header field is the URL of the representation it is associated with. Whereas this patch is passing a JsonApiDocumentTopLevel object as the context.

To be fair, I omitted the two words preceding that quote: by default. So it's probably fair to pass something else. Still, I wonder if it wouldn't be more appropriate to pass the associated JSON:API URL. Or at least just a \Drupal\jsonapi\JsonApiResource\ResourceIdentifier object rather than a completely assembled response document. That'd also prevent crazy complex link additions in https://www.drupal.org/project/jsonapi_hypermedia (i.e. based on some obscure detail in \Drupal\jsonapi\JsonApiResource\JsonApiDocumentTopLevel. (I know you're alluding to this with your Relationship and ResourceObject mentions, but my concern is different: it's not about adding those, it's about providing JsonApiDocumentTopLevel at all.)

Still, I can't help but wonder why it would be insufficient for JsonApiHypermediaLinkCollectionNormalizer to inspect the existing self link and based on what that contains (e.g. which resource type it links to) add some additional links.
I suppose the primary reason for not doing that would be that the whole point is that links are state-dependent. Having merely a URL as context does not allow easy resource state introspection, meaning that you'd need to load the resource (entity) associated with that URL to even figure out which links to add. Being able to access the JsonApiDocumentTopLevel object's data removes the need for loading that resource.
But what if JsonApiDocumentTopLevel has a bunch of sparse fieldsets defined? That might mean you still will not be able to introspect the necessary data. Which actually leads to another question: does it make sense to add all potential links when sparsely retrieving a resource? For example, does a enable link make sense when the status field is omitted? Or does it make sense when you're filtering a collection down to only the enabled resources?

Per-resource LinkCollection instead of a single top-level LinkCollection

In the current patch, it's only possible to customize the links for the primary data. But what about includes? For example, a blog post with included tags, or a cart with included cart items. We may want to perform operations on those included resources via hypermedia links.

It would be a confusing experience to see those hypermedia links only when accessing the individual resources.

Actually, we don't even need to look at the includes case; there's a simpler example: collections. I don't think we want only to be able to have hypermedia links for the overall collection, not for the resources in that collection?

Consequences for partial caching

In #2819335: Resource (entity) normalization should use partial caching, we'll want to add partial caching. We need to make sure that the links added by jsonapi_hypermedia to a specific resource are dependent only on that particular resource; then they will get updated (invalidated) correctly: in sync with the resource itself. But if those links start depending on state of other resources, then jsonapi_hypermedia would need to allow to associate appropriate cacheability. If we're confident this is unneeded, then this should be explicitly documented.

Concrete examples

https://cgit.drupalcode.org/jsonapi_hypermedia/log/ does not list any commits to leverage #37. Seeing an example there of these various use cases would be helpful!

Aside: The links key in the @EntityType annotation.

This is a relative aside because this is less about the infrastructure that this issue is adding, but more about the jsonapi_hypermedia module's implementation. I'm wondering if instead of conjuring arbitrary links, it may be desirable to add additional links to the links annotation of entity types, and have those be state-dependent (if the state means the link does not make sense, that link would render to NULL). I'm well aware that this would require infrastructure changes in Drupal core. So what if we'd call this jsonapi_hypermedia_links initially (or even hypermedia_links) and then let jsonapi_hypermedia validate this, and if proven, it could be a non-breaking new feature in Drupal core.

Related: #2960766-25: Support ResourceType which may have a null bundle defined..

By leaving those lower levels out of the patch, I was trying to keep this patch smaller, but perhaps it's too confusing to not see the whole scope of things in one place? ... I think the answer is yes, its confusing, because that's what the whole next section of your response is all about!

Yep :)

It'd have helped if this was explicitly called out in the title/issue summary. Because the issue summary even explicitly says they may exist on 4 levels.

I definitely didn't realize that/skipped over tat detail when I read #30 some time ago. (My bad!)

it then brings in the complexity of of cross-compatibility with REST module

Core REST does not introspect the links annotation, and certainly not some new annotation key.

#3022584: Consolidate and simplify NormalizerValue objects: introduce CacheableNormalization landed!

Yep!

1. +++ b/src/Controller/EntryPoint.php
   @@ -84,32 +73,22 @@ class EntryPoint extends ControllerBase {
   +        $url = Url::fromRoute(sprintf('jsonapi.%s.%s', $resource_type->getTypeName(), $route_suffix))->setAbsolute();
   
   +++ b/src/JsonApiResource/Link.php
   @@ -0,0 +1,174 @@
   +    $generated_url = $url->setAbsolute()->toString(TRUE);
   

   ✅ Nit: shouldn't the setAbsolute() happen just before normalizing/rendering? Or in Link? We don't want to depend on every piece of code setting this flag.

   EDIT: hah, this is already happening!

2. +++ b/src/JsonApiResource/Link.php
   @@ -0,0 +1,174 @@
   +namespace Drupal\jsonapi\JsonApiResource;
   
   +++ b/src/JsonApiResource/LinkCollection.php
   @@ -0,0 +1,175 @@
   +namespace Drupal\jsonapi\JsonApiResource;
   

   🤔 Übernit: is this the right namespace?

3. +++ b/src/JsonApiResource/Link.php
   @@ -0,0 +1,174 @@
   +class Link implements CacheableDependencyInterface {
   
   +++ b/src/JsonApiResource/LinkCollection.php
   @@ -0,0 +1,175 @@
   +class LinkCollection implements \IteratorAggregate {
   

   ✅ I think this should be final, at least for now. We can choose to open it up for extension later. Done.

4. +++ b/src/JsonApiResource/LinkCollection.php
   @@ -0,0 +1,175 @@
   +   * @var array
   

   ✅ Übernit: could be more specific. Fixed.

5. +++ b/src/JsonApiResource/LinkCollection.php
   @@ -0,0 +1,175 @@
   +   * The link context.
   +   *
   +   * All links objects exist within a context object. Links form a relationship
   +   * between a source IRI and target IRI. A context is the link's source.
   

   ✅ Nit: should link to https://tools.ietf.org/html/rfc8288#section-3.2. Done.

6. +++ b/src/LinkManager/LinkManager.php
   @@ -73,16 +75,16 @@ class LinkManager {
   -  public function getRequestLink(Request $request, $query = NULL) {
   +  public function getRequestUrl(Request $request, $query = NULL) {
   

   🤔 This rename will break \Drupal\Tests\jsonapi_extras\Kernel\EntityToJsonApiTest.

   You're right that this is returning a URL and not a link. But that was already the case.

   Furthermore, I think with the foundation that this patch is laying, it will probably make more sense to refactor LinkManager away completely? I think that will mean one disruption instead of this tiny disruption with very little gain.

7. 🤔 ✅ Related to previous point: I was shocked to see that \Drupal\jsonapi\LinkManager\LinkManager is currently only marked @deprecated and not yet @internal. Let's do that here/now? That is in scope IMHO, because this is laying the foundation for the future, and is hence definitively making LinkManager a "do not use this" thing. It was marked @deprecated just over a year ago, in #2933343: Define, document and mark scope of PHP and HTTP API. We've come a very long way since then. I think we now can mark it @internal without worry.
   EDIT: what confirms this impression definitively is @e0ipso's remark at #2933343-13: Define, document and mark scope of PHP and HTTP API:

   @gabesullice as noted in #2857730-10: Add a link to get only published entities in collections I think that true HATEOAS needs to populate links from within the application (it's not possible to do it from the module itself).

   The link manager is the service the actual sites will use to create those links properly.

   That's exactly what this issue is working on :)

   Oh and further down, at #2933343-17: Define, document and mark scope of PHP and HTTP API, @e0ipso wrote:

   Yeah, @gabesullice is right about LinkManager. I was speaking about what I hoped it had become through iterations and support requests that never happened. That indicates that people don't care about links or HATEOAS, which makes me a bit sad. But it's a reality I embrace by making it an internal API.

   That is finally happening here and now!

   Given all that, I went ahead and marked it @internal.

8. +++ b/src/Normalizer/LinkCollectionNormalizer.php
   @@ -0,0 +1,96 @@
   + * The JSON API specification has the concept of a "links collection". A links
   

   ✅ Übernit: s/JSON API/JSON:API/. Fixed.

9. +++ b/src/Normalizer/LinkCollectionNormalizer.php
   @@ -0,0 +1,96 @@
   +   * This string is unique per instance of a link collection, but always the
   +   * same within it. This means that link key hashes will be non-deterministic
   +   * for outside observers, but two links within the same collection will always
   +   * have the same hash value.
   

   👌

10. +++ b/src/Normalizer/LinkCollectionNormalizer.php
    @@ -0,0 +1,96 @@
    +        $normalized[$link_key] = new CacheableNormalization($link, $normalization);
    +      }
    +    }
    +    return CacheableNormalization::aggregate($normalized);
    

    👌

11. +++ b/tests/src/Functional/EntryPointTest.php
    @@ -0,0 +1,90 @@
    +  protected static $modules = [
    +    'node',
    +    'jsonapi',
    +    'serialization',
    +    'system',
    +    'user',
    +  ];
    

    ✅ Nit: We don't list every dependency explicitly in functional tests. serialization is installed automatically when installing jsonapi, for example.

    EDIT: oh, that's just a leftover from converting this from a KernelTestBase. Cool, then this is the slight simplification we get for free.

12. +++ b/tests/src/Functional/EntryPointTest.php
    @@ -0,0 +1,90 @@
    +   * Copied from \Drupal\Tests\jsonapi\Functional\ResourceTestBase.
    ...
    +  protected function request($method, Url $url, array $request_options) {
    ...
    +   * Copied from \Drupal\Tests\jsonapi\Functional\ResourceTestBase.
    ...
    +  protected function decorateWithXdebugCookie(array $request_options) {
    

    🤔 Probably makes sense to add a JsonapiRequestTestTrait and use that in both?

Points 2, 6 and 12 still need Gabe's input.

2. Ahh, of course!👌
3. 👍
4. 👍
5. 👍

@e0ipso was an explicit supporter of the overall plan in #2994193: [META] The Last Link: A Hypermedia Story and a long time ago in at #2933343-13: Define, document and mark scope of PHP and HTTP API and #2933343-17: Define, document and mark scope of PHP and HTTP API.

I think I remember from our last meeting that @e0ipso said he was happy with the state of this patch, but I'm not 100% certain. @e0ipso last commented on November 8, in #26, and I know that he's said before that if he doesn't comment for a long time, it usually means he's +1 :)

So, for the sake of progress, let's do this — I'm happy to take the blame and revert if you still had concerns, @e0ipso :)

This unblocked #3002646: Follow-up to #2927037: Add test coverage for `meta.links.me` when a user is authenticated and #3014704: Expose API to allow links handling for entities from other modules.

Oh, this also unblocked #2992836: Provide links to resource versions (entity revisions)!

This unfortunately also caused a regression on PHP 5.6: #3027501: [regression] Follow-up for #2995960 and #2992833: syntax errors in PHP 5.5 & 5.6.