[PP-1] Do not overload Symfony's normalize() method to return objects; define a new interface for that.

Nice, thanks @effulgentsia! This is essentially the direction I saw us going. You beat me to making the issue and writing the patch :) It's nice to see it validated. So ofc, I have no objection to the general approach.

Just some mid-level stuff:

1. +++ b/src/Normalizer/CacheableNormalizerInterface.php
   @@ -0,0 +1,40 @@
   +interface CacheableNormalizerInterface extends BaseCacheableNormalizerInterface {
   

   What does extension buy us?

2. +++ b/src/Normalizer/CacheableNormalizerInterface.php
   @@ -0,0 +1,40 @@
   +  public function normalizeToObject($object, $format = NULL, array $context = []);
   

   This is nit-picky, but I feel a little weird about this name (I'm sorry, I'm sure you struggled to come up with one because it's hard! Cache invalidations and naming things, right? Double whammy).

   My complaint is that "normalization" is about taking objects and making them not objects.

   So, what about toCacheableNormalization($object, $format, $context)?

3. +++ b/src/Normalizer/NormalizerBase.php
   @@ -27,4 +40,23 @@ abstract class NormalizerBase extends SerializationNormalizerBase {
   +  public function normalize($object, $format = NULL, array $context = []) {
   +    $cacheable_normalization = $this->normalizeToObject($object, $format, $context);
   +
   +    // It is preferred for callers that want the cacheability information to
   +    // call the normalizeToObject() method themselves to get it. However, for
   

   I think a \LogicException instead of a "BC layer" makes sense here since our normalizers are all internal.

4. +++ b/src/Serializer/Serializer.php
   @@ -63,6 +64,38 @@ final class Serializer extends SymfonySerializer {
   +  /**
   +   * Returns a matching normalizer.
   +   *
   +   * This duplicates \Symfony\Component\Serializer\Serializer::getNormalizer()
   ...
   +  private function getNormalizer($data, $format, array $context) {
   

   Instead of overriding this and excluding non-compliant normalizers, I wonder if it would be better to shim them. I have some pseudo-code in #3028080-6: Add CacheableNormalization for Normalizer to return Normalized value and Cacheablity that shows what I'm thinking about. However, since this issue proposes a new interface, I think it could be further simplified.

1. +++ b/src/Normalizer/CacheableNormalizerInterface.php
   @@ -2,18 +2,22 @@
   + * Unlike \Drupal\serialization\Normalizer\CacheableNormalizerInterface, this
   + * allows for returning the cacheability information in-band with the
   + * normalization, rather than out-of-band via an object within $context.
   

   👌Excellent comment!

2. +++ b/src/Normalizer/CacheableNormalizerInterface.php
   @@ -36,5 +40,5 @@
   -  public function normalizeToObject($object, $format = NULL, array $context = []);
   +  public function getCacheableNormalization($object, $format = NULL, array $context = []);
    
   

   You used "get" instead of @gabesullice's suggested "to". Both work for me.

3. +++ b/src/Normalizer/NormalizerBase.php
   @@ -44,19 +44,7 @@
   -    // It is preferred for callers that want the cacheability information to
   ...
   +    throw new \LogicException('JSON:API normalizers must be called with getCacheableNormalization() rather than normalize().');
   

   👍Strong +1, this is also the biggest concern I had, and @gabesullice already pointed it out :)

4. +++ b/src/Serializer/Serializer.php
   @@ -67,8 +68,11 @@
   -  public function normalizeToObject($data, $format = NULL, array $context = []) {
   -    return $this->getNormalizer($data, $format, $context)->normalizeToObject($data, $format, $context);
   +  public function getCacheableNormalization($data, $format = NULL, array $context = []) {
   +    if ($normalizer = $this->getNormalizer($data, $format, $context)) {
   +      return $normalizer->getCacheableNormalization($data, $format, $context);
   +    }
   +    throw new NotNormalizableValueException('An unexpected value could not be normalized.');
      }
   

   🤔 When does this exception occur? Why do we have this new edge case?

5. +++ b/src/Serializer/Serializer.php
   @@ -90,7 +94,7 @@
      private function getNormalizer($data, $format, array $context) {
        foreach ($this->normalizers as $normalizer) {
   -      if ($normalizer instanceof CacheableNormalizerInterface && $normalizer->supportsNormalization($data, $format, $context)) {
   +      if ($normalizer instanceof NormalizerInterface && $normalizer->supportsNormalization($data, $format, $context)) {
            return $normalizer;
          }
        }
   

   🤔This is now identical to the inherited function, so we can delete this completely. Also, it doesn't match the docblock anymore. I also don't understand the justification for this in #10.

6. 🤔 #13: we're doing that only for Drupal\Tests\jsonapi\Kernel\Normalizer\JsonApiDocumentTopLevelNormalizerTest::testNormalizeException() though. Why not just update the test to not use ::serialize() but getCacheableNormalization()?
7. ⚠️ This will require changes in JSON:API Extras. That's okay, especially if a core committer feels this needs to be clarified. Just calling it out. When this patch is committed, we should also provide a patch for JSON:API Extras.

⚠️ This will require changes in JSON:API Extras. That's okay, especially if a core committer feels this needs to be clarified. Just calling it out. When this patch is committed, we should also provide a patch for JSON:API Extras.

Thanks for calling this out. I'm starting to believe that before we get this into core we should formalize these implicit PHP APIs.

This interdiff tries to preserve my goal for the Serializer to be a "drop-in" replacement for the Symfony serializer.

Preserving that as much as possible means that if our normalizers are ever to be reintegrated with the core serialization system, we could just delete our Serializer. I still hope that will one day happen.

Ofc, the getCacheableNormalization method will make our normalizers forever "different". However, there's a core issue to add this concept to the serialization module. Once/If that lands, then we'd be just as close to reintegrating as we were before.

Now that our serializer has the getCacheableNormalization method that uses the fallback serializer, I was able to get rid of some complexity in a couple normalizers too. I think those changes are fairly straightforward and can be included in this patch.

I still hope that will one day happen.

+1

Once/If that lands, then we'd be just as close to reintegrating as we were before.

Indeed.

EDIT: The 6.44KB interdiff.txt is the right one (ignore the other).

1. +++ b/src/Serializer/Serializer.php
   @@ -56,13 +62,35 @@ final class Serializer extends SymfonySerializer implements CacheableNormalizerI
   -    if ($this->fallbackNormalizer->supportsNormalization($data, $format, $context)) {
   -      return $this->fallbackNormalizer->normalize($data, $format, $context);
   

   Trying to normalize a traversable before giving the fallback normalizer a chance to normalize it means that there was a regression of the fix here: #2958166: Traversable Object with custom normalizer can't be right normalized

2. +++ b/src/Serializer/Serializer.php
   @@ -56,13 +62,35 @@ final class Serializer extends SymfonySerializer implements CacheableNormalizerI
   +      foreach ($normalized as $key => $item) {
   +        if ($item instanceof CacheableOmission) {
   +          static::applyContextCacheability($context, $item);
   +          unset($normalized[$key]);
   +        }
   +        elseif ($item instanceof CacheableNormalization) {
   +          static::applyContextCacheability($context, $item);
   +          $normalized[$key] = $item->getNormalization();
   +        }
   +      }
   

   This can be simplified by using CacheableNormalization::aggregate().

1. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -99,19 +98,14 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer {
   -      // This normalizer leaves JSON:API normalizer land and enters the land of
   -      // Drupal core's serialization system. That system was never designed with
   -      // cacheability in mind, and hence bubbles cacheability out of band. This
   -      // must catch it, and pass it to the value object that JSON:API uses.
   -      // @see \Drupal\jsonapi\Normalizer\FieldItemNormalizer::normalize()
   -      $context[CacheableNormalizerInterface::SERIALIZATION_CONTEXT_CACHEABILITY] = new CacheableMetadata();
   
   +++ b/src/Normalizer/FieldItemNormalizer.php
   @@ -41,20 +40,16 @@ class FieldItemNormalizer extends NormalizerBase implements DenormalizerInterfac
   -    $context[CacheableNormalizerInterface::SERIALIZATION_CONTEXT_CACHEABILITY] = new CacheableMetadata();
   
   +++ b/src/Serializer/Serializer.php
   @@ -54,13 +61,74 @@ final class Serializer extends SymfonySerializer {
   +  public function getCacheableNormalization($data, $format = NULL, array $context = []) {
   +    if ($normalizer = $this->getNormalizer($data, $format, $context)) {
   +      return $normalizer->getCacheableNormalization($data, $format, $context);
   +    }
   +    // Rather than directly calling this serializer's normalizers, call the
   +    // normalize() method and wrap its result in a CacheableNormalization.
   +    // When this serializer could support normalization, this trades extra work
   +    // for less code complexity.
   +    if (!isset($context[CacheableNormalizerInterface::SERIALIZATION_CONTEXT_CACHEABILITY])) {
   +      $context[CacheableNormalizerInterface::SERIALIZATION_CONTEXT_CACHEABILITY] = new CacheableMetadata();
   +    }
   +    $normalized = $this->normalize($data, $format, $context);
   +    return new CacheableNormalization($context[CacheableNormalizerInterface::SERIALIZATION_CONTEXT_CACHEABILITY], $normalized);
   +  }
   

   🤔 Nice, all this complexity is now moved out of a the two field-level normalizers and into a single place! 🎉

   _{But … this comes with some a kind of downside too: this now transparently allows any normalizer to descend into "core normalizers". That's purely theoretical though, since we don't allow any additional normalizers to be defined. But it is less clear/explicit which normalizers allow leaving the JSON:API normalizers and "reach into core normalizers": the @FieldType-level normalizers of JSON:API do this, to get the @DataType-level normalizers.}

2. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -181,6 +181,22 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
      public function normalize($object, $format = NULL, array $context = []) {
   +    // For the top-level document, allow invocation of normalize(), so that the
   +    // serializer's serialize() method can function. In this case, transfer the
   +    // cacheability information to the out-of-band cacheability object.
   +    $cacheable_normalization = $this->getCacheableNormalization($object, $format, $context);
   +    if (isset($context[static::SERIALIZATION_CONTEXT_CACHEABILITY])) {
   +      /** @var \Drupal\Core\Cache\RefinableCacheableDependencyInterface $cacheability */
   +      $cacheability = $context[static::SERIALIZATION_CONTEXT_CACHEABILITY];
   +      $cacheability->addCacheableDependency($cacheable_normalization);
   +    }
   +    return $cacheable_normalization->getNormalization();
   +  }
   

   🤔 This was done in #13. I don't think we want this. It feigns compatibility with serialize(). But … nothing outside the jsonapi module will end up calling this. And the JSON:API module only normalizes this object in \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber::renderResponseBody(), and in there it does the normalization and encodings steps separately.

   In other words: #13 did this just to satisfy \Drupal\Tests\jsonapi\Kernel\Normalizer\JsonApiDocumentTopLevelNormalizerTest::testNormalizeException().

   Worse: the cacheability stuff would simply be ignored, because the JSON:API module is the only caller and it does NOT respect SERIALIZATION_CONTEXT_CACHEABILITY for its own normalizers, only for @DataType-level normalizers.

   Finally, as of #19, JSON:API's Serializer service already takes care of this anyway (the code quoted in the previous point).

   So: simply deleted this! ✅

3. +++ b/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -96,7 +96,7 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
   -   * @param \Symfony\Component\Serializer\SerializerInterface $serializer
   +   * @param \Drupal\jsonapi\Serializer\Serializer $serializer
   
   @@ -105,7 +105,7 @@ class ResourceResponseSubscriber implements EventSubscriberInterface {
   -  protected function renderResponseBody(Request $request, ResourceResponse $response, SerializerInterface $serializer, $format) {
   +  protected function renderResponseBody(Request $request, ResourceResponse $response, Serializer $serializer, $format) {
   

   🤔 JsonApiExtrasFunctionalTest fails with this patch applied, because of its SerializerDecorator implementing SerializerInterface and this patch changing the typehint to not require SerializerInterface but \Drupal\jsonapi\Serializer\Serializer. Worse, since this patch is also marking that class final, there is no work-around for JSON:API Extras possible.

   I think we want to revert this change.

#23.3

I think we want to revert this change.

+1. I think that may have snuck in from my playing with #3007431: Make more things `final`.

#3036904: Decorate JSON:API's normalizers and fake its namespace so JSON:API can eliminate its Serializer's fallback behavior. landed, which unblocked #3032259: Remove the JSON:API serializer's fallback behavior and inject the core serializer in the field item normalizer, which means this is now down to one blocker.