Problem/Motivation

According to RFC8288, a link is:

a typed connection between two resources and is comprised of:

  • a link context,
  • a link relation type (Section 2.1),
  • a link target, and
  • optionally, target attributes (Section 2.2).

When LinkCollection::merge merges two Link objects with the same key and target URI, it does not consider target attributes as a differentiating factor.

An example where this can break link semantics in via the anchor target attribute. Typically, a "link context" is just the resource on which a link appears, however, a link context can be overridden by the anchor target attribute. When JSON:API merges links, it only considers the links object key and the target URI to determine if a link is equivalent. In the case that one link defines an anchor and the other does not, the semantics of the link without the anchor will be broken. Consider the following example (appearing on a resource /bar):

1. {"href": "/foo", "anchor": "/foo/alias", "rel": ["canonical"]}
2. {"href": "/foo", "rel": ["related"]}

These JSON objects are simplified. Real JSON:API link objects look different.

would be merged into:

3. {"href": "/foo", "rel": ["related", "canonical"], "anchor": "/foo/alias"}

Another example where this can be an issue is when two links share a common URI but different link relation types and target attributes:

1. {"href": "/foo", "rel": ["comments"], "title": "Comments on Foo"}
2. {"href": "/foo", "rel": ["add-comment"], "title": "Post a comment about Foo"}

would be merged into:

3. {"href": "/foo", "rel": ["comments", "add-comment"], "title": ["Comments on Foo", "Post a comment about Foo"]}

After that merge, it's impossible to know which of the two links represented by the merged object has which title. The semantics of the link object are pretty much nonsensical.

For background, the reason that Drupal\jsonapi\JsonApiResource\Link support more than one link relation type and merging of this kind is because RFC 8288 permits a Link header to have more than one link relationship type and, apparently, I misread the RFC (mea culpa). In the following paragraph, I saw the first sentence, but didn't internalize the second sentence:

The rel parameter can, however, contain multiple link relation types. When this occurs, it establishes multiple links that share the same context, target, and target attributes. — Section 3.3 (emphasis added)

Proposed resolution

  1. Deprecate passing an array of link relation types to Link::__construct in favor of a single string
  2. Deprecate Link::getLinkRelationTypes (plural) in favor of Link::getLinkRelationType
  3. Update Link::compare to only treat equivalent links as equal (it currently ignores target attributes)
  4. Update Link::merge to only merge cacheability, since links with different relation types or attributes can no longer be merged

User interface changes

None.

API changes

Link::__construct will have a deprecated argument type. Note: Drupal\jsonapi\JsonApiResource\Link is an @internal value object and no JSON:API code actually uses multiple link relation types or target attributes.

It's also important to note that there is currently no public API for adding custom links to JSON:API responses.

Data model changes

None.

Release notes snippet

None.

CommentFileSizeAuthor
#40 3080259-40.patch26.83 KBgabesullice
#40 interdiff.txt3.42 KBgabesullice
#36 3080259-36.patch27.08 KBalexpott
#36 35-36-interdiff.txt911 bytesalexpott
#35 3080259-35.patch26.19 KBalexpott
#35 31-35-interdiff.txt564 bytesalexpott
#31 3080259-31.patch26.25 KBalexpott
#31 30-31-interdiff.txt1.49 KBalexpott
#30 3080259-30.patch24.76 KBalexpott
#30 28-30-interdiff.txt4.86 KBalexpott
#28 3080259-28.patch24.62 KBgabesullice
#28 interdiff.txt3.43 KBgabesullice
#25 3080259-25.patch24.06 KBgabesullice
#25 interdiff.txt733 bytesgabesullice
#24 3080259-24.patch24.06 KBgabesullice
#24 interdiff.txt3.22 KBgabesullice
#21 3080259-21.patch24.15 KBgabesullice
#21 interdiff.txt8.73 KBgabesullice
#18 3080259-18.patch25.55 KBgabesullice
#18 interdiff.txt3.62 KBgabesullice
#16 3080259-16.patch25.13 KBgabesullice
#16 interdiff.txt5.5 KBgabesullice
#13 3080259-13.patch22.59 KBgabesullice
#12 3080259-11--correct.patch22.6 KBgabesullice
#12 interdiff-9-11--correct.txt1.18 KBgabesullice
#11 3080259-11.patch22.6 KBgabesullice
#11 interdiff-9-11.txt1.18 KBgabesullice
#9 3080259-9.patch21.69 KBgabesullice
#9 interdiff-5-9.txt7.48 KBgabesullice
#5 3080259-5.patch14.21 KBgabesullice
#5 interdiff.txt2.64 KBgabesullice
#3 3080259-3.patch13.59 KBgabesullice
#3 3080259-3--tests-only.patch7.14 KBgabesullice
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gabesullice created an issue. See original summary.

gabesullice’s picture

Issue summary: View changes
gabesullice’s picture

gabesullice’s picture

Status: Active » Needs review

Let's see what the testbot thinks.

gabesullice’s picture

The last submitted patch, 3: 3080259-3--tests-only.patch, failed testing. View results

The last submitted patch, 3: 3080259-3.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 5: 3080259-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gabesullice’s picture

Status: Needs work » Needs review
FileSize
7.48 KB
21.69 KB

Looks like it's pretty much all just deprecation notices. Easy fix.

Status: Needs review » Needs work

The last submitted patch, 9: 3080259-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gabesullice’s picture

Status: Needs work » Needs review
FileSize
1.18 KB
22.6 KB

