Links with different target attributes are improperly merged

The examples in the issue summary are crystal clear. They demonstrate in an obvious way what the problem is. I would even say painfully obvious 😅

This made me wonder why we didn't catch this originally. I think we probably didn't originally spot that because it was all still theoretical. We didn't have any concrete examples to look at. Now that @gabesullice has pushed the https://www.drupal.org/project/jsonapi_hypermedia forward towards stability, it now is easy to see how what we did originally is wrong.

1. +++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
   @@ -106,14 +117,32 @@ public function getHref() {
   +   * Gets the link's relation types.
   

   Nit: should be singular.

2. +++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
   @@ -135,16 +164,31 @@ public function getTargetAttributes() {
   +    // Any string concatenation would work, but a Link header-like format makes
   +    // it clear what is being compared.
   

   👍 — our future maintainer selves will be grateful for this 🙏

3. +++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
   @@ -135,16 +164,31 @@ public function getTargetAttributes() {
   +    // This ternary avoids unnecessarily encoding target attributes on non-
   +    // equivalent links.
   +    $json_cmp = strcmp(Json::encode($a->getTargetAttributes()), Json::encode($b->getTargetAttributes()));
   

   🤔 I want to check my own understanding here: this comment and this comparison are happening after the string concatenation in the previous few lines, because there are potentially many target attributes, and if the link href + rel aren't the same already, there's no point in doing so.

   Is that a correct understanding?

   But … this code runs always anyway, so what is the real savings here?

   This seems like a premature optimization … and one that accidentally was ineffective too? 😅

4. +++ b/core/modules/jsonapi/tests/src/Unit/JsonApiResource/LinkTest.php
   @@ -19,67 +19,99 @@
   +  public function linkMergeProvider() {
   

   🤔 I think it would be valuable to include the concrete examples from the issue summary in the test cases.

Looks much better! I found two remaining concerns:

1. +++ b/core/modules/jsonapi/tests/src/Unit/JsonApiResource/LinkTest.php
   @@ -18,68 +18,115 @@
   +        \LogicException::class,
   

   Expecting a particular exception class is fine, if it's meaningful. But \LogicException is super generic.

   So if we add more logic exceptions (with other messages) in the future, this test coverage will continue to pass.

   I think this should be changed to new \LogicException('expected message here').

2. I thought the concrete input + output examples from the issue summary would be good to add. They're very clear. I understand that the test coverage you added indirectly proves the same, but I think it'd help future maintenance if we added the concrete examples from the issue summary too.

🤩 — love the musical reference! 🎸

1. +++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
   @@ -106,14 +117,32 @@ public function getHref() {
   +    // @todo: uncomment the following line and remove the one after it in https://www.drupal.org/project/drupal/issues/3080467.
   +    /*return $this->rel;*/
   

   I don't think we should be adding commented out code. The @todo seems fine.

2. +++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
   @@ -135,16 +164,32 @@ public function getTargetAttributes() {
   +    // Sort the link relation type array so that order does not matter during
   +    // string comparison.
   +    sort($a->rel);
   +    sort($b->rel);
   

   Does this mean that comparing two links actually changes them? That's a bit unexpected.

3. +++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
   @@ -135,16 +164,32 @@ public function getTargetAttributes() {
   +      ? strcmp(Json::encode($a->getTargetAttributes()), Json::encode($b->getTargetAttributes()))
   

   Why are we doing a Json::encode() in order to a strcmp. This looks super odd. It's not as if this function is being used for sorting.

4. +++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
   @@ -152,15 +197,14 @@ public static function compare(Link $a, Link $b) {
   -    assert(static::compare($a, $b) === 0);
   ...
   +    if (static::compare($a, $b) !== 0) {
   +      throw new \LogicException('Only equivalent links can be merged.');
   +    }
   

   The change from an assert to an exception is interesting. We should use @throws to document this. Ah here's why I think this is a bit odd here's the only runtime usage...

           if (Link::compare($existing_link, $new_link) === 0) {
             $merged[$key][$index] = Link::merge($existing_link, $new_link);
             return new static($merged, $this->context);
           }
   

   So here now it's the caller doing the call to ::compare first. With this change we're always going to call compare again.

1. +++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
   @@ -3,7 +3,7 @@
   +use Drupal\Component\Utility\DiffArray;
   
   @@ -168,21 +170,17 @@ public function getTargetAttributes() {
   +      ? (int) !empty(DiffArray::diffAssocRecursive($a->getTargetAttributes(), $b->getTargetAttributes()))
   

   😲😲😲 I wish I'd known about the existence of this! Would've saved me a lot of time a few times in the past.

   To make sense of what this does, I read the test at \Drupal\Tests\Core\Common\DiffArrayTest.

   This logic looks correct to me :)

2. +++ b/core/modules/jsonapi/tests/src/Unit/JsonApiResource/LinkTest.php
   +++ b/core/modules/jsonapi/tests/src/Unit/JsonApiResource/LinkTest.php
   @@ -58,19 +58,28 @@ public function linkComparisonProvider() {
   
   @@ -58,19 +58,28 @@ public function linkComparisonProvider() {
   +      // These links can't be merged because it would while the `href` remains
   ...
   +      // These links can't be merged because it would be unclear which title
   

   🤓 "merged" is now out of place here, because these test cases were moved from the "merging" test coverage to the "comparing" test coverage.

+++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
@@ -135,16 +166,28 @@ public function getTargetAttributes() {
+    // Any string concatenation would work, but a Link header-like format makes
+    // it clear what is being compared.
+    $a_string = sprintf('<%s>;rel="%s"', $a->getHref(), implode(' ', (array) $a->getLinkRelationType()));
+    $b_string = sprintf('<%s>;rel="%s"', $b->getHref(), implode(' ', (array) $b->getLinkRelationType()));
+    $cmp = strcmp($a_string, $b_string);
...
+    // equivalent links since there is no point in checking whether a link's
+    // target attributes are equivalent if the target hrefs or link relation
+    // types are different.
+    return $cmp === 0
+      ? (int) !empty(DiffArray::diffAssocRecursive($a->getTargetAttributes(), $b->getTargetAttributes()))
+      : $cmp;
   }
 

Now that the json encoding is gone we're not unnecessary encoding.

I think this is more readable in a non ternary form.

I.e

    if ($cmp === 0) {
      // If there is no difference in the href and link relation than check the
      // target attributes.
      $cmp = (int) !empty(DiffArray::diffAssocRecursive($a->getTargetAttributes(), $b->getTargetAttributes()));
    }
    return $cmp;

If you have to explain a ternary then perhaps it's not worth using it.

1. +++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
   @@ -41,6 +42,8 @@
   +   *
   +   * @todo: change this type documentation to be a single string in https://www.drupal.org/project/drupal/issues/3080467.
   

   The comment is too long > 80 chars.

2. +++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
   @@ -64,16 +67,24 @@
   +    // @todo Remove this conditional block in drupal:9.0.0 and add a type hint to the $link_relation_type argument of this method in https://www.drupal.org/project/drupal/issues/3080467.
   +    // @see https://www.drupal.org/project/drupal/issues/3080259
   +    if (is_array($link_relation_type)) {
   +      @trigger_error('Constructing a ' . self::class . ' with an array of link relation types is deprecated in drupal:8.8.0 and will throw a fatal error in drupal:9.0.0. Pass a single string instead. See https://www.drupal.org/project/drupal/issues/3080259.', E_USER_DEPRECATED);
   +    }
   +    else {
   +      $link_relation_type = [$link_relation_type];
   +    }
        // @todo: uncomment the extra assertion below when JSON:API begins to use its own extension relation types.
   -    assert(/* !empty($link_relation_types) && */Inspector::assertAllStrings($link_relation_types));
   +    assert(/* !empty($link_relation_types) && */Inspector::assertAllStrings($link_relation_type));
   

   This assert can move into the is_array part of the if.

   We can assert that's its a string in the other.

   Also the @todo can be removed because removing the deprecated path is going solve it.

3. +++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
   @@ -106,14 +117,34 @@ public function getHref() {
   +    @trigger_error(self::class . '::' . __METHOD__ . ' is deprecated in drupal:8.8.0 and will be removed in drupal:9.0.0. Use ' . self::class . '::' . 'getLinkRelationType' . ' instead. See https://www.drupal.org/project/drupal/issues/3080259.', E_USER_DEPRECATED);
   

   __METHOD__ includes the class - see https://3v4l.org/16jeq. Plus there is no deprecation testing which might have revealed this.

+++ b/core/modules/jsonapi/tests/src/Unit/JsonApiResource/LinkTest.php
@@ -118,6 +118,19 @@ public function linkMergeProvider() {
+   * Tests the getLinkRelationTypes() is properly deprecated.

Übernit: s/is properly/method is properly/. Alternatively, this line could just be deleted.

Can be fixed on commit.

1. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -565,7 +565,7 @@ public function getRelationship(ResourceType $resource_type, FieldableEntityInte
        $response = $this->buildWrappedResponse($field_list, $request, $this->getIncludes($request, $resource_object), $response_code, [], array_reduce(array_keys($relationship_object_urls), function (LinkCollection $links, $key) use ($relationship_object_urls) {
   -      return $links->withLink($key, new Link(new CacheableMetadata(), $relationship_object_urls[$key], [$key]));
   +      return $links->withLink($key, new Link(new CacheableMetadata(), $relationship_object_urls[$key], $key));
        }, new LinkCollection([])));
   

   This bit conflicted with HEAD - this code appears to have been removed in #3036285: Add a \JsonApiResource\Relationship object to carry relationship data, metadata and a link collection.

2. +++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
   @@ -106,14 +118,34 @@ public function getHref() {
   +  /**
   +   * Gets the link's relation type.
   +   *
   +   * @return string|string[]
   +   *   The link's relation type. For BC reasons, this method might return an
   +   *   array of strings.
   +   *
   +   * @todo: update the return type docs to only have a single string in https://www.drupal.org/project/drupal/issues/3080467.
   +   */
   +  public function getLinkRelationType() {
   +    // Sort the link relation type array so that the order of link relation
   +    // types does not matter during link comparison.
   +    $rel = $this->rel;
   +    sort($rel);
   +    return count($this->rel) > 1 ? $rel : reset($rel);
   +  }
   
   @@ -135,16 +167,27 @@ public function getTargetAttributes() {
   +    $a_string = sprintf('<%s>;rel="%s"', $a->getHref(), implode(' ', (array) $a->getLinkRelationType()));
   +    $b_string = sprintf('<%s>;rel="%s"', $b->getHref(), implode(' ', (array) $b->getLinkRelationType()));
   

   We can access protected properties in static methods so there's no need to do the sort in the etc in the getLinkRelationType() method - which in turn means that I think we can make that method return the string we expect it to in the future right now. Which means that we have less BC to remove and more consistenct from the get go. Also the new method has no explicit test coverage.

3. +++ b/core/modules/jsonapi/tests/src/Unit/JsonApiResource/LinkTest.php
   @@ -19,67 +19,116 @@
   +  /**
   +   * Tests the getLinkRelationTypes() is properly deprecated.
   +   *
   +   * @group legacy
   +   *
   +   * @expectedDeprecation Drupal\jsonapi\JsonApiResource\Link::getLinkRelationTypes() is deprecated in drupal:8.8.0 and will be removed in drupal:9.0.0. Use getLinkRelationType() instead. See https://www.drupal.org/project/drupal/issues/3080259.
   +   */
   +  public function testGetLinkRelationTypes() {
        $this->mockUrlAssembler();
   -    return array_map(function ($arguments) {
   -      return array_map(function ($attributes) {
   -        return new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/'), ['item'], $attributes);
   -      }, $arguments);
   -    }, $cases);
   +    $link = new Link((new CacheableMetadata())->addCacheTags(['foo']), Url::fromUri('https://jsonapi.org/foo'), 'self');
   +    $link->getLinkRelationTypes();
      }
   

   This test can test the constructor deprecation too and be more explicit (make more assertions) about the BC layer.

4. Also fixed ##2

Attached patch resolves all of this.

Hopefully #30 fails because of deprecated code paths... It looks like #3036285: Add a \JsonApiResource\Relationship object to carry relationship data, metadata and a link collection. added some usages of constructing a Link object with an array of relationships.

#30.2: I wish I had seen that! 👍🤓

+++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
@@ -46,7 +46,7 @@ final class Link implements CacheableDependencyInterface {
-  protected $rel;
+  protected $rel = [];

I'm a tiny bit concerned about this because it makes it ever so slightly more difficult to move it from string[] to string. I think it was added to slightly simplify getLinkRelationTypes() (removal of the cast).

Oops. I think that's unnecessary - it was necessary when I was halfway through coding my thoughts on #30 but then it became unnecessary.

Also noticed some now incorrect docs.

+++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
@@ -64,16 +68,24 @@ final class Link implements CacheableDependencyInterface {
+      @trigger_error('Constructing a ' . self::class . ' with an array of link relation types is deprecated in drupal:8.8.0 and will throw a fatal error in drupal:9.0.0. Pass a single string instead. See https://www.drupal.org/project/drupal/issues/3080259.', E_USER_DEPRECATED);

I missed that this was pointing to this issue. This needs to point to a change record.

+++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
@@ -127,7 +151,7 @@ public function getTargetAttributes() {
-   * Compares two links by their href.
+   * Compares two links.
   public static function compare(Link $a, Link $b) {
@@ -135,16 +159,35 @@ public function getTargetAttributes() {
-   * Merges two link objects' relation types and target attributes.
+   * Merges two equivalent links into one link with the merged cacheability.
-   * The links must share the same URI.
+   * The links must share the same URI, link relation type and attributes.
   public static function merge(Link $a, Link $b) {

I think we need a 2nd CR to cover the above changes. Unless it makes sense to instead broaden https://www.drupal.org/node/3087821 to cover that, but seems like a separate thing to me. Either way though, Needs work for that.

I'm also wondering whether we're at all concerned about the BC break of callers expecting prior behavior of compare() or merge(). For example, callers of merge() will now get an assertion error where they didn't used to. In the case of equivalent hrefs but differing relation or attributes, should we change that assertion to a deprecation error and return a link with the merged relations? Or is there a compelling reason to justify the BC break?

+++ b/core/modules/jsonapi/src/JsonApiResource/Link.php
@@ -152,15 +195,12 @@ public static function compare(Link $a, Link $b) {
   public static function merge(Link $a, Link $b) {
-    assert(static::compare($a, $b) === 0);
-    $merged_rels = array_unique(array_merge($a->getLinkRelationTypes(), $b->getLinkRelationTypes()));
-    $merged_attributes = array_merge_recursive($a->getTargetAttributes(), $b->getTargetAttributes());
+    assert(static::compare($a, $b) === 0, 'Only equivalent links can be merged.');
     $merged_cacheability = (new CacheableMetadata())->addCacheableDependency($a)->addCacheableDependency($b);
-    return new static($merged_cacheability, $a->getUri(), $merged_rels, $merged_attributes);
+    return new static($merged_cacheability, $a->getUri(), $a->getLinkRelationType(), $a->getTargetAttributes());
   }

Callers to merge previously got an assertion error. Prior to this change callers had to ensure they were merging links objects that ::compare returns 0 for. That behaviour is unchanged.

Thanks for the CR. The rationale in there and in #43 for not including a BC layer for merge() works for me. Back to RTBC.

Adding reviewer credit.

Pushed to 9.0.x and 8.9.x.

Leaving RTBC for 8.8.x. We're currently in code freeze for alpha, but I'm +1 for committing this to 8.8.x once it's unfrozen (between alpha and beta). Leaving the CRs unpublished until that's done.

Pushed to 8.8.x and published the CRs.