[Serializer] Add ChainNormalizer and ChainDenormalizer #52900

Nyholm · 2023-12-05T13:36:05Z

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	yes
Issues
License	MIT

This PR is less of a feature and more refactoring.
Our Serializer currently implements 5 interfaces. You may serialize and normalize (and reverse). There are also a lot of logic to figure out context, error messages and what normalizer to use.

This PR suggest to separate responsibilities a bit by splitting the normalizing and denormalizing into ChainNormalizer and ChainDenormalizer.

I found this work when I was creating custom denormalizers. A low level class like MyBlogDenormalizer should not have a dependency on the high level Serializer. It would make more sense to use a ChainDenormalizer. (Yes, there is still an ugly circular reference).

I've marked SerializerAwareInterface as deprecated for this reason. Currently, SerializerAwareInterface only works if you combine it with NormalizerInterface, DenormalizerInterface or EncoderInterface. I deprecate it because one should use NormalizerAwareInterface instead.

Nyholm · 2023-12-05T15:55:15Z

Hm.. Im considering renaming this to NormalizerAggregate instead. Similar to CacheWarmerAggregate.

EDIT:
Changed my mind. We do have ChainEncoder and ChainDecoder at the moment. Using "Chain" is good here.

mtarld

I really think that's a good idea to have a better separation between normalization and serialization, thanks for pushing it forward!

src/Symfony/Component/Serializer/Debug/TraceableEncoder.php

src/Symfony/Component/Serializer/DependencyInjection/SerializerPass.php

src/Symfony/Component/Serializer/Encoder/XmlEncoder.php

src/Symfony/Component/Serializer/Serializer.php

src/Symfony/Component/Serializer/Normalizer/ChainDenormalizerInterface.php

src/Symfony/Component/Serializer/Normalizer/ChainDenormalizer.php

src/Symfony/Component/Serializer/Serializer.php

Nyholm · 2023-12-11T12:23:31Z

Wohoo. All tests are passing.

While working on this PR I found an issues like #52992. Ie some code assume a certain behaviour without any contract.
One example that we have in 7.0 right now is that you can pass $context[DenormalizerInterface::COLLECT_DENORMALIZATION_ERRORS] to Serializer::denormalize() and never get a PartialDenormalizationException because you have used the wrong normalizers.

This is something we should fix and this PR is a tiny step towards that direction.

Nyholm · 2024-01-09T11:53:42Z

PR is rebased and all tests are green again.

Nyholm · 2024-01-31T20:15:10Z

PR is rebased =)

Nyholm · 2024-02-12T04:01:27Z

@mtarld and @chalasr Can you give another review on this please?

mtarld · 2024-02-12T08:34:14Z

src/Symfony/Bundle/FrameworkBundle/composer.json

@@ -59,7 +59,7 @@
        "symfony/scheduler": "^6.4.4|^7.0.4",
        "symfony/security-bundle": "^6.4|^7.0",
        "symfony/semaphore": "^6.4|^7.0",
-        "symfony/serializer": "^6.4|^7.0",
+        "symfony/serializer": "^7.1",


