Use default Symfony serializer service when available #276
Conversation
|
This means that normalizers created for this bundle are now included in the serializer used by your app (which could cause issues depending on the kind of objects you use) and that your own normalizers are now called when serializing the data in the bundle (which can totally break the features of the bundle depending of what your normalizers are, as your own normalizer could be used instead). If the issue is autowiring, the solution would be for Symfony to mark the main serializer services as the one used for autowiring, as it does already for the translator or the router for instance. |
| use Symfony\Component\DependencyInjection\Loader\XmlFileLoader; | ||
| use Symfony\Component\HttpKernel\DependencyInjection\Extension; | ||
| use Symfony\Component\Serializer\Serializer; |
There was a problem hiding this comment.
A rest from some refactoring, probably
| */ | ||
| class SerializerCompilerPass implements CompilerPassInterface | ||
| { | ||
| const SERIALIZER_SERVICE_ID = 'fos_js_routing.serializer'; |
There was a problem hiding this comment.
you should not add such constant. There is no reason to make this public (and private constants are not available in the targeted PHP versions)
|
|
||
| $encoder = $container->register('fos_js_routing.encoder', JsonEncoder::class); | ||
| $encoder->setPublic(false); | ||
| $definition->addArgument(['json' => $encoder]); |
There was a problem hiding this comment.
the composer.json requires php >=5.3.2, so the short array syntax is not OK.
| ]; | ||
| $definition->addArgument($normalizers); | ||
|
|
||
| $encoder = $container->register('fos_js_routing.encoder', JsonEncoder::class); |
|
@stof to have a normalizer that supports the normalization of RouteCollection or RoutesResponse it's weird, or you want to do the same this bundle do, so should be redundant. If you have a normalizer so generic that can be chosen instead of this bundle ones, you probably have a bigger problem (although you can play with priorities) Anyway, probably adding a configuration parameter to select if you want the main serializer or the bundle one could be useful/valid? |
|
As the bundle now requires the symfony serializer (and PHP 8), I think this PR can be closed. |
Previous to this PR, the bundle created a new service using the same class as the Symfony Serializer, what, apart from redundant, makes problems with autowiring (two services with the same class).
With this PR, if the serializer service exists, it's used, tagging the custom normalizers.
If the serializer services doesn't exist, it's registered as before (more or less, instead from file, is registered using a Compiler Pass)
Tests are ok (updated one and adding another)