Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Serializer] Named serializer should get correct normalizer instance#59627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.2
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Named serializers were introduced insymfony#56823 and they work great.We noticed a small bug when using custom name convertors.The MimeMessageNormalizer holds a reference to `serializer.normalizer.property`.But when using named serializers, it would use the specific child normalizer instead.With this change, we fix this problem for any service that starts with `serializer.normalizer.`.
93ca946 toe0caeb7Compare| foreach ($container->getDefinition($id)->getArguments() as $index => $argument) { | ||
| if ($argument instanceof Reference && self::NAME_CONVERTER_METADATA_AWARE_ID === (string) $argument) { | ||
| $definition->replaceArgument($index, new Reference($config['name_converter'])); | ||
| } elseif ($argument instanceof Reference && str_starts_with((string) $argument, 'serializer.normalizer.')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ruudk This won't work since the service is not tagged withserializer.normalizer:
symfony/src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.php
Lines 136 to 146 inac26e1b
| ->set('serializer.normalizer.property', PropertyNormalizer::class) | |
| ->args([ | |
| service('serializer.mapping.class_metadata_factory'), | |
| service('serializer.name_converter.metadata_aware'), | |
| service('property_info')->ignoreOnInvalid(), | |
| service('serializer.mapping.class_discriminator_resolver')->ignoreOnInvalid(), | |
| null, | |
| ]) | |
| ->set('serializer.denormalizer.array', ArrayDenormalizer::class) |
Without the tag, the child service will never be created. As far as I can tell, the service was intentionally not tagged, but I could be wrong, see#37847.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Weird because this fix does solve the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Are you sure? I've tested the fix on a clean install and got an error:
framework:serializer:named_serializers:foo:name_converter:serializer.name_converter.camel_case_to_snake_case
The service "serializer.normalizer.mime_message.foo" has a dependency on a non-existent service "serializer.normalizer.property.foo".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oops, you're right. For some reason, the project where we experience this problem, had the following compiler pass:
$container->addCompilerPass(newclass()implements CompilerPassInterface {publicfunctionprocess(ContainerBuilder$container) :void {$container->getDefinition('serializer.normalizer.property') ->addTag('serializer.normalizer', ['priority' => -990, ]); } }, priority:10, );
I cannot recall why we added this, but probably to make sure serialization worked the same way as JMS Serializer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I know why we added this, because we want to use the PropertyNormalizer to make sure no getters are called while serializing. We only want properties to be serialized.
Is there a better way to enable this? Or is the compiler pass above the recommended way to use this normalizer?
Maybe we can check in this PR if theserializer.normalizer.*.$serializer_name exists, and then replace it?
Uh oh!
There was an error while loading.Please reload this page.
Named serializers were introduced in#56823 and they work great.
We noticed a small bug when using custom name convertors.
The MimeMessageNormalizer holds a reference to
serializer.normalizer.property.But when using named serializers, it should use the specific child normalizer instead.
With this change, we fix this problem for any service that starts with
serializer.normalizer../cc@HypeMC