Add a \JsonApiResource\Relationship object to carry relationship data, metadata and a link collection.

"Waiting for branch to pass"

/me looks

1) Drupal\Tests\jsonapi\Functional\JsonApiRegressionTest::testMapFieldTypeNormalizationFromIssue3040590
Error: Class 'Drupal\entity_test\Entity\EntityTestMapField' not found

What now? I just checked, and it's definitely still in D8 HEAD, in the 8.8.x branch. What's going on here?

(Want you to know I'm keeping an eye on this, I'll do a review once this is green or when you ask me to review it. Its status is Active after all.)

The issue summary doesn't explain what problem this solves. #11 says this is a contributed project blocker. Ideally the issue summary would lay it out very concretely how this issue unblocks that.

Here is a first thorough pass. I covered at least 90%, but I think covering (and grokking) truly 100% will be a lot simpler once you address some of the feedback and answer some questions :)

1. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -543,15 +541,11 @@ function (EntityInterface $entity) {
   -    $field_list = $entity->get($resource_type->getInternalName($related));
   ...
   -    $relationship_object_urls = EntityReferenceFieldNormalizer::getRelationshipLinks($resource_object, $related);
   -    $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]));
   -    }, new LinkCollection([])));
   +    $primary_data = $resource_object->getField($related);
   +    $response = $this->buildWrappedResponse($primary_data, $request, $this->getIncludes($request, $primary_data), $response_code);
   

   So this hardcoded way of how links on relationships are generated disappears thanks to this new abstraction.

   Simultaneously this reduces the coupling of the JSON:API module to entities/fields (thanks to the removed $entity->get(…)). That brings us one step closer to being able to expose non-entity data, which is important for #3032787: [META] Start creating the public PHP API of the JSON:API module and friends.

   👍

2. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -974,12 +968,10 @@ protected static function relationshipResponseRequiresBody(array $received_resou
   -    assert($data instanceof Data || $data instanceof FieldItemListInterface);
   +    assert($data instanceof ResourceObjectData || $data instanceof Relationship);
   

   .

3. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -974,12 +968,10 @@ protected static function relationshipResponseRequiresBody(array $received_resou
   +    $self_link = new Link(new CacheableMetadata(), self::getRequestLink($request), ['self']);
        $links = ($links ?: new LinkCollection([]));
   -    if (!$links->hasLinkWithKey('self')) {
   -      $self_link = new Link(new CacheableMetadata(), self::getRequestLink($request), ['self']);
   -      $links = $links->withLink('self', $self_link);
   -    }
   +    $links = $links->withLink('self', $self_link);
   

   🤔 This seems like an optional optimization? If not, why is this necessary? If it is not essential, let's move that into a separate issue, I'll instantly RTBC it :)

4. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -996,7 +988,7 @@ protected function buildWrappedResponse($data, Request $request, IncludedData $i
   -   * @param \Drupal\jsonapi\JsonApiResource\IncludedData|\Drupal\jsonapi\JsonApiResource\NullIncludedData $includes
   +   * @param \Drupal\jsonapi\JsonApiResource\IncludedData $includes
   
   @@ -1008,8 +1000,7 @@ protected function buildWrappedResponse($data, Request $request, IncludedData $i
   -  protected function respondWithCollection(ResourceObjectData $primary_data, Data $includes, Request $request, ResourceType $resource_type, OffsetPage $page_param) {
   -    assert(Inspector::assertAllObjects([$includes], IncludedData::class, NullIncludedData::class));
   +  protected function respondWithCollection(ResourceObjectData $primary_data, IncludedData $includes, Request $request, ResourceType $resource_type, OffsetPage $page_param) {
   

   🤔 Why is NullIncludedData no longer possible?

5. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -1073,18 +1064,21 @@ protected function updateEntityField(ResourceType $resource_type, EntityInterfac
   -    assert($data instanceof ResourceObject || $data instanceof ResourceObjectData);
   +    assert($data instanceof ResourceObjectData || $data instanceof Relationship || $data instanceof EntityAccessDeniedHttpException);
   

   This is a tightening. 👍

6. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -1073,18 +1064,21 @@ protected function updateEntityField(ResourceType $resource_type, EntityInterfac
   +    if ($data instanceof EntityAccessDeniedHttpException) {
   +      return new IncludedData([$data]);
   +    }
   

   🤔🤔🤔 👍 We detect a 403 exception and package it up in an include. Because the include is not accessible. I had to think this one through to get it, I'd have sworn initially that this was wrong!

   This is a consequence of the changes in \Drupal\jsonapi\JsonApiResource\ResourceObject::extractContentEntityFields().

7. +++ b/core/modules/jsonapi/src/IncludeResolver.php
   @@ -123,28 +123,23 @@ protected function resolveIncludeTree(array $include_tree, Data $data, Data $inc
   -        $field_access = $field_list->access('view', NULL, TRUE);
   
   +++ b/core/modules/jsonapi/src/JsonApiResource/ResourceObject.php
   @@ -290,9 +295,21 @@ protected static function extractContentEntityFields(ResourceType $resource_type
   -      $output[$public_field_name] = $field_value;
   +      $field_access_result = $field_value->access('view', NULL, TRUE);
   +      $is_relationship = in_array($public_field_name, $relationship_field_names, TRUE);
   +      if ($field_access_result->isAllowed()) {
   +        assert(!$is_relationship || $field_value instanceof EntityReferenceFieldItemListInterface);
   +        $output[$public_field_name] = $is_relationship ? Relationship::createFromEntityReferenceField($resource_type, $field_value) : $field_value;
   +      }
   +      else {
   +        $message = $is_relationship ? 'The current user is not allowed to view this relationship.' : 'The current user is not allowed to view this field.';
   +        $exception = new EntityAccessDeniedHttpException($entity, $field_access_result, '', $message, $public_field_name);
   +        $output[$public_field_name] = $exception;
   +      }
   

   🤔 👍 This is now doing field access checking at an earlier time because we want to return a Relationship object for entity reference fields instead of just a FieldItemListInterface instance. If it's accessible, we return a RelationShipobject if it is an entity reference field, otherwise we return exactly the same as before.

   If it's not accessible, we return a 403 exception, both for entity reference fields and all other fields. This is why \Drupal\jsonapi\IncludeResolver::resolveIncludeTree() was able to remove field access checking.

   That is itself a big win, because now the include resolver does not need to bother with field-specific things anymore. 🥳

8. +++ b/core/modules/jsonapi/src/JsonApiResource/JsonApiDocumentTopLevel.php
   @@ -69,12 +69,24 @@ class JsonApiDocumentTopLevel {
   +    // If the data is a relationship, merge its links unless a link with the
   +    // same key has been provided. This is especially important for the
   +    // top-level `self` link, which must match the request URL.
   

   🤔 Why do we need to treat Relationship objects different than others? What is special about them? Linking to relevant information in the JSON:API spec would make this crystal clear.

9. +++ b/core/modules/jsonapi/src/JsonApiResource/Relationship.php
   @@ -0,0 +1,220 @@
   +  public function getContext() {
   

   🤔 "context" sounds very abstract, but this is always returning a ResourceObject. Can't we make the name more concrete too then?

10. +++ b/core/modules/jsonapi/src/JsonApiResource/ResourceIdentifier.php
    @@ -311,12 +311,15 @@ public static function toResourceIdentifier(EntityReferenceItem $item, $arity =
    -    foreach ($items as $item) {
    +    foreach ($items->filterEmptyItems() as $item) {
           // Create a ResourceIdentifier from the field item. This will make it
           // comparable with all previous field items. Here, it is assumed that the
           // resource identifier is unique so it has no arity. If a parallel
           // relationship is encountered, it will be assigned later.
           $relationship = static::toResourceIdentifier($item);
    +      if ($relationship->getResourceType()->isInternal()) {
    +        continue;
    +      }
    

    🤔 These look like bugfixes that could land separately?

11. +++ b/core/modules/jsonapi/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
    @@ -190,13 +188,7 @@ public function normalize($object, $format = NULL, array $context = []) {
    -      // @todo: remove this if-else and just call $this->serializer->normalize($data...) in https://www.drupal.org/project/jsonapi/issues/3036285.
    -      if ($data instanceof EntityReferenceFieldItemListInterface) {
    -        $document['data'] = $this->normalizeEntityReferenceFieldItemList($object, $format, $context);
    -      }
    -      else {
    -        $document['data'] = $this->serializer->normalize($data, $format, $context);
    -      }
    +      $document['data'] = $this->serializer->normalize($data, $format, $context);
    

    😍😍😍

12. +++ b/core/modules/jsonapi/src/Normalizer/RelationshipNormalizer.php
    @@ -0,0 +1,32 @@
    +  public function normalize($object, $format = NULL, array $context = []) {
    +    assert($object instanceof Relationship);
    +    return CacheableNormalization::aggregate([
    +      'data' => $this->serializer->normalize($object->getData(), $format, $context),
    +      'links' => $this->serializer->normalize($object->getLinks(), $format, $context)->omitIfEmpty(),
    +      'meta' => CacheableNormalization::permanent($object->getMeta())->omitIfEmpty(),
    +    ]);
    +  }
    

    😍😍😍

13. +++ b/core/modules/jsonapi/src/Normalizer/ResourceObjectNormalizer.php
    @@ -168,11 +170,8 @@ protected static function buildEmptyNormalization(ResourceObject $object) {
    -    // Only content entities contain FieldItemListInterface fields. Since config
    -    // entities do not have "real" fields and therefore do not have field access
    -    // restrictions.
    

    🥳 Hah! This just confirms what I wrote earlier about this reducing the coupling of JSON:API to entities/fields! 🤓

14. +++ b/core/modules/jsonapi/src/Normalizer/ResourceObjectNormalizer.php
    @@ -168,11 +170,8 @@ protected static function buildEmptyNormalization(ResourceObject $object) {
    -      $field_access_result = $field->access('view', $context['account'], TRUE);
    +      $field_access_result = $field->access('view', NULL, TRUE);
    
    +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
    @@ -385,7 +385,6 @@ protected function normalize(EntityInterface $entity, Url $url) {
    -      'account' => $this->account,
    
    +++ b/core/modules/jsonapi/tests/src/Kernel/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php
    @@ -360,7 +359,6 @@ public function testNormalizeUuid() {
    -          'account' => NULL,
    
    +++ b/core/modules/jsonapi/tests/src/Kernel/Normalizer/RelationshipNormalizerTest.php
    @@ -173,10 +173,11 @@ public function testNormalize($entity_property_names, $field_name, $expected) {
    -      'account' => $this->account,
    
    +++ b/core/modules/jsonapi/tests/src/Kernel/Serializer/SerializerTest.php
    @@ -87,9 +87,7 @@ protected function setUp() {
    -    $context = ['account' => $this->user];
    -
    -    $value = $this->sut->normalize($this->node->field_text, 'api_json', $context);
    +    $value = $this->sut->normalize($this->node->field_text, 'api_json');
    
    @@ -99,17 +97,17 @@ public function testFallbackNormalizer() {
    -    $value = $this->sut->normalize($traversableObject, 'api_json', $context);
    +    $value = $this->sut->normalize($traversableObject, 'api_json');
    

    🥳🆚🤔

    I'm not sure what to think of this one. On the one hand, it's great to see the $context['account'] thing disappear. On the other hand, this seems unrelated to this issue?

15. +++ b/core/modules/jsonapi/src/Normalizer/ResourceObjectNormalizer.php
    @@ -180,6 +179,12 @@ protected function serializeField($field, array $context, $format) {
    +    elseif ($field instanceof Relationship) {
    +      return $this->serializer->normalize($field, $format, $context);
    +    }
    +    elseif ($field instanceof EntityAccessDeniedHttpException) {
    +      return new CacheableOmission($field);
    +    }
    

    🤔 I understand the first one: ResourceObject::getField() now returns a Relationship object for relationship fields.

    But the second one I do not understand. What triggered that change?

16. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalMultilingualTest.php
    @@ -60,8 +60,9 @@ public function testReadMultilingual() {
    -    $included_tags = array_filter($output['included'], function ($entry) {
    -      return $entry['type'] === 'taxonomy_term--tags';
    +    $tag = $this->nodes[0]->get('field_tags')->entity;
    +    $included_tags = array_filter($output['included'], function ($entry) use ($tag) {
    +      return $entry['type'] === 'taxonomy_term--tags' && $entry['id'] === $tag->uuid();
    

    🤔 This one could use a comment. This seems to restrict the filtering further. Which is not what you wrote in #22. I'm probably missing something :)

17. +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
    index ce37fec23b..1703fbefde 100644
    --- a/core/modules/jsonapi/tests/src/Kernel/Controller/EntityResourceTest.php
    
    --- a/core/modules/jsonapi/tests/src/Kernel/Controller/EntityResourceTest.php
    +++ b/core/modules/jsonapi/tests/src/Kernel/Controller/EntityResourceTest.php
    

    A LOT of changes here. This saddened me. You should not have to deal with that.

    So this reminded me to address the feedback at #3060836 — see #3060836-9: Remove 99% of \Drupal\Tests\jsonapi\Kernel\Controller\EntityResourceTest since it is a subset of the functional tests. Hopefully that lands soon, then this patch will become a fair bit smaller :)

Regarding #24.17: YAY, #3060836: Remove 99% of \Drupal\Tests\jsonapi\Kernel\Controller\EntityResourceTest since it is a subset of the functional tests landed, which means all of that can be omitted from this patch 🥳 I am so curious to see the reduction in patch size that I did that for you 🤓

From 19 files changed, 479 insertions, 288 deletions. in 66.67 KB to 18 files changed, 369 insertions, 251 deletions. in 57.36 KB. 😀

Wrong patch attached, see next comment for the right patch.

#25 contained the wrong patch 🤦‍♂️

3. 👍I see!
4. 👍(Keeping the original doxygen then would make this slightly simpler to get committed, because less change to understand.)

8. I get it now, thanks! 🙏👍
9. Hm. Still not sure. A relationship could in theory be stored separately, on the source, or on the target. In Drupal's Entity/Field system, it's always stored on the source. Makes me want to do something like getHost() or getSource() or getOrigin() or something else like that. But that's kinda odd too. I guess getContext() still allows for the freedom for other kinds of contexts in the future? But if that's the intent, then we shouldn't typehint to ResourceObject?
10. Ah, I see! This new place for that same logic actually makes more sense. Nice. 👏

14. You'll revert this in a next patch then, great!
15. Cool :)
16. That helps, thanks!

#29: Yay! 🥳 The Having that object is valuable because […] paragraph is super helpful by the way. I think it'd be a valuable addition to the issue summary. It will make it easier for a non-JSON:API expert (including core committers) to understand the value and justification for this patch. Hah, you already did that! 👏

From 57.78 KB to 36.64 KB 😲👍

1. +++ b/core/modules/jsonapi/jsonapi.services.yml
   @@ -72,11 +72,10 @@ services:
   -  serializer.normalizer.entity_reference_field.jsonapi:
   -    class: Drupal\jsonapi\Normalizer\EntityReferenceFieldNormalizer
   +  serializer.normalizer.relationship.jsonapi:
   +    class: Drupal\jsonapi\Normalizer\RelationshipNormalizer
   

   (For when this is RTBC.)

   ℹ️ Dear core committer: see \Drupal\jsonapi\Serializer\Serializer::__construct() — we already very explicitly prevent modules adding or overriding normalizers! That's why this is not a BC break.

   We have #3032787: [META] Start creating the public PHP API of the JSON:API module to start exposing a public PHP API.

2. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -954,7 +953,7 @@ protected static function relationshipResponseRequiresBody(array $received_resou
   -   * @param mixed $data
   +   * @param \Drupal\jsonapi\JsonApiResource\ResourceObjectData|\Drupal\jsonapi\JsonApiResource\Relationship data
   
   @@ -974,7 +973,7 @@ protected static function relationshipResponseRequiresBody(array $received_resou
   -    assert($data instanceof Data || $data instanceof FieldItemListInterface);
   +    assert($data instanceof ResourceObjectData || $data instanceof Relationship);
   
   +++ b/core/modules/jsonapi/src/JsonApiResource/JsonApiDocumentTopLevel.php
   @@ -69,12 +69,29 @@ class JsonApiDocumentTopLevel {
   -    assert($data instanceof ResourceIdentifierInterface || $data instanceof Data || $data instanceof ErrorCollection || $data instanceof EntityReferenceFieldItemListInterface);
   +    assert($data instanceof ResourceObjectData || $data instanceof Relationship || $data instanceof ErrorCollection);
   

   So … it looks like these occurrences of instanceof Data should already have been instanceof ResourceObjectData?

3. +++ b/core/modules/jsonapi/src/JsonApiResource/LinkCollection.php
   @@ -28,7 +28,7 @@
   -   * @var \Drupal\jsonapi\JsonApiResource\JsonApiDocumentTopLevel|\Drupal\jsonapi\JsonApiResource\ResourceObject
   +   * @var \Drupal\jsonapi\JsonApiResource\JsonApiDocumentTopLevel|\Drupal\jsonapi\JsonApiResource\ResourceObject|\Drupal\jsonapi\JsonApiResource\Relationship
   
   @@ -51,7 +51,7 @@ public function __construct(array $links, $context = NULL) {
   -    assert(is_null($context) || Inspector::assertAllObjects([$context], JsonApiDocumentTopLevel::class, ResourceObject::class));
   +    assert(is_null($context) || Inspector::assertAllObjects([$context], JsonApiDocumentTopLevel::class, ResourceObject::class, Relationship::class));
   

   👍 This is just an addition, to indicate that a LinkCollection's context can now also be a Relation object. That's exactly what the goal of this issue is.

4. +++ b/core/modules/jsonapi/src/JsonApiResource/Relationship.php
   @@ -0,0 +1,219 @@
   +  /**
   +   * The context resource object of the relationship.
   +   *
   +   * @var \Drupal\jsonapi\JsonApiResource\ResourceObject
   +   */
   +  protected $context;
   ...
   +   * @param \Drupal\jsonapi\JsonApiResource\ResourceObject $context
   +   *   (internal use only) The relationship's context object. Use the
   +   *   self::withContext() method to establish a context.
   ...
   +  /**
   +   * Establishes a new ResourceObject context.
   +   *
   +   * @param \Drupal\jsonapi\JsonApiResource\ResourceObject $resource_object
   +   *   The new context object.
   +   *
   +   * @return static
   +   *   A new Relationship with the given context.
   +   */
   +  public function withContext(ResourceObject $resource_object) {
   +    return new static($this->fieldName, $this->data, $this->links, $this->meta, $resource_object);
   +  }
   +
   +  /**
   +   * Gets context resource object of the relationship.
   +   *
   +   * @return \Drupal\jsonapi\JsonApiResource\ResourceObject
   +   *   The context ResourceObject.
   +   */
   +  public function getContext() {
   +    assert(!is_null($this->context), 'A Relationship is invalid unless a context has been established.');
   +    return $this->context;
   +  }
   +
   

   🤓 #31: that RFC link would be valuable to add to the interface, because it helps clarify the intended semantics of the term "context" in this particular case.

   EDIT: While I initially was completely convinced by #31, I realized that #31's argument and reference to that RFC was specifically about links, but this value object is not for a link. It's for a relationship. So perhaps link context is more appropriate? Because this reminded me again of what I wrote in #28.9: the "context" could arguably be either the source or the destination of the relationship!

   So I think this still needs a little more refinement. Perhaps we keep the name the same, but then the docs need to address all potential misinterpretations.

   (It doesn't help that \Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize() also has a $context parameter…)

5. +++ b/core/modules/jsonapi/src/JsonApiResource/Relationship.php
   @@ -0,0 +1,219 @@
   +    $this->fieldName = $public_field_name;
   

   🤔 Should we add an assert(…) to verify this is indeed a public field name? We've had bugs in the past where we forgot the translation from public to internal or vice versa.
   That'd be a cost that is only incurred when assertions are enabled.

6. +++ b/core/modules/jsonapi/src/JsonApiResource/Relationship.php
   @@ -0,0 +1,219 @@
   +   *   locatable and is not an internal entity.
   ...
   +    if ($context_resource_type->isLocatable() && !$context_resource_type->isInternal()) {
   

   🔎🐛 s/entity/resource type/

7. +++ b/core/modules/jsonapi/src/JsonApiResource/Relationship.php
   @@ -0,0 +1,219 @@
   +   *   An instantiated resource object.
   

   🔎🐛 s/resource/relationship/

8. +++ b/core/modules/jsonapi/src/JsonApiResource/Relationship.php
   @@ -0,0 +1,219 @@
   +        if ($context_is_versionable && $context_entity instanceof RevisionableInterface) {
   

   🤔 Do we need that second operand here? $context_is_versionable should be sufficient?

   If it's for prudence, then I think this is more appropriate:

   if (!$context_entity instanceof RevisionableInterface) {
     throw new …
   }
   

9. +++ b/core/modules/jsonapi/src/JsonApiResource/Relationship.php
   @@ -0,0 +1,219 @@
   +      $has_non_internal_resource_type = array_reduce($context_resource_type->getRelatableResourceTypesByField($public_field_name), function ($carry, ResourceType $target) {
   +        return $carry ?: !$target->isInternal();
   +      }, FALSE);
   +      if (!$links->hasLinkWithKey('related') && $has_non_internal_resource_type) {
   +        $route_name = Routes::getRouteName($context_resource_type, "$public_field_name.related");
   +        $related_link = Url::fromRoute($route_name, ['entity' => $context_entity->uuid()]);
   +        if ($context_is_versionable) {
   +          $related_link->setOption('query', [JsonApiSpec::VERSION_QUERY_PARAMETER => 'id:' . $context_entity->getRevisionId()]);
   +        }
   +        $links = $links->withLink('related', new Link(new CacheableMetadata(), $related_link, ['related']));
   +      }
   

   🙏 This section could use a comment.

10. +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
    @@ -1722,7 +1722,6 @@ protected function getExpectedGetRelationshipDocument($relationship_field_name,
    -      $self_link->setOption('query', $version_query);
    

    🤔 This looks like a BC break?

11. +++ b/core/modules/jsonapi/tests/src/Kernel/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php
    @@ -254,8 +254,8 @@ public function testNormalize() {
           'links' => [
    -        'self' => ['href' => Url::fromUri('internal:/jsonapi/node/article/' . $this->node->uuid() . '/relationships/node_type', ['query' => ['resourceVersion' => 'id:' . $this->node->getRevisionId()]])->setAbsolute()->toString(TRUE)->getGeneratedUrl()],
             'related' => ['href' => Url::fromUri('internal:/jsonapi/node/article/' . $this->node->uuid() . '/node_type', ['query' => ['resourceVersion' => 'id:' . $this->node->getRevisionId()]])->setAbsolute()->toString(TRUE)->getGeneratedUrl()],
    +        'self' => ['href' => Url::fromUri('internal:/jsonapi/node/article/' . $this->node->uuid() . '/relationships/node_type', ['query' => ['resourceVersion' => 'id:' . $this->node->getRevisionId()]])->setAbsolute()->toString(TRUE)->getGeneratedUrl()],
           ],
         ], $normalized['data']['relationships']['node_type']);
    

    🤔 Apparently links are now being sorted?

12. +++ b/core/modules/jsonapi/tests/src/Kernel/Normalizer/RelationshipNormalizerTest.php
    @@ -173,11 +174,10 @@ public function testNormalize($entity_property_names, $field_name, $expected) {
    -      'account' => $this->account,
    

    🤔 Why can this one still be removed?

4. I don't see how a context could be either the source or the destination of a link. What part of your argument am I missing?

   Drupal entities happen to store references (relationships) on the source. The source stores a reference to the destination. It could've been implemented differently. And non-entity data sources (ResourceTypes) could do it differently.

   That's why I say that a Relationship's context needing to be a ResourceObject is ambiguous.

   Perhaps the disconnect is that you're thinking of a relationship as a representation of a bidirectional reference […] The spec doesn't see a relationship like that though. A relationship object represents a unidirectional reference […]

   Aha! ☝ 😀

   You're right. I wish the spec explicitly used the word "unidirectional".

   Improving the docs then is sufficient, and you did exactly that in #34 👍

5. Pity. No big deal.

Nice work, I love value objects :)

1. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -548,10 +549,8 @@ public function getRelationship(ResourceType $resource_type, FieldableEntityInte
   -    $relationship_object_urls = EntityReferenceFieldNormalizer::getRelationshipLinks($resource_object, $related);
   ...
   +    $relationship = Relationship::createFromEntityReferenceField($resource_type, $field_list)->withContext($resource_object);
   
   +++ b/core/modules/jsonapi/src/JsonApiResource/Relationship.php
   @@ -0,0 +1,241 @@
   +  protected static function buildLinkCollectionFromEntityReferenceField(ResourceType $context_resource_type, EntityReferenceFieldItemListInterface $field, LinkCollection $links) {
   

   do we need to profile this change, there's a lot going on in the protected static method?

2. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -954,7 +953,7 @@ protected static function relationshipResponseRequiresBody(array $received_resou
   +   * @param \Drupal\jsonapi\JsonApiResource\ResourceObjectData|\Drupal\jsonapi\JsonApiResource\Relationship data
   
   @@ -974,7 +973,7 @@ protected static function relationshipResponseRequiresBody(array $received_resou
   +    assert($data instanceof ResourceObjectData || $data instanceof Relationship);
   
   +++ b/core/modules/jsonapi/src/JsonApiResource/JsonApiDocumentTopLevel.php
   @@ -69,12 +69,29 @@ class JsonApiDocumentTopLevel {
   +    assert($data instanceof ResourceObjectData || $data instanceof Relationship || $data instanceof ErrorCollection);
        assert(!$data instanceof ErrorCollection || $includes instanceof NullIncludedData);
   -    $this->data = $data instanceof ResourceObjectData ? $data->getAccessible() : $data;
   +    if ($data instanceof Relationship) {
   +      $this->data = $data->getData();
   +    }
   +    else {
   +      $this->data = $data instanceof ResourceObjectData ? $data->getAccessible() : $data;
   
   +++ b/core/modules/jsonapi/src/JsonApiResource/LinkCollection.php
   @@ -28,7 +28,7 @@
   +   * @var \Drupal\jsonapi\JsonApiResource\JsonApiDocumentTopLevel|\Drupal\jsonapi\JsonApiResource\ResourceObject|\Drupal\jsonapi\JsonApiResource\Relationship
   
   @@ -39,7 +39,7 @@
   +   * @param \Drupal\jsonapi\JsonApiResource\JsonApiDocumentTopLevel|\Drupal\jsonapi\JsonApiResource\ResourceObject|\Drupal\jsonapi\JsonApiResource\Relationship $context
   
   @@ -51,7 +51,7 @@ public function __construct(array $links, $context = NULL) {
   +    assert(is_null($context) || Inspector::assertAllObjects([$context], JsonApiDocumentTopLevel::class, ResourceObject::class, Relationship::class));
   

   this feels like we're missing an abstraction here.

   One instanceof is red-flag, two indicates we're likely missing something.

   e.g. a method getTopLevelDocumentData or getData or similar on an interface that all three of them implement

   for $data it would return $this, for ResourceObjectData, it would return ::getAccessible, and for relationship, it would replace ::getData

   we'd then typehint on that interface

3. +++ b/core/modules/jsonapi/src/JsonApiResource/JsonApiDocumentTopLevel.php
   @@ -69,12 +69,29 @@ class JsonApiDocumentTopLevel {
   +    $this->links = $data instanceof Relationship
   +      ? LinkCollection::merge($links->withContext($this), $data->getLinks()->filter(function ($key) use ($links) {
   +        return !$links->hasLinkWithKey($key);
   +      })->withContext($this))
   +      : $links->withContext($this);
   +    $this->meta = $data instanceof Relationship ? NestedArray::mergeDeep($meta, $data->getMeta()) : $meta;
   

   the same interface could also have mergeLinks and mergeMetadata methods, offloading the burden of knowing 'how do I merge a N object' to those objects themselves, instead of here.

4. +++ b/core/modules/jsonapi/src/JsonApiResource/Relationship.php
   @@ -0,0 +1,241 @@
   +  public function __construct($public_field_name, RelationshipData $data, LinkCollection $links, array $meta, ResourceObject $context = NULL) {
   +    $this->fieldName = $public_field_name;
   

   Do we expect the user to call the factory method here? If so should we make this protected?

5. +++ b/core/modules/jsonapi/src/JsonApiResource/Relationship.php
   @@ -0,0 +1,241 @@
   +    assert(!is_null($this->context), 'A Relationship is invalid unless a context has been established.');
   ...
   +    assert(!is_null($this->context), 'A Relationship is invalid unless a context has been established.');
   ...
   +    assert(!is_null($this->context), 'A Relationship is invalid unless a context has been established.');
   

   This is concerning, can we not prevent the object from getting into an invalid state?

   I.e any reason not to make the context argument required?

6. +++ b/core/modules/jsonapi/src/JsonApiResource/Relationship.php
   @@ -0,0 +1,241 @@
   +      // If a `related` link was not provided, automatically generate a one from
   

   nit: generate one

7. +++ b/core/modules/jsonapi/src/JsonApiResource/ResourceIdentifier.php
   @@ -311,12 +311,15 @@ public static function toResourceIdentifier(EntityReferenceItem $item, $arity =
   diff --git a/core/modules/jsonapi/src/Normalizer/EntityReferenceFieldNormalizer.php b/core/modules/jsonapi/src/Normalizer/EntityReferenceFieldNormalizer.php
   
   diff --git a/core/modules/jsonapi/src/Normalizer/EntityReferenceFieldNormalizer.php b/core/modules/jsonapi/src/Normalizer/EntityReferenceFieldNormalizer.php
   deleted file mode 100644
   

   I don't think we can just wholesale remove this, it needs to be deprecated, but we've had that discussion before on two other issues and I'll defer to the release managers

8. +++ b/core/modules/jsonapi/src/Normalizer/ResourceObjectNormalizer.php
   @@ -176,7 +178,17 @@ protected function serializeField($field, array $context, $format) {
   +        $relationship = Relationship::createFromEntityReferenceField($resource_object->getResourceType(), $field)->withContext($resource_object);
   
   +++ b/core/modules/jsonapi/tests/src/Kernel/Normalizer/RelationshipNormalizerTest.php
   @@ -173,11 +174,10 @@ public function testNormalize($entity_property_names, $field_name, $expected) {
   +    $relationship = Relationship::createFromEntityReferenceField($resource_object->getResourceType(), $resource_object->getField($field_name))->withContext($resource_object);
   

   these is the only other places I see we call the factory method, and in both cases we use ->withContext immediately, can you elaborate why context is optional in the constructor?

Tagging for RM review for the BC question

#36.4: PROTECTED CONSTRUCTORS!???!!!! 🤯🤯🤯🤯🤯🤯🤯🤯🤯 Like @gabesullice, I had no idea!

1. +++ b/core/modules/jsonapi/src/JsonApiResource/JsonApiDocumentTopLevel.php
   @@ -69,30 +67,13 @@ class JsonApiDocumentTopLevel {
   +    $this->links = $data instanceof TopLevelDataInterface ? $data->getMergedLinks($links->withContext($this)) : $links->withContext($this);
   +    $this->meta = $data instanceof TopLevelDataInterface ? $data->getMergedMeta($meta) : $meta;
   

   This is so much better now! 👏 Great suggestion, @larowlan 😃

2. +++ b/core/modules/jsonapi/src/JsonApiResource/Relationship.php
   @@ -204,33 +221,32 @@ public function getMeta() {
   -        $self_link = Url::fromRoute($route_name, ['entity' => $context_entity->uuid()]);
   +        $self_link = Url::fromRoute($route_name, ['entity' => $context->getId()]);
   ...
   -          assert($context_entity instanceof RevisionableInterface);
   -          $self_link->setOption('query', [JsonApiSpec::VERSION_QUERY_PARAMETER => 'id:' . $context_entity->getRevisionId()]);
   +          $self_link->setOption('query', [JsonApiSpec::VERSION_QUERY_PARAMETER => $context->getVersionIdentifier()]);
   ...
   -        $related_link = Url::fromRoute($route_name, ['entity' => $context_entity->uuid()]);
   +        $related_link = Url::fromRoute($route_name, ['entity' => $context->getId()]);
   ...
   -          $related_link->setOption('query', [JsonApiSpec::VERSION_QUERY_PARAMETER => 'id:' . $context_entity->getRevisionId()]);
   +          $related_link->setOption('query', [JsonApiSpec::VERSION_QUERY_PARAMETER => $context->getVersionIdentifier()]);
   

   Nice wins here, again thanks to @larowlan's suggestion! 👍

3. +++ b/core/modules/jsonapi/src/JsonApiResource/ResourceObject.php
   @@ -112,6 +112,20 @@ public static function createFromEntity(ResourceType $resource_type, EntityInter
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getData() {
   +    return new ResourceObjectData([$this], 1);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getOmissions() {
   +    return new OmittedData([]);
   +  }
   

   👎 These additions claim to inherit docs because they presumably implement an interface. But they don't.

   AFAICT these are accidental additions that can be deleted?

I think we need to take another careful look at TopLevelDataInterface.

It was added in response to @larowlan's review. I like how it simplifies things. But. I am concerned about the set of classes that are implementing this interface:

• ResourceObjectData
• OmittedData
• IncludedData
• NullIncludedData
• Relationship

The latter seems … off. Especially because Relationship contains a RelationshipData object. Based on the apparent symmetry that is present for the other classes implementing this interface, I would expect RelationshipData to implement this interface and not Relationship.

Is this because RelationshipData is misnamed and it should've been called RelationshipObject and Relationship should've been called RelationshipData?

#42 is a crystal-clear answer 👍

RTBC'ing, but can you please update the issue summary ASAP? I explained in #24 why this is necessary.

If we didn't have code specifically preventing more normalizers from being added I would agree we should keep the class around deprecated, even if it was likely to be dead code. But given there's no way for contrib to use it this should be zero impact, and it's not got any other uses. Untagging. See also comment here for longer explanation https://www.drupal.org/project/drupal/issues/3014277#comment-13239653

Did not do an in-depth patch review.

If we didn't have code specifically preventing more normalizers from being added I would agree we should keep the class around deprecated, even if it was likely to be dead code.

Completely agreed!

Needs reroll.

Reroll

Restoring tags

Do a reroll, they said.
It wil be easy, they said.

Back to RTBC after re-roll

@gabesullice no problem,but you might wanna change the "re-tests daily" to #51 instead of #48, because I screwed that last one up badly. #51 is where the rainbows and unicorns are.

Thanks for accommodating my suggestions, I think this is looking pretty tidy now.

1. +++ b/core/modules/jsonapi/src/JsonApiResource/JsonApiDocumentTopLevel.php
   @@ -69,13 +67,13 @@ class JsonApiDocumentTopLevel {
   +    $this->data = $data instanceof TopLevelDataInterface ? $data->getData() : $data;
   ...
   +    $this->links = $data instanceof TopLevelDataInterface ? $data->getMergedLinks($links->withContext($this)) : $links->withContext($this);
   +    $this->meta = $data instanceof TopLevelDataInterface ? $data->getMergedMeta($meta) : $meta;
   +    $this->omissions = $data instanceof TopLevelDataInterface
   

   are we sure there's no way to get rid of these instanceof checks?

   It feels like we're missing two factory methods on this class.

   ::fromTopLevelData and ::fromErrorCollection

   And then the constructor can be protected

   Like I said, instanceof is always a red flag in my book (but hey, so is else and elseif, what can I say, I'm peculiar)

2. +++ b/core/modules/jsonapi/src/JsonApiResource/Relationship.php
   @@ -0,0 +1,257 @@
   +   * Relationship constructor.
   

   Can we comment here that this is protected by design and add a @see to the factory method?

Do a reroll, they said.
It wil be easy, they said.

feels like that should be on a tshirt 😂

Bumping to major since this is a contrib blocker. Thanks!

Crediting myself here as my review changed the patch somewhat.
Crediting @catch for an RM review, as those are invaluable.

Can we get a change-record here - yes there's no BC layer and the changes are internal, but if someone has gone around that, a change-record might help them resolve the issue when they're searching after 8.8 comes out to work out why their thing no longer works

Please put straight back to RTBC, I will keep an eye out for the issue.

Committed 7af2d3d and pushed to 8.8.x. Thanks!

Thanks for persisting on this one 🎉

Published the change record

I think this doesn't need to go in the release notes for the same reason we didn't bother with BC in the first place, as described in #45. The CR should be sufficient.