• \Drupal\jsonapi\Normalizer\EntityReferenceFieldNormalizer::normalize() constructs a Relationship object and then normalizes that
  • \Drupal\jsonapi\Normalizer\RelationshipNormalizer::normalize() then does the actual normalization
  • the Relationship object is only ever created by EntityReferenceFieldNormalizer::normalize()

and…

  • \Drupal\jsonapi\Normalizer\RelationshipNormalizer::denormalize() throws an exception: it doesn't support denormalization
  • \Drupal\jsonapi\Normalizer\EntityReferenceFieldNormalizer::denormalize() does the actual denormalization

This makes for a very hard to understand architecture. Even though I think the intent was laudable: AFAICT the intent was to model the Relationship value object after the "relationship" described by the JSON API spec. So having a value object for that of course would make things simpler to understand.

This just doesn't seem to have worked out in practice.

It seems that at this point, it'd be simpler to merge RelationshipNormalizer into EntityReferenceFieldNormalizer. We can still keep the Relationship(Item)NormalizerValue value objects though!

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

See #2940342-45: Cacheability metadata on an entity fields' properties is lost.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

@Wim Leers the relationship objects where introduced in order to accomodate for non EntityReference relationships.

AFAIK there is still value in that. Even though we are not taking advantage of it yet, we may want to have autocomputed relationships (for instance reverse relationships, relationships to the entity type config entity, …).

Is this a feature we want to stop supporting? (or rather ban that from our future, since we don't support them now).

gabesullice’s picture

gabesullice
Primary language English
commented

Is this a feature we want to stop supporting? (or rather ban that from our future, since we don't support them now).

AFAICT, there's no "feature" there right now. Just an architectural "opening" for the future. Is that right?

I would be vehemently opposed to "banning" it from our future though. I absolutely think this will only become more necessary in the medium to long term of future Drupal development.

It seems that at this point, it'd be simpler to merge RelationshipNormalizer into EntityReferenceFieldNormalizer.

If we want to remove it for now, I would be indifferent, but I'm fairly certain we will be adding it back in sometime in the future (though I can't say if it will be in 2 months or 24).

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I wrote:

AFAICT the intent was to model the Relationship value object after the "relationship" described by the JSON API spec. So having a value object for that of course would make things simpler to understand.

Emphasis added just now.

I agree it's a good idea in principle. But right now, it's causing more confusion than anything. Even more so because:

class Relationship implements AccessibleInterface {
…
  /**
   * {@inheritdoc}
   */
  public function access($operation, AccountInterface $account = NULL, $return_as_object = FALSE) {
    // Hard coded to TRUE. Revisit this if we need more control over this.
    return TRUE;
  }
…
}

i.e. right now the very fact that this level of indirection/point of abstraction is incomplete is dangerous in that we could easily mislead ourselves.

Also, just like premature optimization is the root of all evil, I'd like to say premature abstraction is the root of all evil.

I've learned the hard way in core that one should only ever add layers of abstraction when there's >=2 consumers. Fortunately, this is marked @internal, so at least we're doing better than core in that respect :)

Summarized:

  1. I like the concept!
  2. But it's an unused layer of abstraction.
  3. I'd much rather see the concept be added back in the future when we do have >=2 consumers of it.
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

AFAICT, there's no "feature" there right now. Just an architectural "opening" for the future. Is that right?

Yes. Sorry for the weird wording.

I would be vehemently opposed to "banning" it from our future though. I absolutely think this will only become more necessary in the medium to long term of future Drupal development.

That's the reason why I don't want to remove it.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I don't understand why you wouldn't want to remove an abstraction that is clearly incomplete and has only a single consumer. Why are you opposed to reintroducing it in the future, when there is a better understanding of how this should work, because there would be >=2 consumers?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Closed (duplicate)

Turns out the confusion described in the issue summary of this issue actually ended up causing problems: see #2955615-11: Field properties are not being denormalized. Don't worry, no seismic shifts in functionality. Only moving logic from one normalizer to another! And in doing so, this actually is untangled. At least, untangled enough.

That patch is currently RTBC. Once that lands, there will be nothing left to untangle. The discussion in this issue was trending towards bigger architecture/elegance questions. Which are nice questions to dig into, and ambitions to aspire to, but they're not essential. So, closing this issue.