Is that bump really needed? AFAIK, we try to avoid as much as possible to bump dependencies (see #53160 (comment))

(same for other composer.json files)

FrameworkBundle often bumps the min version instead of trying to support multiple versions of the service definitions, for simplicity.
This is different for other package, which can be reused outside the fullstack framework and for which supporting compatibility with 2 different major versions of Symfony reduces the risk of dependency hell in those projects (the lower level the package is, the more useful this is).

Thank you.

I've bumped version on FrameworkBundle and HttpKernel. I have reverted the other packages and make sure they (still) support both 6.4 and 7.2.

src/Symfony/Component/Serializer/Serializer.php

...y/Component/HttpKernel/Tests/Controller/ArgumentResolver/RequestPayloadValueResolverTest.php

src/Symfony/Component/Serializer/CHANGELOG.md

src/Symfony/Component/Serializer/Normalizer/ChainDenormalizer.php

src/Symfony/Component/Serializer/SerializerAwareTrait.php

src/Symfony/Component/Serializer/Tests/Encoder/XmlEncoderTest.php

src/Symfony/Component/Serializer/Tests/Normalizer/AbstractNormalizerTest.php

src/Symfony/Component/Serializer/Tests/SerializerTest.php

mtarld · 2024-02-12T08:55:42Z

src/Symfony/Component/Serializer/Tests/SerializerTest.php

@@ -80,6 +82,9 @@

 class SerializerTest extends TestCase
 {
+    /**
+     * @group legacy
+     */


I think that we can add the non-legacy version as well

dunglas · 2024-06-17T15:11:01Z

It's a good step forward. I'm +1 for the idea.

yceruto

Nice refactoring! I left minor comments only.

src/Symfony/Component/ErrorHandler/composer.json

src/Symfony/Component/Serializer/Normalizer/ChainDenormalizer.php

src/Symfony/Component/Serializer/Serializer.php

stof · 2024-06-19T21:32:11Z

src/Symfony/Component/Serializer/Serializer.php

    /**
     * @param array<NormalizerInterface|DenormalizerInterface> $normalizers
     * @param array<EncoderInterface|DecoderInterface>         $encoders
     */
    public function __construct(
-        private array $normalizers = [],
+        array $normalizers = [],


defining the new signature of the constructor as being new Serializer([], $encoders, $chainNormalizer, $chainDenormalizer) looks weird to me. It don't think the array as first argument should be kept in the new signature

alternatively, we could keep the current constructor signature, making the ChainNormalizer and ChainDenormalizer internal implementation details, as done for the ChainEncoder and ChainDecoder (for which we classify them as being encoders, decoders or both inside the constructor). As most implementations are implementing both interfaces in the same class, it allows passing a single list in instantiation, making the configuration easier

Btw, this would mean that the chain classes would not need to implement the *aware interfaces (as the injection would be handled by the Serializer on each registered class)

Yeah. I dont like the current signature too.

I want to have a signature like:

new Serializer($encoders, $normalizer, $denormalizer); // or new Serializer($normalizer, $denormalizer, $encoders);

Should I immediately go to that signature or should we change it in SF8?

On your separate topic of making the Chain* classes an implementation detail.. That was an earlier version of the PR, but we decided against that to make the Serializer more simple. See #52900 (comment)

Treating them like $encoders is a valid idea. However, you have 1-3 encoders MAX. But you may have 100+ normalizers/denormalizers. So I think it makes more sense to keep them separate and give the developer more control.

Should I immediately go to that signature or should we change it in SF8?

You must support the new signature immediately. Otherwise doing the change in 8.0 will be forbidden as it would be a BC break without migration path.

On your separate topic of making the Chain* classes an implementation detail.. That was an earlier version of the PR, but we decided against that to make the Serializer more simple. See #52900 (comment)

That comment was actually suggesting to also change the way encoders are injected (we should avoid making 2 successive signature changes on the constructor, as that would be a pain to have 2 deprecated signatures to support in the BC layer)

Treating them like $encoders is a valid idea. However, you have 1-3 encoders MAX. But you may have 100+ normalizers/denormalizers. So I think it makes more sense to keep them separate and give the developer more control.

but then, you make it more likely to have nested ChainNormalizer instance, making it necessary to be 100% sure about the reliability of getSupportedTypes for instance (my suggested return ['*' => false]` will be reliable, even though it might not be the most optimized one).

src/Symfony/Component/Serializer/Serializer.php

src/Symfony/Component/Serializer/SerializerAwareInterface.php

stof · 2024-06-19T21:46:49Z

src/Symfony/Component/Serializer/DependencyInjection/SerializerPass.php

+            }
+
+            $class = $definition->getClass();
+            if (null === $class) {


this logic looks wrong to me. As this compiler pass is registered as a before-optimization pass, the definition class might not be known yet as a class name:

when using ChildDefinition, it can be null (if the class will be inherited from the parent), but this does not mean that skipping the registration of the service is the valid processing (btw, this is a BC break in case someone uses a ChildDefinition to configure a normalizer today)

if a service definition uses a parameter in the class name (which is supported since 2.0), the parameter is not yet resolved at that point, requiring to resolve it explicitly

src/Symfony/Component/Serializer/SerializerAwareInterface.php

src/Symfony/Component/Serializer/Normalizer/ChainNormalizer.php

stof · 2024-06-19T22:03:36Z

src/Symfony/Component/Serializer/Normalizer/ChainNormalizer.php

+    public function addNormalizer(NormalizerInterface $normalizer): void
+    {
+        if ($normalizer instanceof NormalizerAwareInterface) {
+            $normalizer->setNormalizer($this);


if we make the ChainNormalizer available publicly, this is flawed as it breaks when nesting a ChainNormalizer inside another ChainNormalizer (the normalizers inside the inner ChainNormalizer won't get access to the main normalizer supporting everything)

Would a solution be to have ChainNormalizer also implement the NormalizerAwareInterface?

src/Symfony/Component/Serializer/Normalizer/ChainNormalizer.php

Nyholm requested a review from dunglas as a code owner December 5, 2023 13:36

carsonbot added Status: Needs Review Deprecation Feature labels Dec 5, 2023

carsonbot added this to the 7.1 milestone Dec 5, 2023

Nyholm force-pushed the normalizer-aggregator branch from 16e5ed6 to 2bee4b6 Compare December 5, 2023 13:41

Nyholm added the Serializer label Dec 5, 2023

carsonbot changed the title ~~Add Normalizer and Denormalizer aggregators~~ [Serializer] Add Normalizer and Denormalizer aggregators Dec 5, 2023

Nyholm changed the title ~~[Serializer] Add Normalizer and Denormalizer aggregators~~ [Serializer] Add ChainNormalizer and ChainDenormalizer Dec 5, 2023

mtarld reviewed Dec 6, 2023

View reviewed changes

Nyholm force-pushed the normalizer-aggregator branch from 03f4233 to 72b85a5 Compare December 11, 2023 10:40

Nyholm requested a review from yceruto as a code owner December 11, 2023 11:07

Nyholm requested review from chalasr and mtarld December 11, 2023 11:11

Nyholm force-pushed the normalizer-aggregator branch 3 times, most recently from a36a11a to 1846b99 Compare January 9, 2024 11:48

Nyholm force-pushed the normalizer-aggregator branch 2 times, most recently from 5e913c5 to 98bdcb8 Compare January 31, 2024 20:14

Nyholm force-pushed the normalizer-aggregator branch from 8910244 to d212a7d Compare February 12, 2024 02:03

mtarld requested changes Feb 12, 2024

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 12, 2024

xabbuh removed this from the 7.1 milestone May 15, 2024

xabbuh added this to the 7.2 milestone May 15, 2024

Nyholm force-pushed the normalizer-aggregator branch 2 times, most recently from d18dd6d to 7fe79f3 Compare June 19, 2024 11:27

Add ChainNormalizer and ChainDenormalizer

5e56886

Nyholm force-pushed the normalizer-aggregator branch from ee04567 to 5e56886 Compare June 19, 2024 15:58

Nyholm added 2 commits June 19, 2024 18:22

allow sf 6.4 on Mime and Messenger

fc9c9ff

avoid updating composer.json in Messenger bridges

07c75ad

yceruto reviewed Jun 19, 2024

View reviewed changes

Nyholm added 5 commits June 19, 2024 23:02

updates from review

70896a4

make TwigBridge support sf 6.4

3ef1e1c

make error handler support sf 6.4

0e9704a

cs

1271e49

minors

2ebb160

stof requested changes Jun 19, 2024

View reviewed changes

Nyholm added 2 commits June 20, 2024 10:03

updating from feedback

1d9d0c2

introduce serializer.denormalizer tag

4f52388

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] Add ChainNormalizer and ChainDenormalizer #52900

[Serializer] Add ChainNormalizer and ChainDenormalizer #52900

Nyholm commented Dec 5, 2023 •

edited

Loading

Nyholm commented Dec 5, 2023 •

edited

Loading

mtarld left a comment

Nyholm commented Dec 11, 2023

Nyholm commented Jan 9, 2024

Nyholm commented Jan 31, 2024

Nyholm commented Feb 12, 2024

mtarld Feb 12, 2024

stof Jun 19, 2024

Nyholm Jun 19, 2024 •

edited

Loading

mtarld Feb 12, 2024

dunglas commented Jun 17, 2024

yceruto left a comment

stof Jun 19, 2024

stof Jun 19, 2024

Nyholm Jun 20, 2024 •

edited

Loading

stof Jun 20, 2024

stof Jun 19, 2024

stof Jun 19, 2024

Nyholm Jun 20, 2024

[Serializer] Add ChainNormalizer and ChainDenormalizer #52900

Are you sure you want to change the base?

[Serializer] Add ChainNormalizer and ChainDenormalizer #52900

Conversation

Nyholm commented Dec 5, 2023 • edited Loading

Nyholm commented Dec 5, 2023 • edited Loading

mtarld left a comment

Choose a reason for hiding this comment

Nyholm commented Dec 11, 2023

Nyholm commented Jan 9, 2024

Nyholm commented Jan 31, 2024

Nyholm commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Jun 17, 2024

yceruto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Dec 5, 2023 •

edited

Loading

Nyholm commented Dec 5, 2023 •

edited

Loading

Nyholm Jun 19, 2024 •

edited

Loading

Nyholm Jun 20, 2024 •

edited

Loading