Remove Relationship and RelationshipItem classes

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

#5:

+++ b/src/Normalizer/RelationshipNormalizer.php
@@ -51,16 +42,20 @@ class RelationshipNormalizer extends NormalizerBase implements DenormalizerInter
+    throw new \LogicException('👋 This is not the normalizer you\'re looking for');

😂

#8: JSON:API Extras only started using RelationshipItem(Normalizer) since the last week, since #3025284: Allow field enhancers for relationships.

Curious to see where this ends up! Not doing a deep review yet because that seems premature. My key critical remark right now would be: \Drupal\jsonapi\Normalizer\RelationshipNormalizer is still there for its ::denormalize(), that's confusing. I suspect it'll be renamed to ResourceIdentifierDenormalizer. Looking forward to finding out :)

This will require another coordinated update of JSON:API Extras and more release babysitting :-(

If this ends up happening, @gabesullice and I will provide a patch for JSON:API Extras. Just like we're doing at #3025037: Make JSON:API Extras compatible with the upcoming 2.1 release.

#14:

1. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -88,8 +84,7 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer {
   -          $entity_list[] = NULL;
   -          $entity_list_metadata[] = [
   

   👍 I sure like removing this! That's one of the trickier bits of code.

2. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -99,10 +94,10 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer {
   -          $entity_list[] = FALSE;
   
   @@ -149,10 +145,33 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer {
   -    $entity_collection = new EntityCollection($entity_list, $cardinality);
   -    return $this->normalizeRelationship($field->getName(), $entity_collection, $field->getEntity(), $cacheability, $cardinality, $main_property, $entity_list_metadata, $format, $context);
   

   👍 Ah, this is why you say we can make EntityCollection stricter now: because we stop using it as a vessel for passing data in one particular internal-only use case. Great!

#15:

+++ b/src/JsonApiResource/EntityCollection.php
@@ -54,13 +54,7 @@ class EntityCollection implements \IteratorAggregate, \Countable {
-    assert(Inspector::assertAll(function ($entity) {
-        return $entity === NULL
-        || $entity === FALSE
-        || $entity instanceof EntityInterface
-        || $entity instanceof LabelOnlyEntity
-        || $entity instanceof EntityAccessDeniedHttpException;
-    }, $resources));
+    assert(Inspector::assertAllObjects($resources, EntityInterface::class, LabelOnlyEntity::class, EntityAccessDeniedHttpException::class));

👍 Huge simplification!

And once #3015438: Wrap entity objects in a ResourceObject which carries a ResourceType lands, this will be down from 3 possible classes to only 2!

#16:
Great, an explicit regression test ensures we don't reintroduce the same regression 🤘

1. +++ b/src/JsonApiResource/ResourceIdentifier.php
   @@ -215,13 +218,20 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
        $target = $item->get($property_name)->getValue();
   ...
   +    if ($target === NULL || $target === FALSE) {
   
   @@ -289,4 +312,68 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   +    assert($value === NULL || $value === FALSE);
   

   🐛 AFAICT this is wrong, and it's most likely the root cause for the remaining failure. #2984647: Dangling entity references in entity reference field with multiple possible target bundles: results in exception/500 response introduced passing FALSE into EntityCollection to model a missing entity. But the target ID is not FALSE! It's a valid target ID. It just cannot be resolved to anything.

   The overall condition for NULL or FALSE to be passed into EntityCollection in HEAD is if ($item->get('entity')->getValue() === NULL) {. And then we inspect target_id: we special case a target ID of 0 for the taxonomy_term target type: this gets NULL. Everything else gets FALSE.

2. +++ b/src/JsonApiResource/ResourceIdentifier.php
   @@ -234,24 +244,37 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   +   * @param bool $omit_single_arity
   +   *   (optional) Whether ResourceIdentifiers that are not part of a parallel
   +   *   relationship should have their arity omitted. This is useful when
   +   *   creating ResourceIdentifiers to be serialized, but not for comparing
   +   *   sets of ResourceIdentifiers to perform a mutation.
   ...
   -  public static function toResourceIdentifiers(EntityReferenceFieldItemListInterface $items) {
   +  public static function toResourceIdentifiers(EntityReferenceFieldItemListInterface $items, $omit_single_arity = FALSE) {
   

   🤔 Rather than introducing an optional parameter, wouldn't toResourceIdentifiersWithoutArity() that decorates toResourceIdentifiers() be clearer?

   In any case, I find the changes in this method very difficult to grok.

3. +++ b/src/JsonApiResource/ResourceIdentifier.php
   @@ -289,4 +312,68 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   +  protected  static function getVirtualOrMissingResourceIdentifier(EntityReferenceItem $item) {
   

   Nit: double space after protected.

4. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -66,112 +64,27 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer {
   -      // A non-empty entity reference field that refers to a non-existent entity
   

   👍 All of this logic is moved into ResourceIdentifier::getVirtualOrMissingResourceIdentifier(). Interesting. Seems like a good plan overall.

Great! 😀

18 files changed, 529 insertions, 921 deletions.

1. +++ b/jsonapi.services.yml
   @@ -191,7 +186,7 @@ services:
   -      - '@jsonapi.file.uploader.field'
   +      - '@file.uploader'
   
   @@ -260,7 +255,7 @@ services:
   -  jsonapi.file.uploader.field:
   -    class: Drupal\jsonapi\ForwardCompatibility\FileFieldUploader
   +  file.uploader:
   +    class: Drupal\jsonapi\ForwardCompatibility\FileUploader
   
   +++ b/src/Controller/FileUpload.php
   @@ -54,7 +54,7 @@ class FileUpload {
   -   * @var \Drupal\jsonapi\ForwardCompatibility\FileFieldUploader
   +   * @var \Drupal\jsonapi\ForwardCompatibility\FileUploader
   
   @@ -79,14 +79,14 @@ class FileUpload {
   -   * @param \Drupal\jsonapi\ForwardCompatibility\FileFieldUploader $file_uploader
   +   * @param \Drupal\jsonapi\ForwardCompatibility\FileUploader $file_uploader
   
   similarity index 99%
   rename from src/ForwardCompatibility/FileFieldUploader.php
   
   rename from src/ForwardCompatibility/FileFieldUploader.php
   rename to src/ForwardCompatibility/FileUploader.php
   
   +++ b/src/ForwardCompatibility/FileUploader.php
   @@ -38,7 +37,7 @@ use Symfony\Component\Validator\ConstraintViolation;
   -class FileFieldUploader {
   +class FileUploader {
   

   ✅ These changes must be accidental: they revert back to a previous iteration of #2958554: Allow creation of file entities from binary data via JSON API requests. Reverted.

2. +++ b/src/Controller/EntityResource.php
   @@ -220,7 +220,7 @@ class EntityResource {
   -    $response = $this->buildWrappedResponse($parsed_entity, $request, $this->getIncludes($request, $parsed_entity), 201);
   +    $response = $this->buildWrappedResponse($parsed_entity, $request, new NullEntityCollection(), 201);
   
   @@ -278,7 +278,7 @@ class EntityResource {
   -    return $this->buildWrappedResponse($entity, $request, $this->getIncludes($request, $entity));
   +    return $this->buildWrappedResponse($entity, $request, new NullEntityCollection());
   
   +++ b/tests/src/Functional/JsonApiRegressionTest.php
   @@ -765,83 +765,4 @@ class JsonApiRegressionTest extends JsonApiFunctionalTestBase {
   -  public function testPostToIncludeUrlDoesNotReturnIncludeFromIssue3026030() {
   ...
   -  public function testPatchToIncludeUrlDoesNotReturnIncludeFromIssue3026030() {
   

   ✅ And these revert #3026030: [regression] Includes are no longer respected when POSTing/PATCHing, so surely accidental too. Reverted.

3. +++ b/src/Controller/EntityResource.php
   @@ -852,8 +851,7 @@ class EntityResource {
   -    $links = ($links ?: new LinkCollection([]));
   -    $links = $links->withLink('self', $self_link);
   +    $links = ($links ?: new LinkCollection([]))->withLink('self', $self_link);
   
   +++ b/src/Revisions/ResourceVersionRouteEnhancer.php
   @@ -44,19 +44,19 @@ final class ResourceVersionRouteEnhancer implements EnhancerInterface {
   -  const CACHE_CONTEXT = 'url.query_args:resourceVersion';
   +  const CACHE_CONTEXT = 'url.query_args:' . self::RESOURCE_VERSION_QUERY_PARAMETER;
   ...
   -  const VERSION_IDENTIFIER_VALIDATOR = '/^[a-z]+[a-z_]*[a-z]+:[a-zA-Z0-9\-]+(:[a-zA-Z0-9\-]+)*$/';
   +  const VERSION_IDENTIFIER_VALIDATOR = '/^[a-z]+[a-z_]*[a-z]+'
   

   ✅ Similar problem: this undoes #3027501: [regression] Follow-up for #2995960 and #2992833: syntax errors in PHP 5.5 & 5.6. Reverted.

Hopefully I didn't make mistakes restoring those.

Now down to 14 files changed, 505 insertions(+), 816 deletions(-), still similarly awesome!

1. +++ b/src/Controller/EntityResource.php
   @@ -529,10 +529,9 @@ class EntityResource {
   -    $new_resource_identifiers = array_udiff(
   +    $new_resource_identifiers = ResourceIdentifier::diff(
   ...
   -      [ResourceIdentifier::class, 'compare']
   
   @@ -682,7 +681,7 @@ class EntityResource {
   -    $removed_resource_identifiers = array_uintersect($resource_identifiers, $original_resource_identifiers, [ResourceIdentifier::class, 'compare']);
   +    $removed_resource_identifiers = ResourceIdentifier::intersect($resource_identifiers, $original_resource_identifiers);
   

   👏 Nice new helpers!

2. +++ b/src/JsonApiResource/ResourceIdentifier.php
   @@ -241,20 +317,104 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   +   * For the purpose of comparing a previous relationship field with an updated
   +   * relationship field value (as in the case of a POST/PATCH request), every
   +   * ResourceIdentifier must have an arity, even if it is not duplicated.
   

   🤔 Is this actually true though? Because \Drupal\jsonapi\JsonApiResource\ResourceIdentifier::compare() contains this:

   // If both $a and $b have an arity, then return the order by arity.
       // Otherwise, they are considered equal.
       return $a->hasArity() && $b->hasArity()
         ? $a->getArity() - $b->getArity()
         : 0;
   

3. +++ b/src/JsonApiResource/ResourceIdentifier.php
   @@ -241,20 +317,104 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   +  protected static function toSerializableResourceIdentifiers(array $resource_identifiers) {
   

   😮 I think this is now officially the most complex function in the JSON:API codebase! But this is just existing complexity, now better isolated, and far better documented, so +1.

4. +++ /dev/null
   @@ -1,295 +0,0 @@
   -   * The official JSON:API JSON-Schema document requires that no two resource
   -   * identifier objects are duplicated.
   -   *
   -   * This adds an @code arity @endcode member to each object's
   -   * @code meta @endcode member. The value of this member is an integer that is
   -   * incremented by 1 (starting from 0) for each repeated resource identifier
   -   * sharing a common @code type @endcode and @code id @endcode.
   ...
   -   * @see http://jsonapi.org/format/#document-resource-object-relationships
   -   * @see https://github.com/json-api/json-api/pull/1156#issuecomment-325377995
   -   * @see https://www.drupal.org/project/jsonapi/issues/2864680
   -   */
   

   👎 We lost these valuable docs.

5. +++ b/src/Normalizer/ResourceIdentifierNormalizer.php
   @@ -0,0 +1,151 @@
   +  /**
   +   * The interface or class that this Normalizer supports.
   +   *
   +   * @var string
   +   */
   

   ✅ inheritdoc

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

   ✅ Same as parent, can be removed!

NW for points 2 and 4.

The main goal for this was to serve as a wrapper for other kind of relationships that are not entity references. This was initially used to create virtual relationships, but after all the reactors that's no longer possible. At least not this way.

#28: I'm not sure I understand. This issue is adding more domain logic to ResourceIdentifier. the only coupling to "entity" is ::fromEntity(). Non-entity relationships can still be modeled using ResourceIdentifier: by doing new ResourceIdentifier('something-custom', 'UUID'). What am I missing?

That being said, thanks to your comment, and my grepping for "entity" in that class, I did notice something! 😀 public static function fromEntity(EntityInterface $entity) has existed since #2997600: Resolve included resources prior to normalization. This patch is adding public static function toResourceIdentifier(EntityReferenceItem $item, $arity = NULL) and public static function toResourceIdentifiers(EntityReferenceFieldItemListInterface $items). Those should be named fromEntityReference() and fromEntityReferenceList()!

And … actually … perhaps that's your concern? That we're adding that "map entity-like stuff to ResourceIdentifier" logic to this class?

If that's not it, let's talk tomorrow about it?

diff --git a/src/BackwardCompatibility/tests/Traits/ContentModerationTestTrait.php b/src/BackwardCompatibility/tests/Traits/ContentModerationTestTrait.php
deleted file mode 100644
index 310bcfb..0000000
--- a/src/BackwardCompatibility/tests/Traits/ContentModerationTestTrait.php
+++ /dev/null

is out of scope here.

And all of the fixes I made in #25 are lost too. I don't know what's going on, but something seems very off with your dev env. I wanted to RTBC this, but can't. I wanted to quickly fix it for you, but the #25 interdiff (which I made manually) no longer works. So, marking NW and assigning back to you.

1. +++ b/src/JsonApiResource/EntityCollection.php
   @@ -54,13 +54,7 @@ class EntityCollection implements \IteratorAggregate, \Countable {
   +    assert(Inspector::assertAllObjects($resources, EntityInterface::class, LabelOnlyEntity::class, EntityAccessDeniedHttpException::class));
   

   👍 Together with #3015438: Wrap entity objects in a ResourceObject which carries a ResourceType, this becomes even simpler.

2. +++ b/src/JsonApiResource/ResourceIdentifier.php
   @@ -142,6 +165,24 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   +  public static function isParallel(ResourceIdentifier $a, ResourceIdentifier $b) {
   +    return sprintf('%s:%s', $a->getTypeName(), $a->getId()) === sprintf('%s:%s', $b->getTypeName(), $b->getId());
   +  }
   

   🤔 Wouldn't this be more clear as:

   return static::isDuplicate($a->withArity(0), $b->withArity(0));
   

   ? That would then show far more clearly that this is the same as ::isDuplicate() while keeping arity equal.

3. +++ b/src/JsonApiResource/ResourceIdentifier.php
   @@ -168,6 +209,49 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   +   * @param array $array1
   ...
   +   * @return array
   ...
   +   * @param array $array1
   

   🤔 Shouldn't all these typehints be \Drupal\jsonapi\JsonApiResource\ResourceIdentifier[]? toResourceIdentifiers() does this for example. Is there a reason I'm missing?

4. +++ b/src/JsonApiResource/ResourceIdentifier.php
   @@ -168,6 +209,49 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   +      function (ResourceIdentifier $a, ResourceIdentifier $b) {
   +        return static::compare($a, $b);
   +      }
   ...
   +      function (ResourceIdentifier $a, ResourceIdentifier $b) {
   +        return static::compare($a, $b);
   +      }
   

   🤔 Why have a closure here, why not just [static::class, 'compare']?

5. +++ b/src/JsonApiResource/ResourceIdentifier.php
   @@ -241,20 +334,104 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   +   * Gets a comparable array of ResourceIdentifiers.
   ...
   +   * For the purpose of comparing a previous relationship field with an updated
   +   * relationship field value (as in the case of a POST/PATCH request), every
   +   * ResourceIdentifier must have an arity, even if it is not duplicated.
   ...
   +  protected static function toComparableResourceIdentifiers(array $resource_identifiers) {
   

   🤔 You explained the rationale in more detail in #30. Thanks for that. But I think there's still a bit of confusion in here.

   ::toComparable… implies you're reading this array of resource identifiers for use with ::compare().

   But ::compare() ignores arity if it's absent on either operand. So for ::compare(), the hasArity() ? … : …->withArity() isn't necessary. It's only necessary because you want to use it for this particular purpose.

   I think that either:

   1. ::compare() should require an arity on all operands (then everything about this method makes unambiguous sense, plus ::isDuplicate() wouldn't return TRUE when one operand has an arity and the other doesn't)
   2. or this method should gain an additional required parameter indicating whether arity should be considered or not be renamed to indicate it's about comparisons including arity.

   I think the first makes most sense, it removes ambiguity from multiple places.

13 files changed, 306 insertions, 684 deletions.

I like this much better! 2.2 times more deletions than insertions! 👌

But I think we can do better still.

1. +++ b/src/JsonApiResource/ResourceIdentifier.php
   @@ -98,7 +118,7 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   -   * Returns a copy of the given ResourceIdentifier with the given arity.
   +   * Returns a copy of the ResourceIdentifier with the given arity.
   

   ✅ I dropped this change because it's clear enough as is and distracts from the purpose/scope of the issue.

2. +++ b/src/JsonApiResource/ResourceIdentifier.php
   @@ -111,6 +131,16 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   +  protected function withoutArity() {
   +    return new static($this->getTypeName(), $this->getId(), array_diff_key($this->getMeta(), array_flip([static::ARITY_KEY])));
   +  }
   +
   
   @@ -142,6 +175,24 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   +  public static function isParallel(ResourceIdentifier $a, ResourceIdentifier $b) {
   +    return static::compare($a->withoutArity(), $b->withoutArity()) === 0;
   +  }
   

   ✅👎 The new ::withoutArity() method wouldn't be necessary if we'd just use withArity(0) like I suggested in #38.2. Made that change, because it further simplifies this patch.

3. +++ b/src/JsonApiResource/ResourceIdentifier.php
   @@ -215,13 +266,20 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
        $property_name = static::getDataReferencePropertyName($item);
   ...
   +    $property_name = static::getDataReferencePropertyName($item);
   

   ✅👎 This is repeating something. Fixed.

4. +++ b/src/JsonApiResource/ResourceIdentifier.php
   @@ -215,13 +266,20 @@ class ResourceIdentifier implements ResourceIdentifierInterface {
   +    $field = $item->getParent();
   +    assert($field instanceof FieldItemListInterface);
   

   ✅👎 These lines can be removed. The only thing we do with $field is asserting it's of a certain type. But given the type restriction on the $item function argument, this is guaranteed to be true anyway. So … removed this.

13 files changed, 291 insertions, 683 deletions. — 2.3 times as many deletions as insertions 👍

My chief remaining concern is this:

+++ b/src/Controller/EntityResource.php
@@ -682,7 +682,7 @@ class EntityResource {
-    $original_resource_identifiers = ResourceIdentifier::toResourceIdentifiers($field_list);
+    $original_resource_identifiers = ResourceIdentifier::toResourceIdentifiersWithArityRequired($field_list);

Could we not change this name, to further reduce the amount of change? The name change does more accurately describe what this method does though. (The confusing bit is that toResourceIdentifiers() in HEAD is effectively renamed to toResourceIdentifiersWithArityRequired(), but toResourceIdentifiers() is simultaneously reintroduced, for the public representation of resource identifiers, where single-occurrence relationships have their arity omitted.)

The answer to that question is "no", at least as long as we keep \Drupal\jsonapi\JsonApiResource\ResourceIdentifier::compare()'s confusing logic:

    return $a->hasArity() && $b->hasArity()
      ? $a->getArity() - $b->getArity()
      : 0;

And changing that is definitely out of scope here.

(I tried to move the majority of changes in the ResourceIdentifier class out of here, to do it in a separate issue, but I don't see how that gets us anywhere: perhaps_do_separately-3024896-do-not-test.patch.)

Despite that portion, which I'd like to be simpler, I think this is still a solid step forward:

1. The significant reduction in lines of code speaks for itself.
2. The code we are left with, maps closer to https://jsonapi.org/format, which makes it easier for other people to contribute.
3. Fewer concepts/classes. The code we are left with has is own domain logic: ResourceIdentifier changes it from an anemic domain model to one that contains all of the associated behaviors/logic.

So: RTBC.

@gabesullice I think it's now up to you to provide a patch for JSON:API Extras — \Drupal\jsonapi_extras\Normalizer\RelationshipItemNormalizer::normalize() is impacted.

Great cleanup here! This isn't necessarily a commit blocker for #2843147: Add JSON:API to core as a stable module, but IMO it's a pretty strong nice to have, so making it a child issue for now.

Sorry, wrong parent issue in #53. This is the one I meant.

As of #53, this apparently has become a semi-blocker to JSON:API getting committed to core. So, rather than waiting for @gabesullice (who's currently asleep), I'm doing #52 in his place.

It's only been a few weeks that JSON:API Extras has had RelationshipItemNormalizer — since #3025284: Allow field enhancers for relationships. Unfortunately, without test coverage.

On it …

Per #55, I had to do manual testing to create a patch for JSON:API Extras to keep it compatible. So I did.

Updating #3025284: Allow field enhancers for relationships mostly is easy, with one exception: JSON:API Extras' Field Enhancers are designed to be configured per field. Meaning that it needs to know the field. In HEAD, it's able to go and introspect the Typed Data data (which by the way means that it conflicts with #28: Field Enhancers are tightly coupled to entity references), but with the patch in this issue that's no longer possible.

Fortunately, a one-line addition to the current patch makes that possible again. So this interdiff is to make it possible to keep JSON:API Extras working like it does today.

Any chance we can test Consumer Image Styles 3.x before committing this?

Thanks for this! +1

I broke tests in Consumer Image Styles… :-/

#56 was incomplete; found by running JSON:API Extras' tests. Tiny addition on commit attached.

_{BTW, consumer_image_styles seems to work fine here… its test passed with this + #3032451-10: Compatibility with upcoming JSON:API 2.2 release: JSON:API Extras 3.4! 🙂}

This also allowed me to close #3029043: EntityReferenceFieldNormalizer can improperly normalize items when a reference to an internal entity is involved.!

Ugh, I just noticed that d.o made me the author of this issue simply because of my rerolls :( d.o--

@gabesullice, in the next path that I'm the primary author of, mark yourself as the author, to counterbalance it :)

This broke JSON:API Extras in #3048341: Enhancers are not passed context