Spec Compliance: Impossible to include related resources when relationship field is not in a request's sparse fieldset

Confirmed the proposed resolution. This will mean lifting this logic out of \Drupal\jsonapi\Normalizer\RelationshipItemNormalizer::normalize():

    $host_field_name = $relationship_item->getParent()->getPropertyName();
    if (!empty($context['include']) && in_array($host_field_name, $context['include'])) {
      $context = $this->buildSubContext($context, $target_entity, $host_field_name);
      $entity_and_access = EntityResource::getEntityAndAccess($target_entity);
      $included_normalizer_value = $this
        ->jsonapiDocumentToplevelNormalizer
        ->buildNormalizerValue($entity_and_access['entity'], $format, $context);
    }
    else {
      $included_normalizer_value = NULL;
    }

and running it independently of relationship normalization.

Observation: I'm not sure how/if this will be possible, because this (only includes of visible relationships) has been pretty much an essential basic assumption in the entire normalization architecture. This is what ValueExtractorInterface is designed for: a tree of objects implementing this interface allows includes to bubble up from the deepest level to the top level, and to the response.

The solution proposed in the IS would change this fundamental assumption. I believe it'd be simpler, or at least more maintainable for now, to stick to the current design.

I believe that's doable by converting RelationshipNormalizerValue value objects to some new object, which has no values to be rasterized, but only includes. That'd be perfectly consistent with what we already have.

Patch attached. This patch works! We can prove that by enabling the test coverage that #2953321: Comprehensive JSON API integration test coverage phase 5: nested includes and sparse field sets added. In doing so, I noticed that it's not wise to list preferred_langcode as one of the fields in the sparse fieldset. Because that particular field is only viewable by the user itself, which means that the access checking adds the user cache context. Replacing it with created still tests the same principle, while removing the need for complex test alterations.

I spent >30 minutes trying to reproduce the test failures; but then I suddenly could. I must've done something wrong, but don't know what!

1. I can see that. I just tried to minimize for change. The entirety of \Drupal\jsonapi\Normalizer\Value\EntityNormalizerValue::__construct() is pretty hard to follow IMHO.

   I specifically did instanceof INTERFACE rather than instanceof CLASS to maximize contrib compatibility. But I agree that your proposal is better.

   However… the irony is that I find your code much harder to follow :P There now is a conditional early return (well, continue), and the same conditions reappear later. And in doing so, you introduced a subtle bug that causes the test failures: a field that is not listed in the sparse fieldset and is a relationship field will never enter the if-test with the continue statement, and if no ?include is specified, $has_includes will always be false, which means that it'll be included inappropriately.
   This is the danger of repeating complex conditions multiple times. I prefer the structure I had before, because it stresses how exceptional it is: it is only in an extremely rare case that we need to wrap/decorate a normalization value we get back!

   That doesn't mean I don't see your point! I think there's another way to achieve DRY. See interdiff. This does result in some unnecessary work, but optimizing that seems premature optimization TBH :) Simplicity & correctness first, performance second!

2. 🙂 I was also pleasantly surprised!
3. ➕ WFM

Nope, that was fine :) And it's fascinating how different things can be simpler to different people!

Pushed!