Looks like I missed one.

gabesullice’s picture

Gah, I generated that last interdiff and patch backwards. Correct ones attached.

gabesullice’s picture

(ノಠ益ಠ)ノ彡┻━┻

Status: Needs review » Needs work

The last submitted patch, 13: 3080259-13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

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.

gabesullice’s picture

Status: Needs work » Needs review
FileSize
5.5 KB
25.13 KB

1. Good catch.
2. :)
3. Gah, I'm sorry you read so much into that. That was not supposed to be there. I broke that variable assignment out onto its own line for debugging reasons. Your overall rationale is correct though, I'm sorry my comment made no sense next to that line.
4. Good idea, I added some more cases.

Wim Leers’s picture

Status: Needs review » Needs work

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.
gabesullice’s picture

Status: Needs work » Needs review
FileSize
3.62 KB
25.55 KB

1. Good call. Done.
2. I was trying not to use that example because comments and add-comment are made-up link relation types. I'm hoping that if we follow JSON:API Hypermedia's lead in core, we'll start validating those. Then this test would have to change. But, I see that it could be made a bit clearer. I came up with a coupled other examples and added some comments explaining why it'd be wrong to merge them.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

🤩 — love the musical reference! 🎸

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

gabesullice’s picture

Status: Needs work » Needs review
FileSize
8.73 KB
24.15 KB

1. Removed. This was a practice we got in the habit of doing in JSON:API contrib to make followups trivial.

2. That's correct, but I don't think it's consequential. Those sorts are action on a protected property on a final value object. It's an implementation detail. Besides, the order of link relation types shouldn't be significant:

Relation types SHOULD NOT infer any additional semantics based upon the presence or absence of another link relation type, or its own cardinality of occurrence.

Regardless, I changed the implementation slightly at that should eliminate this as a potential sticking point.

3. The idea was to use Json::encode as a way to compare a multidimensional array without writing some recursive function. You're right that this isn't actually being used for sorting, though I think it was used for sorting in some earlier iteration of this code (that was never committed). I think it's important to preserve the ability to use this method for sorting links since compare is currently suitable for that purpose. However, instead of encoding, I looked around and found DiffArray::diffAssocRecursive which should achieve the same thing. Maybe that's preferable and more readable.

4. I changed from an assert to a LogicException to make the validation testable. After you pointed this out, I thought more about it and that desire was misplaced. assert verifies a contract with a method caller (that only equivalent links may be merged). By testing for a LogicException I'm just ensuring that the contract can't be violated. But that's exactly what the assert is designed to do in the first place. Instead, it's better to test the validation that's running within the assert (Link::compare) so that I know that the assert will pass or fail just as I expect it to... I updated the test coverage accordingly and changed the LogicException back to an assert. Thanks for calling this out and forcing me to think about it more critically!

PS. I noticed some changes in the #18 patch that weren't in the interdiff so I reverted those in this interdiff along with the other changes. (for some reason there were whole lines that were lowercased in #18)

Wim Leers’s picture

  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.

alexpott’s picture

+++ 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.

gabesullice’s picture

Addressed #22 and #23.

gabesullice’s picture

Lol, literally forgot a semi-colon.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

gabesullice’s picture

Status: Needs work » Needs review
FileSize
3.43 KB
24.62 KB

Done. Please LMK if what I did is not the best practice for deprecation testing. I emulated what I could find by going through https://www.drupal.org/list-changes/drupal.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.86 KB
24.76 KB
  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.

alexpott’s picture

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.

The last submitted patch, 30: 3080259-30.patch, failed testing. View results

gabesullice’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, @alexpott for the reroll and showing me the right way to do that deprecation testing!

I think this is good to go.

Wim Leers’s picture

#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).

alexpott’s picture

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

alexpott’s picture

Also noticed some now incorrect docs.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
+++ 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.

gabesullice’s picture

Issue tags: -Needs change record

CR created: https://www.drupal.org/node/3087821

I am not quite sure how to set the version metadata for it re: 8.9 vs 8.8

Patch to follow.

gabesullice’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.42 KB
26.83 KB
effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record updates
+++ 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?

alexpott’s picture

+++ 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.

gabesullice’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record updates

I created a change record explaining the change: https://www.drupal.org/node/3088385

I'm reluctant to provide a BC-layer because:

  1. This BC-layer would only be perpetuating buggy behavior
  2. As @alexpott explained, callers already calling Link::compare will continue to function just fine. Callers that were not using Link::compare are using an @internal API in a way that it was not meant to be used.
  3. Assertions shouldn't be turned on in production, so this should only fail "hard" on dev/stage
  4. I can only find one example of a contrib module using this, WoT:API, and that module appears to contain a wholesale copy of the JSON:API module, which is properly calling Link::compare before merge
effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

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.

effulgentsia’s picture

Adding reviewer credit.

  • effulgentsia committed 68bacad on 9.0.x
    Issue #3080259 by gabesullice, alexpott, Wim Leers: Links with different...

  • effulgentsia committed f5a32f4 on 8.9.x
    Issue #3080259 by gabesullice, alexpott, Wim Leers: Links with different...
effulgentsia’s picture

Version: 8.9.x-dev » 8.8.x-dev

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.

gabesullice’s picture

🎉

  • effulgentsia committed b232781 on 8.8.x
    Issue #3080259 by gabesullice, alexpott, Wim Leers: Links with different...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.8.x and published the CRs.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.