Wrap entity objects in a ResourceObject which carries a ResourceType

I was going to ask how this would interact with @e0ipso's proposal in #3014277: ResourceTypes should know about their fields, but I see you're already thinking about that in #3.6. Great!

#3.7: "between 5 and 11 seconds" is sort of meaningless, we want to know the relative speedup. And I'm not sure that's actually true; it'd only be true if we don't commit the static caching patch at #3014232: [regression] ResourceTypeRepository is significantly slower in JSON:API 2, becomes noticeable when handling hundreds of interlinked resources. With that applied, the cost is only 423 ms, and getting that to zero will not be possible. Additionally, that profile includes JSON API Extras, which inherently increases wall time since it needs to load and inspect its config too.
However, that whole static cache/memory cache business in ResourceTypeRepository has always been a pain. So I most definitely agree with I do think it illustrates the point of this issue though: using a global object to store information that should be directly associated with the entity being processed has negative consequences..

I'm interested in exploring this further, but only post-2.0. Since all of this is an internal API anyway, we can do that.

+++ b/src/JsonApiResource/ResourceObject.php
@@ -0,0 +1,184 @@
+/**
+ * Represent a JSON API resource object.
+ *
+ * This value object wraps a Drupal entity so that it can carry a JSON API
+ * resource type object alongside it.
+ *
+ * @internal
+ */
+final class ResourceObject implements CacheableDependencyInterface, ResourceIdentifierInterface {

I definitely like this, not only because of the simplifications it brings (some of which you described in #3), but also because we then have a value object that clearly maps 1:1 to the JSON:API spec. I suspect this will make the codebase easier to maintain and grok.

Sorry for misunderstanding!

My point was to illustrate that the bottleneck could be solved with a cache OR this.

That makes sense.

I think adding a cache was obviously the faster/less disruptive solution and I'm glad we did it, however it really only addressed the symptom, not the cause.

I wholeheartedly agree!

This is a small step in the direction of getting rid of jsonapi_normalizer_do_not_use_removal_imminent because

🎶 Music to my ears! Especially since people have been making remarks about that on the core issue

eliminate some requests to the repository.

👍

Sneaky little label callbackses...

😀

grand total of 297 deletions!

🎉

Actual patch review:

1. +++ b/jsonapi.services.yml
   @@ -64,19 +64,19 @@ services:
   +    class: Drupal\jsonapi\Normalizer\ContentEntityDenormalizer
   ...
   +    class: Drupal\jsonapi\Normalizer\ConfigEntityDenormalizer
   ...
   +    class: Drupal\jsonapi\Normalizer\ResourceObjectNormalizer
   

   This means we're now normalizing all "resource objects" in JSON:API terminology in exactly the same way.

   Denormalization is different out of necessity.

   Makes me curious about the rest of the patch :)

2. +++ b/src/Controller/EntityResource.php
   @@ -213,24 +214,19 @@ class EntityResource {
   -      if (isset($document['data']['attributes'])) {
   ...
   -      if (isset($document['data']['relationships'])) {
   ...
   +      foreach (['attributes', 'relationships'] as $data_member_name) {
   

   I think this simplification could already happen in a separate minor issue.

   Let's keep it here for now. If there's many more of these simplifications, lets' spin it out into a separate issue.

3. +++ b/src/Controller/EntityResource.php
   @@ -1030,30 +1029,32 @@ class EntityResource {
   -   * @return \Drupal\Core\Entity\EntityInterface|\Drupal\jsonapi\LabelOnlyEntity|\Drupal\jsonapi\Exception\EntityAccessDeniedHttpException
   +   * @return \Drupal\jsonapi\JsonApiResource\ResourceObject|\Drupal\jsonapi\LabelOnlyEntity|\Drupal\jsonapi\Exception\EntityAccessDeniedHttpException
       *   The loaded entity, a label only version of that entity or an
   

   Übernit: this comment needs to be updated.

4. +++ b/src/Controller/EntityResource.php
   @@ -1030,30 +1029,32 @@ class EntityResource {
   -        return new LabelOnlyEntity($entity);
   +        return new LabelOnlyEntity($resource_type, $entity);
   

   If we're changing the parameters, shouldn't this become LabelOnlyResourceObject?

5. +++ b/src/IncludeResolver.php
   @@ -100,21 +103,25 @@ class IncludeResolver {
   -        if (!$entity->hasField($field_name)) {
   +        if (!$entity->hasField($public_field_name)) {
   

   The old line was correct, this only needs to change because $entity is no longer an EntityInterface instance. I think it may be less confusing to just rename it to $resource_object … but I'm guessing you didn't do that to minimize change?

6. +++ b/src/IncludeResolver.php
   @@ -100,21 +103,25 @@ class IncludeResolver {
   -        // @todo: raise an omitted item to an inaccessible related field in https://www.drupal.org/project/jsonapi/issues/2956084.
   

   Why can we remove this TODO?

7. +++ b/src/JsonApiResource/EntityCollection.php
   @@ -15,11 +13,11 @@ use Drupal\jsonapi\LabelOnlyEntity;
    class EntityCollection implements \IteratorAggregate, \Countable {
   

   Not yet renamed to ResourceObjectCollection to minimize change probably.

   (Could happen in a separate issue.)

8. +++ b/src/JsonApiResource/ResourceObject.php
   @@ -0,0 +1,189 @@
   +class ResourceObject implements CacheableDependencyInterface, ResourceIdentifierInterface {
   

   😍

9. +++ b/src/JsonApiResource/ResourceObject.php
   @@ -0,0 +1,189 @@
   +  use CacheableDependencyTrait;
   +  use ResourceIdentifierTrait;
   

   😍

10. +++ b/src/JsonApiResource/ResourceObject.php
    @@ -0,0 +1,189 @@
    +  /**
    +   * The resource type of this resource object.
    +   *
    +   * @var array
    +   */
    +  protected $fields;
    

    Comment is wrong.

11. +++ b/src/JsonApiResource/ResourceObject.php
    @@ -0,0 +1,189 @@
    +   *   The underlying entity.
    

    Nit: what about "decorated"?

12. +++ b/src/JsonApiResource/ResourceObject.php
    @@ -0,0 +1,189 @@
    +   * Gets the ResourceObject's fields.
    +   *
    +   * @return mixed|\Drupal\Core\Field\FieldItemListInterface[]
    +   *   The resource object's fields.
    

    Some doubts about exposing a Drupal-specific data structure when this object is all about decorating everything in JSON:API terminology. But I can't think of a better alternative.

13. +++ b/src/JsonApiResource/ResourceObject.php
    @@ -0,0 +1,189 @@
    +  protected function extractFields(EntityInterface $entity) {
    +    assert($entity instanceof ContentEntityInterface || $entity instanceof ConfigEntityInterface);
    +    return $entity instanceof ContentEntityInterface
    +      ? $this->extractContentEntityFields($entity)
    +      : $this->extractConfigEntityFields($entity);
    +  }
    

    👌

14. +++ b/src/LabelOnlyEntity.php
    @@ -2,34 +2,15 @@
    +final class LabelOnlyEntity extends ResourceObject {
    
    @@ -41,6 +22,17 @@ class LabelOnlyEntity implements CacheableDependencyInterface, ResourceIdentifie
    +  protected function extractFields(EntityInterface $entity) {
    +    $fields = parent::extractFields($entity);
    

    😍

15. +++ b/src/Normalizer/EntityDenormalizerBase.php
    @@ -0,0 +1,122 @@
    +  public function normalize($object, $format = NULL, array $context = []) {
    +    assert(FALSE);
    +  }
    

    I'd even throw a \LogicException I think.

16. +++ b/src/Normalizer/EntityDenormalizerBase.php
    @@ -0,0 +1,122 @@
    +  abstract protected function prepareInput(array $data, ResourceType $resource_type, $format, array $context);
    

    👍

17. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
    @@ -87,7 +86,7 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer {
    -          $entity_list[] = FALSE;
    +          $resource_objects[] = FALSE;
               $entity_list_metadata[] = [
    

    I don't think it makes sense to rename $entity_list if we're not renaming so many other things, and especially when there's the closely associated/in sync $entity_list_metadata.

18. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
    @@ -123,34 +122,42 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer {
    +    $relationship = new Relationship($field->getName(), $entity_collection, $context['resource_object'], $cacheabilty, $cardinality, $main_property, $entity_list_metadata);
    
    +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
    @@ -193,6 +194,7 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
    +      $context['resource_object'] = new ResourceObject($context['resource_type'], $data->getEntity());
    

    This is why we're suddenly using $context for crucial things, right?

    If we're to do this, then let's at least immediately unset() it after it's fulfilled its purpose. We want to prevent unintentional dependencies on this context.

19. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
    @@ -123,34 +122,42 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer {
    +    // TODO: We are always loading the referenced entity. Even if it is not
    +    // going to be included. That may be a performance issue. We do it because
    
    +++ b/src/Normalizer/RelationshipItemNormalizer.php
    @@ -42,11 +42,6 @@ class RelationshipItemNormalizer extends FieldItemNormalizer {
    -    // TODO: We are always loading the referenced entity. Even if it is not
    -    // going to be included. That may be a performance issue. We do it because
    

    That is not a regression compared to HEAD though. Optimizing this deeply, based on the requested sparse fieldset is not only going to be tricky and perhaps pretty complex, it would also conflict with #2819335-18: Resource (entity) normalization should use partial caching. So I think this TODO can be removed!

    Ah, I see, I mis understood it. Why move this?

20. +++ /dev/null
    @@ -1,91 +0,0 @@
    -class LabelOnlyEntityNormalizer extends NormalizerBase {
    

    🤘

21. +++ b/src/Normalizer/Relationship.php
    @@ -45,13 +44,6 @@ class Relationship implements AccessibleInterface, CacheableDependencyInterface
    -  protected $resourceTypeRepository;
    
    @@ -80,19 +70,17 @@ class Relationship implements AccessibleInterface, CacheableDependencyInterface
           $this->items[] = new RelationshipItem(
    -        $resource_type_repository,
    

    Nice!

22. +++ b/src/Normalizer/Relationship.php
    @@ -62,13 +54,11 @@ class Relationship implements AccessibleInterface, CacheableDependencyInterface
    +   * @param \Drupal\jsonapi\JsonApiResource\ResourceObject $host_resource_object
        *   The host entity.
    

    Docs nit.

23. +++ b/src/Normalizer/Relationship.php
    @@ -113,21 +101,11 @@ class Relationship implements AccessibleInterface, CacheableDependencyInterface
    -  public function setHostEntity(EntityInterface $hostEntity) {
    

    🤘 That means this is now an immutable value object, great!

    Looks like this actually can also be done already in HEAD.

24. +++ b/src/Normalizer/RelationshipItem.php
    @@ -92,13 +85,10 @@ class RelationshipItem {
    -      $this->targetResourceType = $resource_type_repository->get(
    -        $target_entity->getEntityTypeId(),
    -        $target_entity->bundle()
    -      );
    +      $this->targetResourceType = $target_resource_object->getResourceType();
    

    🤘

25. +++ b/src/Normalizer/ResourceObjectNormalizer.php
    @@ -0,0 +1,141 @@
    +   * The interface or class that this Normalizer supports.
    +   *
    +   * @var string
    ...
    +   * The formats that the Normalizer can handle.
    +   *
    +   * @var array
    

    Nit: inheritdoc.

26. +++ b/src/Normalizer/Value/ResourceObjectNormalizerValue.php
    @@ -51,20 +51,20 @@ class EntityNormalizerValue implements ValueExtractorInterface, CacheableDepende
         $this->context = $context;
    

    I believe that $this->context was made obsolete here? :)

I forgot two things:

1. I'd like to get a better idea of how #3014277: ResourceTypes should know about their fields and this issue work together.

2. +++ b/jsonapi.services.yml
   @@ -64,19 +64,19 @@ services:
   -    class: Drupal\jsonapi\Normalizer\ContentEntityNormalizer
   +    class: Drupal\jsonapi\Normalizer\ContentEntityDenormalizer
   ...
   -    class: Drupal\jsonapi\Normalizer\LabelOnlyEntityNormalizer
   +    class: Drupal\jsonapi\Normalizer\ConfigEntityDenormalizer
   ...
   -    class: Drupal\jsonapi\Normalizer\ConfigEntityNormalizer
   +    class: Drupal\jsonapi\Normalizer\ResourceObjectNormalizer
   

   This will break JSON:API Extras.

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

Oh PHP… 😭

I don't have time to do a deep review of this today. But ideally the issue summary would explain what work this unblocks.

It's not in the issue summary, but #3.7 is citing improved performance as an important reason for doing that. But based on preliminary benchmarking (admittedly with only 4 nodes), I'm seeing a slowdown from ~87 ms for 100 requests to ~90 ms (ab -c 1 -n 100 -H "Accept: application/vnd.api+json" http://d8/jsonapi/node/article, with Page Cache & Dynamic Page Cache disabled.)

It looks like this does not block #3014277: ResourceTypes should know about their fields.

I'm sure I'm missing something, if you can just add a list of reasons why we should do this (benefits/unblocking) to the issue summary, that'd definitely help :)

#36 lost the fix from #32.

1. With this, we can pass links into ResourceObjects prior to normalization too

   💪 Let's update the patch to do this. So that we can see the full set of benefits that this brings us. But let's first address what's below.

2. +++ b/src/Controller/EntityResource.php
   @@ -482,7 +478,10 @@ class EntityResource {
      public function getRelationship(ResourceType $resource_type, FieldableEntityInterface $entity, $related, Request $request, $response_code = 200) {
   ...
   +    // Access will have already been checked by the RelationshipFieldAccess
   +    // service, so we don't need to call ::getAccessCheckedResourceObject().
   

   👍 So this is only ever called if \Drupal\jsonapi\Access\RelationshipFieldAccess::access() granted access.

   This is just adding documentation to help future readers.

3. +++ b/src/JsonApiResource/EntityCollection.php
   @@ -15,11 +13,11 @@ use Drupal\jsonapi\LabelOnlyEntity;
   +   * @var \Drupal\jsonapi\JsonApiResource\ResourceIdentifierInterface[]
   
   @@ -45,7 +43,7 @@ class EntityCollection implements \IteratorAggregate, \Countable {
   +   * @param \Drupal\jsonapi\JsonApiResource\ResourceIdentifierInterface[]|null[]|false[] $resource_objects
   
   @@ -53,17 +51,15 @@ class EntityCollection implements \IteratorAggregate, \Countable {
   +        || $entity instanceof ResourceIdentifierInterface;
   

   🤔 Hm … but I don't think we allow or ever intend to allow class ResourceIdentifier implements ResourceIdentifierInterface, do we?

   Why can't we change this to \Drupal\jsonapi\JsonApiResource\ResourceObject?

4. +++ b/src/JsonApiResource/JsonApiDocumentTopLevel.php
   @@ -44,7 +46,7 @@ class JsonApiDocumentTopLevel {
   +   * @param \Drupal\jsonapi\JsonApiResource\ResourceObject|\Drupal\jsonapi\JsonApiResource\EntityCollection|\Drupal\jsonapi\LabelOnlyResourceObject|\Drupal\jsonapi\JsonApiResource\ErrorCollection $data
   

   🤔 I don't think we need to list LabelOnlyResourceObject here, since it subclasses ResourceObject. Previously, LabelOnlyEntity did not implement EntityInterface, so it had to be listed separately.

   That makes us get quite close to an ideal state, because it means only 3 kinds of things are allowed: individual resource objects, collections of resource objects (for now still called EntityCollection, but we'll soon be able to rename that to ResourceObjectCollection) or a collection of errors.

5. +++ b/src/JsonApiResource/JsonApiDocumentTopLevel.php
   @@ -56,6 +58,7 @@ class JsonApiDocumentTopLevel {
   +    assert($data instanceof ResourceIdentifierInterface || $data instanceof EntityCollection || $data instanceof ErrorCollection || $data instanceof EntityReferenceFieldItemListInterface);
   

   🤔 This … sort of does what I was getting at above: it omits LabelOnlyResourceObject. But curiously, it checks ResourceIdentifierInterface, not ResourceObject.

   And apparently one potential type was missing from the docblock? Because we're also allowing

   EntityReferenceFieldItemListInterface</code. Remind me when that's necessary again?
   </li>
   
   <li>
   <code>
   +++ b/src/JsonApiResource/ResourceIdentifier.php
   @@ -40,18 +48,22 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   +   * @param \Drupal\jsonapi\ResourceType\ResourceType|string $resource_type
   +   *   The JSON:API resource type or a JSON:API resource type name.
   ...
   +    $this->resourceTypeName = is_string($resource_type) ? $resource_type : $resource_type->getTypeName();
   ...
   +    if (!is_string($resource_type)) {
   +      $this->resourceType = $resource_type;
   +    }
   
   @@ -65,6 +77,18 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   +  public function getResourceType() {
   +    if (!isset($this->resourceType)) {
   

   🤔 Remind me why we allow both a ResourceTypeobject and string, and retrieve the object when necessary? Why not always require it to be passed in?

   You don't need to answer that on the issue, but documenting the rationale and its assumptions in this class allow us to more easily make this simpler in the future.

6. +++ b/src/JsonApiResource/ResourceObject.php
   @@ -0,0 +1,189 @@
   + * This value object wraps a Drupal entity so that it can carry a JSON:API
   + * resource type object alongside it.
   

   🐛 That's one purpose. Another purpose is to be able to remove the need to distinguish between config and content entities all over the JSON:API code base. Let's document that too.

7. +++ b/src/JsonApiResource/ResourceObject.php
   @@ -0,0 +1,189 @@
   +   * The fields of the represented entity.
   ...
   +   * @var array
   

   🐛 Just array means I still need to guess whether this is field names (string[]) or field instances (FieldItemListInterface[]).

8. +++ b/src/LabelOnlyResourceObject.php
   @@ -0,0 +1,48 @@
   +  public function getEntity() {
   +    return $this->entity;
   +  }
   

   🤔 Hm … why does this live on this class and not on ResourceObject? Looks like this is only for the label only case in \Drupal\jsonapi\IncludeResolver::resolveIncludeTree().

   I think this makes sense for now, but I suspect we'll be able to remove it when we refactor EntityAccessDeniedHttpException to ResourceObjectAccessDeniedHttpException?

9. +++ b/src/LabelOnlyResourceObject.php
   @@ -0,0 +1,48 @@
   +    return $label_field_name && $this->resourceType->isFieldEnabled($label_field_name) ?
   +      [$this->resourceType->getPublicName($label_field_name) => $fields[$label_field_name]]
   +      : [];
   

   ✅ Can't this be simplified to an
   array_intersect_key() call? We're repeating logic that the parent method already did. Done.

10. +++ b/src/LabelOnlyResourceObject.php
    @@ -0,0 +1,48 @@
    +  /**
    +   * Determines the entity type's (internal) label field name.
    +   */
    +  public function getLabelFieldName() {
    +    $label_field_name = $this->entity->getEntityType()->getKey('label');
    +    // @todo Remove this work-around after https://www.drupal.org/project/drupal/issues/2450793 lands.
    +    if ($this->entity->getEntityTypeId() === 'user') {
    +      $label_field_name = 'name';
    +    }
    +    return $label_field_name;
    +  }
    

    ✅ This is also duplicating logic from the parent class. It'd be better to move this instead then. Also: this should not have been public. Done.

11. +++ b/src/Normalizer/ConfigEntityDenormalizer.php
    @@ -0,0 +1,33 @@
    +   * The interface or class that this Normalizer supports.
    

    ✅ Nit: Can just be {@inheritdoc}. Fixed.

12. +++ b/src/Normalizer/ContentEntityDenormalizer.php
    @@ -0,0 +1,81 @@
    +         ));
    

    ✅ Übernit: indentation is one space off. Fixed.

13. +++ b/src/Normalizer/EntityDenormalizerBase.php
    @@ -0,0 +1,122 @@
    +  /**
    +   * The formats that the Normalizer can handle.
    +   *
    +   * @var array
    +   */
    +  protected $formats = ['api_json'];
    

    ✅ This is already inherited from the base class, can be removed. Done.

14. +++ b/src/Normalizer/EntityDenormalizerBase.php
    @@ -0,0 +1,122 @@
    +   * Constructs an EntityNormalizer object.
    

    ✅ Nit: mismatch. Fixed.

15. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
    @@ -66,8 +75,8 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer {
    -          $entity_list[] = NULL;
    -          $entity_list_metadata[] = [
    +          $resource_objects[] = NULL;
    +          $relationship_object_metadata[] = [
    

    🤔 This used to be clearly paired before. I mentioned this before in #22.17. You addressed it. But I think this is still fairly confusing. The array indices need to be kept in sync.
    Perhaps #3024896: Remove Relationship and RelationshipItem classes will simplify this?

16. +++ b/src/Normalizer/Relationship.php
    @@ -36,7 +35,7 @@ class Relationship implements AccessibleInterface, CacheableDependencyInterface
        * @var \Drupal\Core\Entity\EntityInterface
    
    @@ -113,21 +101,11 @@ class Relationship implements AccessibleInterface, CacheableDependencyInterface
        * Gets the host entity.
    
    +++ b/src/Normalizer/RelationshipItem.php
    @@ -23,9 +22,9 @@ class RelationshipItem {
        * The target entity.
    

    ✅ Nit: entity -> resource object leftovers. Fixed.

17. +++ b/src/Normalizer/RelationshipItem.php
    @@ -23,9 +22,9 @@ class RelationshipItem {
    +   * @var \Drupal\jsonapi\JsonApiResource\ResourceObject|null|false
    
    @@ -51,11 +50,9 @@ class RelationshipItem {
    +   * @param \Drupal\Core\Entity\EntityInterface|null|false $target_resource_object
    
    @@ -63,23 +60,19 @@ class RelationshipItem {
    +    assert($target_resource_object === NULL || $target_resource_object === FALSE || $target_resource_object instanceof ResourceIdentifierInterface);
    

    🐛 Mismatch: ResourceObject vs ResourceIdentifierInterface vs EntityInterface.

18. +++ b/src/Normalizer/RelationshipNormalizer.php
    @@ -168,10 +169,10 @@ class RelationshipNormalizer extends NormalizerBase implements DenormalizerInter
    -    assert($context['resource_type'] instanceof ResourceType);
    -    $resource_type = $context['resource_type'];
    +    assert($context['resource_object'] instanceof ResourceObject);
    +    $resource_type = $context['resource_object']->getResourceType();
    

    🤔 I'm not sure I understand the value of this change?

19. +++ b/src/Normalizer/ResourceObjectNormalizer.php
    @@ -0,0 +1,162 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected $formats = ['api_json'];
    

    ✅ This is already inherited, can be removed. Done.

20. +++ b/src/Normalizer/ResourceObjectNormalizer.php
    @@ -0,0 +1,162 @@
    +   * Constructs an EntityNormalizer object.
    

    ✅ Mismatch. Fixed.

21. +++ b/src/Normalizer/ResourceObjectNormalizer.php
    @@ -0,0 +1,162 @@
    +  protected function getFields(ResourceObject $object) {
    ...
    +  protected function getFieldNames(ResourceObject $object) {
    

    ✅ These can now be static. Done. Actually, having these helper methods is pointless now, thanks to ResourceObject! Removed.

22. +++ b/src/Normalizer/ResourceObjectNormalizer.php
    @@ -0,0 +1,162 @@
    +    /* @var \Drupal\Core\Entity\ContentEntityInterface $entity */
    

    ✅ Needed updating. Fixed.

In doing this, I made the patch slightly smaller :) Now the diffstat is: 869 insertions(+), 866 deletions(-). So: same LoC, but clearer code.

Also: if you roll the patch with -M5% it will record the moves, making the patch not only a few K smaller, but also easier to understand. (659 insertions(+), 656 deletions(-))

Yay, #41 is still green, but it contains a single CS violation 😬 Easy fix.

Assigning to Gabe for points 1, 3–8, 15, 18 in #41

<off-topic>

Idea for follow-up issue: add ::toUrl() method inspired by EntityInterface::toUrl(). This would remove the need for \Drupal\jsonapi\LinkManager\LinkManager::getEntityLink(): you'd do ResourceObject::toUrl('individual') instead. Or even ::toLink(), which would return a \Drupal\jsonapi\JsonApiResource\Link().

</off-topic>

_{(Shall I create a follow-up issue for that?)}

You've mentioned this before and I did it for this patch. I often see a small reduction in size, but I rarely find it easier to understand. I wonder what's different...

File renames and/or moving significant pieces of logic are shown as moves rather than additions and deletions. It's super difficult for humans (at least this human!) to go through 100 lines of deletions and 100 lines of additions and verify that only a few LoC actually changed. It's much easier to see this when the diff only contains those few LoC that actually changed. 🙂

#46:

3. 👍 Ah, of course: the existing docblock was just incomplete. I should've noticed that.
4. 👍
5. 👍
6. 👍
7. 👍
8. 👍

15. Devil's advocate: what if #3024896: Remove Relationship and RelationshipItem classes doesn't land, or doesn't land in the same release?

18. I see! That sounds solid :)

#49: I usually use 5%, but yes, sometimes you need to tweak that to, say, 20%.

#50: no longer necessary to […] 🎉🎉🎉 — I've always found that super confusing! This interdiff is full of win!

#51/#41.1: Your first paragraph there describes everything I expected and hoped 😀 👏 Everything else sounds like the hard labor of our iterating towards maintainability and simplicity is paying off :)

1. +++ b/src/Controller/EntityResource.php
   @@ -220,14 +220,10 @@ class EntityResource {
   -    $entity_url = $this->linkManager->getEntityLink(
   -      $parsed_entity->uuid(),
   -      $resource_type,
   -      [],
   -      'individual'
   -    );
   -    if ($entity_url) {
   -      $response->headers->set('Location', $entity_url);
   +    if ($resource_type->isLocatable()) {
   +      $url = $resource_object->toUrl()->setAbsolute()->toString(TRUE);
   +      $response->addCacheableDependency($url);
   +      $response->headers->set('Location', $url->getGeneratedUrl());
        }
   

   😍

2. +++ b/src/Controller/FileUpload.php
   @@ -66,13 +65,6 @@ class FileUpload {
   -  protected $linkManager;
   
   +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -41,13 +40,6 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   -  protected $linkManager;
   

   👍 Apparently these were already obsolete service dependencies, fascinating.

3. +++ b/src/JsonApiResource/LinkCollection.php
   @@ -90,16 +91,29 @@ final class LinkCollection implements \IteratorAggregate {
   -   * @param \Drupal\jsonapi\JsonApiResource\JsonApiDocumentTopLevel $context
   +   * @param \Drupal\jsonapi\JsonApiResource\JsonApiDocumentTopLevel|\Drupal\jsonapi\JsonApiResource\ResourceObject $context
   ...
   -  public function withContext(JsonApiDocumentTopLevel $context) {
   +  public function withContext($context) {
   

   🤓 ✅ Nit: Let's bring a similar assert here too.

4. +++ b/src/JsonApiResource/ResourceObject.php
   @@ -49,6 +52,13 @@ class ResourceObject implements CacheableDependencyInterface, ResourceIdentifier
   +   * Resource object level links.
   

   🤓 Isn't this just Resource object's links.?

5. +++ b/src/JsonApiResource/ResourceObject.php
   @@ -56,13 +66,21 @@ class ResourceObject implements CacheableDependencyInterface, ResourceIdentifier
   +   *   (optional) Any links for the resource object, if a `self` link is not
   +   *   provided, one will be automatically added if the resource is locatable.
   

   🤔 Pondering … why/when is a self link already provided?

6. +++ b/src/Routing/Routes.php
   @@ -407,7 +407,7 @@ class Routes implements ContainerInjectionInterface {
   -  protected static function getRouteName(ResourceType $resource_type, $route_type) {
   +  public static function getRouteName(ResourceType $resource_type, $route_type) {
   

   👍 Given the benefits this brings above, and given that this is still an @internal class, I'm fine with this.

Overall diffstat: 32 files changed, 757 insertions(+), 761 deletions(-). LoC = status quo. But it brings more simplicity, and has stricter code (more assert()s) as well as better-documented helper methods that will surely prove useful elsewhere in the future too.

I reviewed it in detail. I think this is ready, because it's an iteration on the internals of our code that I believe sets us up for success in the future. Zero functional tests needing to change proves this is purely internal.

Can you update the issue summary to state that as one of the concrete benefits?

Sorry for not jumping in earlier. Just letting everyone know that this is being discussed in Slack. So progress is happening.

#3028970: Audit all @todos, #3030148: Clean-up: remove dead $formats property from normalizers and #2965056: Support `include` parameter on relationship routes (turns out it accidentally works on cold caches: fix + test this) conflicted with this, rerolled. (I can't wait for RTBC retesting for contrib modules…)

In #23, I pointed out which things would break https://www.drupal.org/project/jsonapi_extras. @gabesullice responded in #25 with the changes JSON:API Extras would need to make. Let's provide that patch here, @gabesullice?

Since https://www.drupal.org/project/consumer_image_styles/releases/8.x-3.x-dev requires jsonapi_extras, there is no way to test that yet either.

For #65.

Confirmed that with #3032451-12: Compatibility with upcoming JSON:API 2.2 release: JSON:API Extras 3.4, JSON:API Extras works. Consumer Image Styles doesn't work yet though, working on a patch for that.

Verified that this unblocks #2992836 — patch at #2992836-27: Provide links to resource versions (entity revisions).

Verified that this does not conflict with #3014277 — see rebase + comment at #3014277-17: ResourceTypes should know about their fields.

I reviewed this patch once more.

1. Fixed a few nits not worth calling out here.

2. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -44,20 +37,27 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer {
   +  protected $entityAccessChecker;
   
   +++ b/tests/src/Kernel/Normalizer/EntityReferenceFieldNormalizerTest.php
   @@ -137,9 +139,14 @@ class EntityReferenceFieldNormalizerTest extends JsonapiKernelTestBase {
        $this->normalizer = new EntityReferenceFieldNormalizer(
   ...
   +      new EntityAccessChecker(
   +        $this->container->get('jsonapi.resource_type.repository'),
   +        $this->container->get('router.no_access_checks'),
   +        $this->container->get('current_user'),
   +        $this->container->get('entity.repository')
   +      )
   

   This made me suspicious: why is this necessary? So I went to look at EntityReferenceFieldNormalizer. It had this extra dependency injected since #28. It was used in getTranslatedResourceObject().

   But as of the #67 rebase, getTranslatedResourceObject() is gone, yet the service is still injected.

   Either the service can be removed, or this is a bug in the current patch. But the earlier patch iterations had this:

   +    // @todo: determine if getting the entity translation is still relevant since includes are not processed differently.
   +    $translation = $this->entityRepository->getTranslationFromContext($entity);
   

   Indeed it doesn't make sense to get the translated entity reference — it's still the same entity that is being referenced.

   And finally, the line after that:

   +    return $this->entityAccessChecker->getAccessCheckedResourceObject($translation);
   

   That access checking is not something HEAD does either.

   Conclusion: it's fine to remove the entity access checker from EntityReferenceFieldNormalizer altogether, and in doing so it matches HEAD's behavior.

Forgot one hunk :)

That failure in 8.7 must be related to #3031750: Drupal core compatibility: User `mail` field varies by the ‘user’ cache context in Drupal >= 8.7.

(And that in turn was caused by a change in Drupal 8.7 core.)

Besides another detailed review, I've also analyzed impact of this on various things:

1. JSON:API Extras/JSON:API Defaults (see #75)
2. Blocked issue verified to be unblocked: #2992836-27: Provide links to resource versions (entity revisions)
3. I was fairly tempted to still say "no" to this because the benefits may seem a bit iffy. But … #2992836: Provide links to resource versions (entity revisions) clearly shows the value, especially combined with #2994193: [META] The Last Link: A Hypermedia Story in the future. If we want Drupal contrib modules to be able to provide extensible HATEOAS links (and @e0ipso and @gabesullice both felt very strongly about this long before I did!), then a wrapper object is a necessity. To then name that new object ResourceObject just like in the JSON:API spec and bringing more clarity to the rest of the code base makes it a win-win!
4. Related issue verified to be unaffected: #3014277-17: ResourceTypes should know about their fields
5. Filed issue to further improve the understandability of the codebase — this change made sense a long time ago but much more so after this lands: #3032679: Clean-up: rename EntityCollection to Data
6. The one remaining thing (besides making #82 pass on 8.7) is updating the patch at #3032468: Compatibility with upcoming JSON:API 2.2 release: JSON:API Extras 3.4 to account for the changes here. I started doing that, but found it quite tricky. I just talked to @gabesullice and his answer made me feel like a fool: I was trying to keep the existing structure (which is to have a normalizer that kicks in for a particular type of entity with a particular kind of value — that's what \Drupal\consumer_image_styles\Normalizer\ImageEntityNormalizer does). But he pointed out that a far simpler solution is now possible: change it to a LinkCollection normalizer that decorates the default normalization only when a particular type of entity with a particular kind of value is present — because all this normalizer does, is adding links! So it will serve as a further validation of #2994193: [META] The Last Link: A Hypermedia Story.

Assigning to @gabesullice for that last point.

Ah, yes, of course, I remember discussing that! Perfect!

Per #85, I think we should commit this as soon as we have a patch that updates consumer_image_styles to keep it working.

I just manually ran tests for Consumer Image Styles with #3032468-4: Compatibility with upcoming JSON:API 2.2 release: JSON:API Extras 3.4 applied and using #87. I reproduced the bug that #89 fixes. So it all checks out, and now the last thing of #85 has been addressed!

Let's do this! 💪

+++ b/src/LabelOnlyResourceObject.php
@@ -2,34 +2,15 @@
-class LabelOnlyEntity implements CacheableDependencyInterface, ResourceIdentifierInterface {
+final class LabelOnlyResourceObject extends ResourceObject {

While we're renaming anyway, any reason not to move this to the JsonApiResource namespace, where the base class and other similar classes are?

I'm fine with that to be a follow-up if this is otherwise ready for commit.

WELL SPOTTED! 🧐

One silly mistake.

🎉

This unblocked #2992836: Provide links to resource versions (entity revisions) and #3032679: Clean-up: rename EntityCollection to Data.