Skip to content

[FrameworkBundle] Serializer groups support #13107

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -660,6 +660,10 @@ private function addSerializerSection(ArrayNodeDefinition $rootNode)
->arrayNode('serializer')
->info('serializer configuration')
->canBeEnabled()
->children()
->booleanNode('enable_annotations')->defaultFalse()->end()
->scalarNode('cache')->end()
->end()
->end()
->end()
;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\DefinitionDecorator;
use Symfony\Component\DependencyInjection\Exception\LogicException;
use Symfony\Component\DependencyInjection\Reference;
Expand All @@ -29,6 +30,7 @@
*
* @author Fabien Potencier <fabien@symfony.com>
* @author Jeremy Mikola <jmikola@gmail.com>
* @author Kévin Dunglas <dunglas@gmail.com>
*/
class FrameworkExtension extends Extension
{
Expand Down Expand Up @@ -121,11 +123,10 @@ public function load(array $configs, ContainerBuilder $container)
}

$this->registerAnnotationsConfiguration($config['annotations'], $container, $loader);

$this->registerPropertyAccessConfiguration($config['property_access'], $container);

if (isset($config['serializer']) && $config['serializer']['enabled']) {
$loader->load('serializer.xml');
if (isset($config['serializer'])) {
$this->registerSerializerConfiguration($config['serializer'], $container, $loader);
}

$loader->load('debug_prod.xml');
Expand Down Expand Up @@ -866,6 +867,86 @@ private function registerSecurityCsrfConfiguration(array $config, ContainerBuild
$loader->load('security_csrf.xml');
}

/**
* Loads the serializer configuration.
*
* @param array $config A serializer configuration array
* @param ContainerBuilder $container A ContainerBuilder instance
* @param XmlFileLoader $loader An XmlFileLoader instance
*/
private function registerSerializerConfiguration(array $config, ContainerBuilder $container, XmlFileLoader $loader)
{
if (!$config['enabled']) {
return;
}

$loader->load('serializer.xml');
$chainLoader = $container->getDefinition('serializer.mapping.chain_loader');

$serializerLoaders = array();
if (isset($config['enable_annotations']) && $config['enable_annotations']) {
$annotationLoader = new Definition(
'Symfony\Component\Serializer\Mapping\Loader\AnnotationLoader',
array(new Reference('annotation_reader'))
);
$annotationLoader->setPublic(false);

$serializerLoaders[] = $annotationLoader;
}

$bundles = $container->getParameter('kernel.bundles');
foreach ($bundles as $bundle) {
$reflection = new \ReflectionClass($bundle);
$dirname = dirname($reflection->getFilename());

if (is_file($file = $dirname.'/Resources/config/serialization.xml')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have other examples in Symfony where such a "convention" is harcoded? I know it's not the case for the routing, but probably done the same way for the translator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JMSSerializerBundle which is probably the most used use the serializer directory name for configs (see conf) , but it can be changed for any bundle manually. I think we should look for resource existence before searching for a default one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying that we should make it configurable. As a matter of fact, I think there is no need to make it configurable, I just wanted to be sure that we behave like other parts of the framework.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is to have a coherent behavior for both components (Validation, Translation, Routing...). If we allow to create a file by class (why not, I personally use annotations), it must be the same at least for validation mappings.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this PR, I'd prefer to keep what we have in other parts of the framework.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Validator component just got the ability to read the configuration from a directory (see #13855). Why not adapt this approach here too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointer @xabbuh. I had not seen this PR.
Support for Resources/config/serialization/*[.xml|.yml] added.

$definition = new Definition('Symfony\Component\Serializer\Mapping\Loader\XmlFileLoader', array(realpath($file)));
$definition->setPublic(false);

$serializerLoaders[] = $definition;
$container->addResource(new FileResource($file));
}

if (is_file($file = $dirname.'/Resources/config/serialization.yml')) {
$definition = new Definition('Symfony\Component\Serializer\Mapping\Loader\YamlFileLoader', array(realpath($file)));
$definition->setPublic(false);

$serializerLoaders[] = $definition;
$container->addResource(new FileResource($file));
}

if (is_dir($dir = $dirname.'/Resources/config/serialization')) {
foreach (Finder::create()->files()->in($dir)->name('*.xml') as $file) {
$definition = new Definition('Symfony\Component\Serializer\Mapping\Loader\XmlFileLoader', array($file->getRealpath()));
$definition->setPublic(false);

$serializerLoaders[] = $definition;
}
foreach (Finder::create()->files()->in($dir)->name('*.yml') as $file) {
$definition = new Definition('Symfony\Component\Serializer\Mapping\Loader\YamlFileLoader', array($file->getRealpath()));
$definition->setPublic(false);

$serializerLoaders[] = $definition;
}

$container->addResource(new DirectoryResource($dir));
}
}

$chainLoader->replaceArgument(0, $serializerLoaders);

if (isset($config['cache']) && $config['cache']) {
$container->setParameter(
'serializer.mapping.cache.prefix',
'serializer_'.hash('sha256', $container->getParameter('kernel.root_dir'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this cause issues if cache is warmed up in one place, and later moved to another directory? I know we not necessarily support such case, but I'd rather avoid adding one more obstacle here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably but this is already the case for the validator: https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L749

If we change this behavior, we must change it everywhere in another PR.

);

$container->getDefinition('serializer.mapping.class_metadata_factory')->replaceArgument(
1, new Reference($config['cache'])
);
}
}

/**
* Returns the base path for the XSD files.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
<xsd:element name="validation" type="validation" minOccurs="0" maxOccurs="1" />
<xsd:element name="annotations" type="annotations" minOccurs="0" maxOccurs="1" />
<xsd:element name="property-access" type="property_access" minOccurs="0" maxOccurs="1" />
<xsd:element name="serializer" type="serializer" minOccurs="0" maxOccurs="1" />
</xsd:all>

<xsd:attribute name="http-method-override" type="xsd:boolean" />
Expand Down Expand Up @@ -210,4 +211,10 @@
<xsd:attribute name="magic-call" type="xsd:boolean" />
<xsd:attribute name="throw-exception-on-invalid-index" type="xsd:boolean" />
</xsd:complexType>

<xsd:complexType name="serializer">
<xsd:attribute name="enabled" type="xsd:boolean" />
<xsd:attribute name="cache" type="xsd:string" />
<xsd:attribute name="enable-annotations" type="xsd:boolean" />
</xsd:complexType>
</xsd:schema>
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,31 @@
</service>

<!-- Normalizer -->
<service id="serializer.normalizer.get_set_method" class="Symfony\Component\Serializer\Normalizer\GetSetMethodNormalizer" public="false">
<service id="serializer.normalizer.object" class="Symfony\Component\Serializer\Normalizer\ObjectNormalizer" public="false">
<argument type="service" id="serializer.mapping.class_metadata_factory" />

<!-- Run after all custom serializers -->
<tag name="serializer.normalizer" priority="-1000" />
</service>

<!-- Loader -->
<service id="serializer.mapping.chain_loader" class="Symfony\Component\Serializer\Mapping\Loader\LoaderChain" public="false">
<argument type="collection" />
</service>

<!-- Class Metadata Factory -->
<service id="serializer.mapping.class_metadata_factory" class="Symfony\Component\Serializer\Mapping\Factory\ClassMetadataFactory" public="false">
<argument type="service" id="serializer.mapping.chain_loader" />
<argument>null</argument>
</service>

<!-- Cache -->
<service id="serializer.mapping.cache.apc" class="Doctrine\Common\Cache\ApcCache" public="false">
<call method="setNamespace">
<argument>%serializer.mapping.cache.prefix%</argument>
</call>
</service>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, though it still looks a little odd to me to have this service here that may not be used, I do like it. As I mentioned before, it's private, so will be removed if it's not being used. And I think it ends up being quite usable. 👍


<!-- Encoders -->
<service id="serializer.encoder.xml" class="%serializer.encoder.xml.class%" public="false" >
<tag name="serializer.encoder" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ protected static function getBundleDefaultConfig()
),
'serializer' => array(
'enabled' => false,
'enable_annotations' => false,
),
'property_access' => array(
'magic_call' => false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
'debug' => true,
'file_cache_dir' => '%kernel.cache_dir%/annotations',
),
'serializer' => array('enabled' => true),
'ide' => 'file%%link%%format',
'request' => array(
'formats' => array(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,6 @@
<framework:translator enabled="true" fallback="fr" logging="true" />
<framework:validation enabled="true" cache="apc" />
<framework:annotations cache="file" debug="true" file-cache-dir="%kernel.cache_dir%/annotations" />
<framework:serializer enabled="true" />
</framework:config>
</container>
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ framework:
cache: file
debug: true
file_cache_dir: %kernel.cache_dir%/annotations
serializer: { enabled: true }
ide: file%%link%%format
request:
formats:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,18 @@ public function testStopwatchEnabledWithDebugModeDisabled()
$this->assertTrue($container->has('debug.stopwatch'));
}

public function testSerializerDisabled()
{
$container = $this->createContainerFromFile('default_config');
$this->assertFalse($container->has('serializer'));
}

public function testSerializerEnabled()
{
$container = $this->createContainerFromFile('full');
$this->assertTrue($container->has('serializer'));
}

protected function createContainer(array $data = array())
{
return new ContainerBuilder(new ParameterBag(array_merge(array(
Expand Down