[FrameworkBundle] Fix setting default context for certain normalizers #57273
Conversation
| $kernel = static::createKernel(['test_case' => 'Serializer', 'root_config' => 'default_context.yaml']); | ||
| $container = $kernel->getContainerWithLoadedExtensions(); | ||
|
|
||
| $services = array_merge( | ||
| $container->findTaggedServiceIds('serializer.normalizer'), | ||
| $container->findTaggedServiceIds('serializer.encoder') | ||
| ); | ||
| foreach ($services as $serviceId => $attributes) { | ||
| $class = $container->getDefinition($serviceId)->getClass(); | ||
| if (null === $reflectionConstructor = (new \ReflectionClass($class))->getConstructor()) { | ||
| continue; | ||
| } | ||
| foreach ($reflectionConstructor->getParameters() as $reflectionParam) { | ||
| if ('array $defaultContext' === $reflectionParam->getType().' $'.$reflectionParam->getName()) { | ||
| yield [$serviceId.'.alias']; | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I made this dynamic in case new normalizers/encoders were added. Is it worth it, no idea.
There was a problem hiding this comment.
I moved this into a separate config were circular_reference_handler and max_depth_handler are not set.
HypeMC
force-pushed
the
fix-setting-default-context-for-certain-normalizers
branch
from
2f01e22
to
b61e43a
Compare
HypeMC
force-pushed
the
fix-setting-default-context-for-certain-normalizers
branch
from
b61e43a
to
acf5fbc
Compare
There was a problem hiding this comment.
i just ran into this regression with Symfony 6.4.
maybe the maintainers could merge just the BC break fix quickly and look into the expanded tests separately, to at least fix the problem? meanwhile i will need to lock onto version 6.4.7 to prevent the bug. 5.4.39 and ~7.0 would be versions without the bug - 7.1.0 has the bug so if you are on 7.1 you are out of luck.
but i also like the idea of adding the missing tests to prevent the regression from re-occurring.
HypeMC
force-pushed
the
fix-setting-default-context-for-certain-normalizers
branch
2 times, most recently
from
484bf10
to
416c891
Compare
|
the ci failures seem unrelated to the changes: fabbot seems to look at the DI extension class because it is touched, but the complaints are not about the changes in this PR, and appveyor fails on something completely unrelated. if the latest 5.4 branch is green, this PR could be rebased on 5.4 to get them green. |
HypeMC
force-pushed
the
fix-setting-default-context-for-certain-normalizers
branch
from
416c891
to
653e8e0
Compare
HypeMC
force-pushed
the
fix-setting-default-context-for-certain-normalizers
branch
3 times, most recently
from
d2735c0
to
f903893
Compare
|
Thank you @HypeMC. |
|
Thanks @HypeMC for all the work fixing this 🥳 |
|
@fabpot Could this be tagged for 7.1? |
|
As it was merged in 5.4, it will indeed be part of the next 7.1 release. |
Caused by #54791. The main problem is that
$contextdefaults to[]instead of$defaultContext. There's a test to check this, but it didn't work whencircular_reference_handlerormax_depth_handlerwere notnull.I also found an issue with
serializer.normalizer.property. Since it’s not tagged withserializer.normalizer, which, to my understanding, is intentional, it would never have the default context bound to it.