Clean-up: JsonApiDocumentTopLevelNormalizer and children to prep for further clean-ups

1. +++ b/jsonapi.services.yml
   @@ -38,6 +38,10 @@ services:
   +  serializer.normalizer.data.jsonapi:
   +    class: Drupal\jsonapi\Normalizer\DataNormalizer
   

   🤔 First thought: adding a new normalizer seems unrelated to cleaning up JsonApiDocumentTopLevelNormalizer ?

2. +++ b/src/Controller/TemporaryJsonapiFileFieldUploader.php
   @@ -336,8 +335,8 @@ class TemporaryJsonapiFileFieldUploader {
   -      // Close the input file stream since we can't proceed with the upload.
   -      // Don't try to close $temp_file since it's FALSE at this point.
   +      // Close the file streams.
   +      fclose($temp_file);
          fclose($file_data);
          $this->logger->error('Temporary file "%path" could not be opened for file upload.', ['%path' => $temp_file_path]);
          throw new HttpException(500, 'Temporary file could not be opened');
   @@ -464,11 +463,7 @@ class TemporaryJsonapiFileFieldUploader {
   
   @@ -464,11 +463,7 @@ class TemporaryJsonapiFileFieldUploader {
        $settings = $field_definition->getSettings();
    
        // Cap the upload size according to the PHP limit.
   -    // @todo Remove when we stop supporting Drupal 8.5 and 8.6.
   -    $max_bytes = floatval(\Drupal::VERSION) < 8.7
   -      ? file_upload_max_size()
   -      : Environment::getUploadMaxSize();
   -    $max_filesize = Bytes::toInt($max_bytes);
   +    $max_filesize = Bytes::toInt(file_upload_max_size());
   

   ✅ These surely were added to this patch accidentally, removing.

3. +++ b/jsonapi.services.yml
   @@ -38,6 +38,10 @@ services:
   +  serializer.normalizer.data.jsonapi:
   +    class: Drupal\jsonapi\Normalizer\DataNormalizer
   +    tags:
   +      - { name: jsonapi_normalizer }
   
   +++ b/src/Normalizer/DataNormalizer.php
   @@ -0,0 +1,33 @@
   +  protected $supportedInterfaceOrClass = Data::class;
   ...
   +  public function normalize($object, $format = NULL, array $context = []) {
   

   🤔 It's not crystal-clear after reading the patch why this new normalizer is necessary.

4. +++ b/src/JsonApiResource/JsonApiDocumentTopLevel.php
   @@ -43,17 +43,24 @@ class JsonApiDocumentTopLevel {
      /**
       * The includes to normalize.
       *
   -   * @var \Drupal\jsonapi\JsonApiResource\Data
   +   * @var \Drupal\jsonapi\JsonApiResource\IncludedData
       */
      protected $includes;
   

   🥳

5. +++ b/src/JsonApiResource/NullData.php
   @@ -11,7 +11,7 @@ namespace Drupal\jsonapi\JsonApiResource;
   -class NullData extends Data {
   +class NullData extends IncludedData {
   

   🤔 Shouldn't this then be renamed NullIncludesData?

6. +++ b/src/JsonApiResource/ResourceObjectData.php
   @@ -31,4 +31,34 @@ class ResourceObjectData extends Data {
   +  public function getAccessible() {
   ...
   +      if (!$resource_object instanceof EntityAccessDeniedHttpException) {
   ...
   +  public function getOmissions() {
   ...
   +      if ($resource_object instanceof EntityAccessDeniedHttpException) {
   

   Genius additions. 👏 I wish I had pointed this out :) This is indeed a perfect two-way split, thanks to this in the constructor: assert(Inspector::assertAllObjects($data, ResourceObject::class, EntityAccessDeniedHttpException::class));.

7. +++ b/src/Normalizer/FieldNormalizer.php
   @@ -38,7 +37,7 @@ class FieldNormalizer extends NormalizerBase implements DenormalizerInterface {
   -      ? array_shift($normalized_items) ?: new CacheableNormalization(new CacheableMetadata(), NULL)
   +      ? array_shift($normalized_items) ?: CacheableNormalization::permanent(NULL)
   

   🤔 Can this not instead use CacheableOmission?

8. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -175,18 +173,43 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   +      $document['meta'] = CacheableNormalization::aggregate(['omitted' => $normalized_omissions])->map(function ($normalization) use ($object) {
   +        return array_merge($normalization, $object->getMeta());
   
   @@ -222,186 +252,76 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   +    return $normalized_relationship->map(function ($normalization) {
   +      return $normalization['data'];
   +    });
   
   +++ b/src/Normalizer/Value/CacheableNormalization.php
   @@ -52,6 +66,39 @@ class CacheableNormalization implements CacheableDependencyInterface {
   +  public function map(callable $mapper) {
   +    $additional_cacheability = new CacheableMetadata();
   +    $mapped = new static($this, $mapper($this->getNormalization(), $additional_cacheability));
   +    return $mapped->withCacheableDependency($additional_cacheability);
   +  }
   

   🤔 Fascinating addition. But AFAICT that $additional_cacheability is not yet used anywhere. It may be better to not yet add this method in this issue, it seems to expand the scope unnecessarily, and given the second parameter is used, I'm not yet fully confident this will actually be used as intended?

9. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -201,13 +224,20 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   +    $normalized_values = array_map(function (HttpExceptionInterface $exception) use ($format, $context) {
   ...
   +    foreach ($normalized_values as $normalized_error) {
   +      $cacheability->addCacheableDependency($normalized_error);
   +      $errors = array_merge($errors, $normalized_error->getNormalization());
   +    }
   

   🤔 This looks like we're re-normalizing $normalized_values. I think this is confusing.

   It feels like this is impossible to understand without stepping through it with a debugger?

10. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
    @@ -222,186 +252,76 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
    -    return new CacheableNormalization($cacheability, $rasterized);
    +    return new CacheableNormalization($cacheability, $omitted);
    

    🤔 If I read this, I immediately think "why is this not using CacheableOmission?". But that's only for "NULL normalizations".

    This is where our naming could still use some love.

    Perhaps renaming $omitted to $communicate_omissions or something like that? (I still don't like that name, but that concept is what we should name it after.)

    Oh, how about $omission_links?

11. +++ b/src/Normalizer/ResourceIdentifierNormalizer.php
    @@ -58,7 +57,7 @@ class ResourceIdentifierNormalizer extends NormalizerBase implements Denormalize
    -    return new CacheableNormalization(new CacheableMetadata(), $normalization);
    +    return CacheableNormalization::permanent($normalization);
    
    +++ b/src/Normalizer/ResourceObjectNormalizer.php
    @@ -103,7 +97,7 @@ class ResourceObjectNormalizer extends NormalizerBase {
    -      return new CacheableNormalization(new CacheableMetadata(), $field);
    +      return CacheableNormalization::permanent($field);
    
    +++ b/src/Normalizer/Value/CacheableNormalization.php
    @@ -42,6 +42,20 @@ class CacheableNormalization implements CacheableDependencyInterface {
    +  public static function permanent($normalization) {
    

    👍🤔 Nice addition, but makes this patch harder to review and technically out-of-scope. How about we split this off into a separate issue and land that first?

12. +++ b/src/Normalizer/ResourceObjectNormalizer.php
    @@ -58,20 +58,14 @@ class ResourceObjectNormalizer extends NormalizerBase {
    +      'attributes' => CacheableNormalization::aggregate(array_diff_key($normalizer_values, array_flip($relationship_field_names)))->omitIfEmpty(),
    +      'relationships' => CacheableNormalization::aggregate(array_intersect_key($normalizer_values, array_flip($relationship_field_names)))->omitIfEmpty(),
    +      'links' => $this->serializer->normalize($object->getLinks(), $format, $context)->omitIfEmpty(),
    
    +++ b/src/Normalizer/Value/CacheableNormalization.php
    @@ -52,6 +66,39 @@ class CacheableNormalization implements CacheableDependencyInterface {
    +  public function omitIfEmpty() {
    +    return empty($this->normalization) ? new CacheableOmission($this) : $this;
    +  }
    

    👍 🤔 Hm, this means all these might use CacheableOmission. Does that make sense? Yes, it does, because it's not tied to "primary data omissions", \Drupal\jsonapi\Normalizer\Value\CacheableOmission is already generalized away from that (original) use case in HEAD.

    The more I think about it, the more elegant I think this is.

    This snippet of code is a huge step forward actually in terms of understandability! 🥳

#13: good answers, thanks!

Final round:

1. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -175,18 +173,44 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   +    $document['jsonapi'] = CacheableNormalization::permanent([
   ...
   +        $document['data'] = $this->normalizeEntityReferenceFieldItemList($object, $format, $context);
   ...
   +      $document['included'] = $this->serializer->normalize($object->getIncludes(), $format, $context)->omitIfEmpty();
   ...
   +      $document['meta'] = (new CacheableNormalization($normalized_omissions, $meta))->omitIfEmpty();
   

   👏🥳 I like how we're now truly explicitly constructing a document, and the top-level keys prescribed by https://jsonapi.org/format are clearly here: that makes it crystal clear even to any developer reading this source code what's going on here :)

   (Which definitely cannot be said of the code in HEAD.)

   This is also the heart of the patch.

2. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -175,18 +173,44 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   +        // Always deal with a collection.
   +        $document['data'] = $this->serializer->normalize($data, $format, $context);
   

   I don't think this comment is accurate anymore. Could be removed on commit.

   (Also: this is where DataNormalizer is used, which together with #13.3 forms the answer to my scepticism in #12.1: the DataNormalizer is there to normalize whichever primary data the generated JSON:API document contains.)

This was the only issue with "Needs follow-up" including hyphen.
Consolidating tags.