Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[2.2] [FrameworkBundle] [Serializer] Loads the Serializer component as a service in the Framework Bundle #5347

Closed
wants to merge 26 commits into from

6 participants

@loalf

It looks like the Serializer service is not in the Service Container. This PR allows users to get this service through the DIC by just simple doing $container->get('serializer');

Also, by default, the Framework Bundle will load a couple of encoders (JSON and XML) and one normalizer (GetSetMethodNormalizar). If the user wants to add more encoders or normalizer he will just have to tag it, using the specials tags "serializer.encoder" and "serializer.normalizer".

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: [comma separated list of tickets fixed by the PR]
Todo: If this PR is approved then this file
https://github.com/symfony/symfony-standard/blob/master/app/config/config.yml
should be modified for something like this
https://gist.github.com/3471185
License of the code: MIT

@travisbot

This pull request fails (merged 881e9a6 into d1be451).

...undle/DependencyInjection/Compiler/SerializerPass.php
((2 lines not shown))
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+use Symfony\Component\DependencyInjection\Reference;
+/**

empty line is needed here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pborreli pborreli commented on the diff
...undle/DependencyInjection/Compiler/SerializerPass.php
((16 lines not shown))
+use Symfony\Component\DependencyInjection\Reference;
+/**
+ * Adds all services with the tags "serializer.encoder" and "serializer.normalizer" as
+ * encoders and normalizers to the Serializer service.
+ *
+ * @author Javier Lopez <f12loalf@gmail.com>
+ */
+class SerializerPass implements CompilerPassInterface
+{
+ public function process(ContainerBuilder $container)
+ {
+ if (!$container->hasDefinition('serializer')) {
+ return;
+ }
+
+ // Looks for all the services tagged "serializer.normalizer" and adds them to the Serializer service

you execute the whole same code twice maybe factorize it inside a loop or a method ?

@loalf
loalf added a note

I´ve reduced the length of this code so I think there is no need to refactor it anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...undle/FrameworkBundle/Resources/config/serializer.xml
((9 lines not shown))
+ <parameter key="serializer.encoder.xml.class">Symfony\Component\Serializer\Encoder\XmlEncoder</parameter>
+ <parameter key="serializer.encoder.json.class">Symfony\Component\Serializer\Encoder\JsonEncoder</parameter>
+ <parameter key="serializer.normalizer.get_set_method.class">Symfony\Component\Serializer\Normalizer\GetSetMethodNormalizer</parameter>
+ </parameters>
+
+ <services>
+ <service id="serializer" class="%serializer.class%" >
+ <argument type="collection" />
+ <argument type="collection" />
+ </service>
+ <!-- Encoders -->
+ <service id="serializer.encoder.xml" class="%serializer.encoder.xml.class%" public="false" >
+ <tag name="serializer.encoder" priority="1000" />
+ </service>
+ <service id="serializer.encoder.json" class="%serializer.encoder.json.class%" public="false" >
+ <tag name="serializer.encoder" priority="2000" />
@stof Collaborator
stof added a note

Why using such high priorities ? and why using different priorities for json and xml ? They don't support the same formats anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...undle/FrameworkBundle/Resources/config/serializer.xml
((14 lines not shown))
+ <services>
+ <service id="serializer" class="%serializer.class%" >
+ <argument type="collection" />
+ <argument type="collection" />
+ </service>
+ <!-- Encoders -->
+ <service id="serializer.encoder.xml" class="%serializer.encoder.xml.class%" public="false" >
+ <tag name="serializer.encoder" priority="1000" />
+ </service>
+ <service id="serializer.encoder.json" class="%serializer.encoder.json.class%" public="false" >
+ <tag name="serializer.encoder" priority="2000" />
+ </service>
+ <!-- Normalizers -->
+ <service id="serializer.normalizer.get_set_method" class="%serializer.normalizer.get_set_method.class%" public="false" >
+ <tag name="serializer.normalizer" priority="1000" />
+ </service>
@stof Collaborator
stof added a note

Registering this one should be optional. The GetSetMethodNormalizer is broken (by design) as soon as you have a cyclic object graph (you have an infinite loop when calling getters), so forcing to register it is a bad idea (expecially as many people tend to use bidirectional relations in their entities apparently)

@stof Collaborator
stof added a note

for this service, you should add the tag conditionally in the DI extension according to a configuration, to allow disabling it.

and it should have a negative priority rather that a high priority IMO, so that custom normalizers registered with the default priority can be checked first (the GetSetNormalizer will accept any input)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...undle/DependencyInjection/Compiler/SerializerPass.php
((19 lines not shown))
+ * encoders and normalizers to the Serializer service.
+ *
+ * @author Javier Lopez <f12loalf@gmail.com>
+ */
+class SerializerPass implements CompilerPassInterface
+{
+ public function process(ContainerBuilder $container)
+ {
+ if (!$container->hasDefinition('serializer')) {
+ return;
+ }
+
+ // Looks for all the services tagged "serializer.normalizer" and adds them to the Serializer service
+ $normalizers = array();
+
+ $min_priority = 0;
@stof Collaborator
stof added a note

Please use camelCased names

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...undle/DependencyInjection/Compiler/SerializerPass.php
((28 lines not shown))
+ return;
+ }
+
+ // Looks for all the services tagged "serializer.normalizer" and adds them to the Serializer service
+ $normalizers = array();
+
+ $min_priority = 0;
+ foreach ($container->findTaggedServiceIds('serializer.normalizer') as $serviceId => $tag) {
+ if(isset($tag[0]['priority'])){
+ $priority = $tag[0]['priority'];
+ }else{
+ $priority = $min_priority;
+ $min_priority++;
+ }
+
+ $normalizers[$priority] = new Reference($serviceId);
@stof Collaborator
stof added a note

the way you handle priorities is inconsistent with the other places in Symfony. You force using unique priority, which would require knowing all services being tagged to be sure it is not used twice (as one of them would be lost here), which totally defeats the benefit of tags (not having to know all services using the tag)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@loalf

I´ve got rid of the priorities as they seem to confusing.

@stof Do I still need at least one normalizer to set up the Serializer service?

@travisbot

This pull request fails (merged 0278df8 into d1be451).

@stof
Collaborator

Priorities are needed IMO as the order of normalizers is relevant (the first one supporting it is used). But they should behave in the same way than in all other parts of Symfony.

@loalf

@stof could you please show what those parts are so I can use them as a reference to implement that feature in this PR. Thanks.

@stof
Collaborator

@loalf the registration of listeners for instance

@loalf

@stof I guess you mean this implementation, https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/RegisterKernelListenersPass.php, no?

I´ve taken a look at this code and it does pretty much the same thing it did in my previous code so I wonder if when you say "they should behave in the same way than in all other parts of Symfony" you actually mean adding a couple of methods in the Serializer service (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Serializer/Serializer.php), namely, Serializer::addNormalizer($normalizer, $priority) and Serializer::addEncoder($encoder, $priority) to be used by the CompilerPass.

@stof
Collaborator

@loalf no. You previous implementation was allowing only one reference for each priority (and so you had to keep an counter for the guessed priority) whereas the existing code uses an array for each priority to have several ones.

And no, you don't need to handle the priority in the Serializer IMO. when configuring the serializer by hand, you control the registration order. The priority in tags is needed to control it, but it can be sorted at compile time.

@loalf

Sorry for the long delay to reply to this.

There is one thing I am not still happy with. I created a private method, findAndSortTaggedServices, in the SerializerCompiler class but, to be honest, I think this method should be on the Container class so everyone could benefit of this feature. Any comment?

...undle/DependencyInjection/Compiler/SerializerPass.php
((40 lines not shown))
+
+ private function findAndSortTaggedServices($tag, $container)
+ {
+ // Find tagged services
+ $servs = array();
+ foreach ($container->findTaggedServiceIds($tag) as $serviceId => $value) {
+ $priority = isset($value[0]['priority']) ? $value[0]['priority'] : 0;
+ $servs[$priority][] = new Reference($serviceId);
+ }
+
+ // Sort them
+ krsort($servs);
+
+ // Flatten the array
+ $services = array();
+ array_walk_recursive($servs, function($a) use (&$services) { $services[] = $a; });
@stof Collaborator
stof added a note
if (!empty($servs)) {
    $services = call_user_fun_array('array_merge', $servs);
}
@loalf
loalf added a note

What´s that for?

@stof Collaborator
stof added a note

A way to do it more efficiently and without having to use the array by reference (and used in some other places)

@loalf
loalf added a note

nice trick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...undle/DependencyInjection/Compiler/SerializerPass.php
((31 lines not shown))
+
+ // Looks for all the services tagged "serializer.normalizer" and adds them to the Serializer service
+ $normalizers = $this->findAndSortTaggedServices('serializer.normalizer', $container);
+ $container->getDefinition('serializer')->replaceArgument(0, $normalizers);
+
+ // Looks for all the services tagged "serializer.encoders" and adds them to the Serializer service
+ $encoders = $this->findAndSortTaggedServices('serializer.encoder', $container);
+ $container->getDefinition('serializer')->replaceArgument(1, $encoders);
+ }
+
+ private function findAndSortTaggedServices($tag, $container)
+ {
+ // Find tagged services
+ $servs = array();
+ foreach ($container->findTaggedServiceIds($tag) as $serviceId => $value) {
+ $priority = isset($value[0]['priority']) ? $value[0]['priority'] : 0;
@stof Collaborator
stof added a note

using $value[0] is wrong. You need to loop over all serializer.normalizer tags, not handle only the first one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@travisbot

This pull request passes (merged e579aac into d1be451).

@travisbot

This pull request passes (merged 91812e2 into d1be451).

@travisbot

This pull request passes (merged 4d6fdc0 into d1be451).

...undle/DependencyInjection/Compiler/SerializerPass.php
((26 lines not shown))
+ public function process(ContainerBuilder $container)
+ {
+ if (!$container->hasDefinition('serializer')) {
+ return;
+ }
+
+ // Looks for all the services tagged "serializer.normalizer" and adds them to the Serializer service
+ $normalizers = $this->findAndSortTaggedServices('serializer.normalizer', $container);
+ $container->getDefinition('serializer')->replaceArgument(0, $normalizers);
+
+ // Looks for all the services tagged "serializer.encoders" and adds them to the Serializer service
+ $encoders = $this->findAndSortTaggedServices('serializer.encoder', $container);
+ $container->getDefinition('serializer')->replaceArgument(1, $encoders);
+ }
+
+ private function findAndSortTaggedServices($tag_name, $container)
@stof Collaborator
stof added a note

Please use camelCased names for variables

@stof Collaborator
stof added a note

and you should typehint the ContainerBuilder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...FrameworkBundle/DependencyInjection/Configuration.php
@@ -409,4 +410,22 @@ private function addAnnotationsSection(ArrayNodeDefinition $rootNode)
->end()
;
}
+
+ private function addSerializerSection(ArrayNodeDefinition $rootNode)
+ {
+ $rootNode
+ ->children()
+ ->arrayNode('serializer')
+ ->info('serializer configuration')
+ ->canBeUnset()
+ ->treatNullLike(array('enabled' => true))
+ ->treatTrueLike(array('enabled' => true))
+ ->children()
+ ->booleanNode('enabled')->defaultTrue()->end()
+ ->end()
@stof Collaborator
stof added a note

please use the new canBeDisabled() shortcut

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...undle/FrameworkBundle/Resources/config/serializer.xml
((9 lines not shown))
+ <parameter key="serializer.encoder.xml.class">Symfony\Component\Serializer\Encoder\XmlEncoder</parameter>
+ <parameter key="serializer.encoder.json.class">Symfony\Component\Serializer\Encoder\JsonEncoder</parameter>
+ <parameter key="serializer.normalizer.get_set_method.class">Symfony\Component\Serializer\Normalizer\GetSetMethodNormalizer</parameter>
+ </parameters>
+
+ <services>
+ <service id="serializer" class="%serializer.class%" >
+ <argument type="collection" />
+ <argument type="collection" />
+ </service>
+ <!-- Encoders -->
+ <service id="serializer.encoder.xml" class="%serializer.encoder.xml.class%" public="false" >
+ <tag name="serializer.encoder" priority="2000" />
+ </service>
+ <service id="serializer.encoder.json" class="%serializer.encoder.json.class%" public="false" >
+ <tag name="serializer.encoder" priority="1000" />
@stof Collaborator
stof added a note

Please remove the priority for the encoders. The order of these 2 is irrelevant (they are handling different formats anyway) and I don't see a reason to make them use a high priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof
Collaborator

And you should also add a note in the Changelog of the FrameworkBundle, and send a PR to the documentation to update the list of DIC tags.

@loalf

Hi @stof, I think I've done everything you asked me. I also created a new PR on the symfony doc, symfony/symfony-docs#1829.

Please, let me know if there is anything left. Thanks!

...undle/DependencyInjection/Compiler/SerializerPass.php
((41 lines not shown))
+ private function findAndSortTaggedServices($tagName, ContainerBuilder $container)
+ {
+ // Find tagged services
+ $services = array();
+ foreach ($container->findTaggedServiceIds($tagName) as $serviceId => $tags) {
+ foreach($tags as $tag) {
+ $priority = isset($tag['priority']) ? $tag['priority'] : 0;
+ $services[$priority][] = new Reference($serviceId);
+ }
+ }
+
+ // Sort them
+ krsort($services);
+
+ // Flatten the array
+ if(!empty($services)) {
@stof Collaborator
stof added a note

missing spaec after if

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabpot
Owner

@loaf: Can you rebase on master and squash your commits?

I think there is still one issue you have not solved yet: the automatic registration of GetSetMethodNormalizer.

To quote @stof (the comments are now hidden because you made some changes to the files):

"Registering this one should be optional. The GetSetMethodNormalizer is broken (by design) as soon as you have a cyclic object graph (you have an infinite loop when calling getters), so forcing to register it is a bad idea (expecially as many people tend to use bidirectional relations in their entities apparently)
stof repo collab 16 days ago
for this service, you should add the tag conditionally in the DI extension according to a configuration, to allow disabling it.

and it should have a negative priority rather that a high priority IMO, so that custom normalizers registered with the default priority can be checked first (the GetSetNormalizer will accept any input)"

@loalf

@fabpot I can only think about removing the GetSetMethodNormalizer in the serializer.yml and load it as tagged serviced in case you want to load it (I have updated the documenation to explain why this normalizer is not loaded by default and how to do it, symfony/symfony-docs#1829). Is this what you have in mind?

humandb added some commits
@humandb humandb Adding the Serializer service to the Framework Bundle c6253ce
@humandb humandb Merge branch 'serializer_in_dic' of github.com:loalf/symfony into ser…
…ializer_in_dic

Conflicts:
	src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.xml
5fd4e90
@humandb humandb Merge branch 'serializer_in_dic' of github.com:loalf/symfony into ser…
…ializer_in_dic

Conflicts:
	src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.xml
1ddcba4
@stof
Collaborator

@loalf IMO, you should keep the service definition, but add the tag only based on a config flag (enabled by default to make it easier to use for simple cases but allowing to disable this normalizer)

framework:
    serializer:
        enabled: true
        get_set_normalizer: false
@loalf

@stof Honestly, I don't like this kind of "magic". Since you only have to create a new service and tag it properly, what's the point of overengineering the code just for this normalizer? Why not treat it as any other normalizer/serializer you may want to load and make it explicit and transparent? @fabpot

By the way, my apologies about the long delays in my responses. I've been a little bit busy lately.

@stof
Collaborator

@loalf The point is, your current setup requires registering a normalizer to be usable. With my suggestion, the serializer would be usable directly (when you don't have circular references in your object graph) but you would still be able to remove the GetSetNormalizer if you don't want to use it (for instance for the case where you have circular references)

@loalf

@stof ok, I do get it. The thing is I am not a big fun of ad hoc solutions when there is already a standard way to load a normalizer (tagging it).

As you said, the current setup is unusable unless you register at least one normalizer so, what about throwing an exception in the SerializerPass class if there aren't any normalizer/encoder registered?

@loalf

@stof @fabpot any thoughts on this?

@fabpot
Owner

I agree with @loalf: no tagged services by default and an exception if none are.

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
((5 lines not shown))
* A new parameter has been added to the DIC: `router.request_context.base_url`
You can customize it for your functional tests or for generating urls with
the right base url when your are in the cli context.
+>>>>>>> master
@fabpot Owner
fabpot added a note

merge conflict resolution problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...undle/DependencyInjection/Compiler/SerializerPass.php
((30 lines not shown))
+ }
+
+ // Looks for all the services tagged "serializer.normalizer" and adds them to the Serializer service
+ $normalizers = $this->findAndSortTaggedServices('serializer.normalizer', $container);
+ $container->getDefinition('serializer')->replaceArgument(0, $normalizers);
+
+ // Looks for all the services tagged "serializer.encoders" and adds them to the Serializer service
+ $encoders = $this->findAndSortTaggedServices('serializer.encoder', $container);
+ $container->getDefinition('serializer')->replaceArgument(1, $encoders);
+ }
+
+ private function findAndSortTaggedServices($tagName, ContainerBuilder $container)
+ {
+ $services = $container->findTaggedServiceIds($tagName);
+
+ if(empty($services)) {
@fabpot Owner
fabpot added a note

there is a missing space after if.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...undle/DependencyInjection/Compiler/SerializerPass.php
((31 lines not shown))
+
+ // Looks for all the services tagged "serializer.normalizer" and adds them to the Serializer service
+ $normalizers = $this->findAndSortTaggedServices('serializer.normalizer', $container);
+ $container->getDefinition('serializer')->replaceArgument(0, $normalizers);
+
+ // Looks for all the services tagged "serializer.encoders" and adds them to the Serializer service
+ $encoders = $this->findAndSortTaggedServices('serializer.encoder', $container);
+ $container->getDefinition('serializer')->replaceArgument(1, $encoders);
+ }
+
+ private function findAndSortTaggedServices($tagName, ContainerBuilder $container)
+ {
+ $services = $container->findTaggedServiceIds($tagName);
+
+ if(empty($services)) {
+ throw new \RuntimeException(sprintf("You must tag at least one service as '%s' to use the Serializer service", $tagName));
@fabpot Owner
fabpot added a note

We are using " instead of ' in sentences. So it should be "%s" instead of '%s'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabpot fabpot commented on the diff
...FrameworkBundle/DependencyInjection/Configuration.php
@@ -380,4 +381,17 @@ private function addAnnotationsSection(ArrayNodeDefinition $rootNode)
->end()
;
}
+
+ private function addSerializerSection(ArrayNodeDefinition $rootNode)
+ {
+ $rootNode
+ ->children()
+ ->arrayNode('serializer')
+ ->info('serializer configuration')
+ ->canBeDisabled()
+ ->end()
+ ->end()
+ ;
+ }
+
@fabpot Owner
fabpot added a note

This blank line should be removed

@loalf
loalf added a note

I can't see a duplicate blank line cause there should be one blank line between "{" and "private ... ", shouldn't it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabpot fabpot commented on the diff
...FrameworkBundle/DependencyInjection/Configuration.php
@@ -380,4 +381,17 @@ private function addAnnotationsSection(ArrayNodeDefinition $rootNode)
->end()
;
}
+
+ private function addSerializerSection(ArrayNodeDefinition $rootNode)
+ {
+ $rootNode
+ ->children()
+ ->arrayNode('serializer')
+ ->info('serializer configuration')
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...workBundle/DependencyInjection/FrameworkExtension.php
@@ -646,6 +650,19 @@ private function registerAnnotationsConfiguration(array $config, ContainerBuilde
}
/**
+ * Loads the Serializer configuration.
+ *
+ * @param array $config A Serializer configuration array
+ * @param XmlFileLoader $loader An XmlFileLoader instance
+ */
+ private function registerSerializerConfiguration(array $config, XmlFileLoader $loader)
+ {
+ if (!empty($config['enabled'])) {
@fabpot Owner
fabpot added a note

should be just $config['enabled']

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabpot fabpot commented on the diff
...s/DependencyInjection/Compiler/SerializerPassTest.php
((10 lines not shown))
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\SerializerPass;
+
+/**
+ * Tests for the SerializerPass class
+ *
+ * @author Javier Lopez <f12loalf@gmail.com>
+ */
+class SerializerPassTest extends \PHPUnit_Framework_TestCase
+{
+
@fabpot Owner
fabpot added a note

Those blank lines should be removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabpot
Owner

@loalf Can you take my comments into account, squash your commits, and submit a PR for the docs? Thanks.

humandb added some commits
@humandb humandb Merge branch 'master' into serializer_in_dic
Conflicts:
	src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
	src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php
2fbd210
@humandb humandb Fixed minor issues 8cb0564
@loalf

This is the link to the PR for the docs, https://github.com/symfony/symfony-docs/pull/1829/files, is that enough?

@loalf

Please, close this PR and carry on #6815

@fabpot fabpot closed this
@77web 77web referenced this pull request from a commit in 77web/symfony-docs
@humandb humandb Update reference/configuration/framework.rst
This PR is related to this one symfony/symfony#5347.
c01bc53
@ondrejmirtes ondrejmirtes referenced this pull request from a commit in ondrejmirtes/symfony
@fabpot fabpot merged branch loalf/add_serializer_service (PR #6815)
This PR was merged into the master branch.

Discussion
----------

[2.3] [FrameworkBundle] [Serializer] Loads the Serializer component as a service in the Framework Bundle

This PR is the same as
symfony#5347

but since I am struggling to squash all the commits I better create a new one. Sorry for the inconveniences, :)

Commits
-------

b4e4844 Add the serializer service
9c4ba6f
@fabpot fabpot referenced this pull request from a commit
@fabpot fabpot feature #12098 [Serializer] Handle circular references (dunglas)
This PR was merged into the 2.6-dev branch.

Discussion
----------

[Serializer] Handle circular references

| Q             | A
| ------------- | ---
| Bug fix?      | Yes: avoid infinite loops. Allows to improve #5347
| New feature?  | yes (circular reference handler)
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT
| Doc PR        | symfony/symfony-docs#4299

This PR adds handling of circular references in the `Serializer` component.
The number of allowed iterations is configurable (one by default).
The behavior when a circular reference is detected is configurable. By default an exception is thrown. Instead of throwing an exception, it's possible to register a custom handler (e.g.: a Doctrine Handler returning the object ID).

Usage:
```php
use Symfony\Component\Serializer\Normalizer\GetSetMethodNormalizer;
use Symfony\Component\Serializer\Serializer;

class MyObj
{
    private $id = 1312;

    public function getId()
    {
        return $this->getId();
    }

    public function getMe()
    {
        return $this;
    }
}

$normalizer = new GetSetMethodNormalizer();
$normalizer->setCircularReferenceLimit(3);
$normalizer->setCircularReferenceHandler(function ($obj) {
    return $obj->getId();
});

$serializer = new Serializer([$normalizer]);
$serializer->normalize(new MyObj());
```

Commits
-------

48491c4 [Serializer] Handle circular references
a05379e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 25, 2012
  1. @humandb
  2. @humandb
  3. @humandb
  4. @humandb
  5. @humandb

    Updated serializer.xml

    humandb authored
Commits on Aug 26, 2012
  1. @humandb

    Getting rid of the priorities

    humandb authored
Commits on Sep 3, 2012
  1. @humandb
  2. @humandb
  3. @humandb
Commits on Oct 14, 2012
  1. @humandb
Commits on Oct 18, 2012
  1. @humandb
  2. @humandb
  3. @humandb
  4. @humandb

    Using canBeDisabled()

    humandb authored
  5. @humandb
Commits on Oct 20, 2012
  1. @humandb

    Missing space after 'if'

    humandb authored
Commits on Oct 29, 2012
  1. @humandb
  2. @humandb

    Merge branch 'serializer_in_dic' of github.com:loalf/symfony into ser…

    humandb authored
    …ializer_in_dic
    
    Conflicts:
    	src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.xml
  3. @humandb

    Merge branch 'serializer_in_dic' of github.com:loalf/symfony into ser…

    humandb authored
    …ializer_in_dic
    
    Conflicts:
    	src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.xml
Commits on Nov 4, 2012
  1. @humandb
Commits on Dec 16, 2012
  1. @humandb

    Merge branch 'master' into serializer_in_dic

    humandb authored
    Conflicts:
    	src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
  2. @humandb
  3. @humandb
  4. @humandb
Commits on Jan 19, 2013
  1. @humandb

    Merge branch 'master' into serializer_in_dic

    humandb authored
    Conflicts:
    	src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
    	src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php
  2. @humandb

    Fixed minor issues

    humandb authored
This page is out of date. Refresh to see the latest.
View
1  src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
@@ -17,6 +17,7 @@ CHANGELOG
* replaced Symfony\Bundle\FrameworkBundle\Controller\TraceableControllerResolver by Symfony\Component\HttpKernel\Controller\TraceableControllerResolver
* replaced Symfony\Component\HttpKernel\Debug\ContainerAwareTraceableEventDispatcher by Symfony\Component\HttpKernel\Debug\TraceableEventDispatcher
* added Client::enableProfiler()
+ * added posibility to load the serializer component in the service container
* A new parameter has been added to the DIC: `router.request_context.base_url`
You can customize it for your functional tests or for generating urls with
the right base url when your are in the cli context.
View
62 src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/SerializerPass.php
@@ -0,0 +1,62 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+use Symfony\Component\DependencyInjection\Reference;
+
+/**
+ * Adds all services with the tags "serializer.encoder" and "serializer.normalizer" as
+ * encoders and normalizers to the Serializer service.
+ *
+ * @author Javier Lopez <f12loalf@gmail.com>
+ */
+class SerializerPass implements CompilerPassInterface
+{
+ public function process(ContainerBuilder $container)
+ {
+ if (!$container->hasDefinition('serializer')) {
+ return;
+ }
+
+ // Looks for all the services tagged "serializer.normalizer" and adds them to the Serializer service

you execute the whole same code twice maybe factorize it inside a loop or a method ?

@loalf
loalf added a note

I´ve reduced the length of this code so I think there is no need to refactor it anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ $normalizers = $this->findAndSortTaggedServices('serializer.normalizer', $container);
+ $container->getDefinition('serializer')->replaceArgument(0, $normalizers);
+
+ // Looks for all the services tagged "serializer.encoders" and adds them to the Serializer service
+ $encoders = $this->findAndSortTaggedServices('serializer.encoder', $container);
+ $container->getDefinition('serializer')->replaceArgument(1, $encoders);
+ }
+
+ private function findAndSortTaggedServices($tagName, ContainerBuilder $container)
+ {
+ $services = $container->findTaggedServiceIds($tagName);
+
+ if (empty($services)) {
+ throw new \RuntimeException(sprintf('You must tag at least one service as "%s" to use the Serializer service', $tagName));
+ }
+
+ $sortedServices = array();
+ foreach ($services as $serviceId => $tags) {
+ foreach ($tags as $tag) {
+ $priority = isset($tag['priority']) ? $tag['priority'] : 0;
+ $sortedServices[$priority][] = new Reference($serviceId);
+ }
+ }
+
+ krsort($sortedServices);
+
+ // Flatten the array
+ return call_user_func_array('array_merge', $sortedServices);
+ }
+}
View
14 src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
@@ -80,6 +80,7 @@ public function getConfigTreeBuilder()
$this->addTranslatorSection($rootNode);
$this->addValidationSection($rootNode);
$this->addAnnotationsSection($rootNode);
+ $this->addSerializerSection($rootNode);
return $treeBuilder;
}
@@ -393,4 +394,17 @@ private function addAnnotationsSection(ArrayNodeDefinition $rootNode)
->end()
;
}
+
+ private function addSerializerSection(ArrayNodeDefinition $rootNode)
+ {
+ $rootNode
+ ->children()
+ ->arrayNode('serializer')
+ ->info('serializer configuration')
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ ->canBeDisabled()
+ ->end()
+ ->end()
+ ;
+ }
+
@fabpot Owner
fabpot added a note

This blank line should be removed

@loalf
loalf added a note

I can't see a duplicate blank line cause there should be one blank line between "{" and "private ... ", shouldn't it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
View
17 src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
@@ -110,6 +110,10 @@ public function load(array $configs, ContainerBuilder $container)
$this->registerTranslatorConfiguration($config['translator'], $container);
}
+ if (isset($config['serializer'])) {
+ $this->registerSerializerConfiguration($config['serializer'], $loader);
+ }
+
$this->registerAnnotationsConfiguration($config['annotations'], $container, $loader);
$this->addClassesToCompile(array(
@@ -662,6 +666,19 @@ private function registerAnnotationsConfiguration(array $config, ContainerBuilde
}
/**
+ * Loads the Serializer configuration.
+ *
+ * @param array $config A Serializer configuration array
+ * @param XmlFileLoader $loader An XmlFileLoader instance
+ */
+ private function registerSerializerConfiguration(array $config, XmlFileLoader $loader)
+ {
+ if ($config['enabled']) {
+ $loader->load('serializer.xml');
+ }
+ }
+
+ /**
* Returns the base path for the XSD files.
*
* @return string The XSD base path
View
2  src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php
@@ -25,6 +25,7 @@
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\CompilerDebugDumpPass;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\TranslationExtractorPass;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\TranslationDumperPass;
+use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\SerializerPass;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\HttpRenderingStrategyPass;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Compiler\PassConfig;
@@ -66,6 +67,7 @@ public function build(ContainerBuilder $container)
$container->addCompilerPass(new AddCacheClearerPass());
$container->addCompilerPass(new TranslationExtractorPass());
$container->addCompilerPass(new TranslationDumperPass());
+ $container->addCompilerPass(new SerializerPass());
$container->addCompilerPass(new HttpRenderingStrategyPass(), PassConfig::TYPE_AFTER_REMOVING);
if ($container->getParameter('kernel.debug')) {
View
25 src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.xml
@@ -0,0 +1,25 @@
+<?xml version="1.0" ?>
+
+<container xmlns="http://symfony.com/schema/dic/services"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
+
+ <parameters>
+ <parameter key="serializer.class">Symfony\Component\Serializer\Serializer</parameter>
+ <parameter key="serializer.encoder.xml.class">Symfony\Component\Serializer\Encoder\XmlEncoder</parameter>
+ <parameter key="serializer.encoder.json.class">Symfony\Component\Serializer\Encoder\JsonEncoder</parameter>
+
+ <services>
+ <service id="serializer" class="%serializer.class%" >
+ <argument type="collection" />
+ <argument type="collection" />
+ </service>
+ <!-- Encoders -->
+ <service id="serializer.encoder.xml" class="%serializer.encoder.xml.class%" public="false" >
+ <tag name="serializer.encoder" />
+ </service>
+ <service id="serializer.encoder.json" class="%serializer.encoder.json.class%" public="false" >
+ <tag name="serializer.encoder" />
+ </service>
+ </services>
+</container>
View
105 src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Compiler/SerializerPassTest.php
@@ -0,0 +1,105 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\SerializerPass;
+
+/**
+ * Tests for the SerializerPass class
+ *
+ * @author Javier Lopez <f12loalf@gmail.com>
+ */
+class SerializerPassTest extends \PHPUnit_Framework_TestCase
+{
+
@fabpot Owner
fabpot added a note

Those blank lines should be removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ public function testThrowExceptionWhenNoNormalizers()
+ {
+ $container = $this->getMock('Symfony\Component\DependencyInjection\ContainerBuilder');
+
+ $container->expects($this->once())
+ ->method('hasDefinition')
+ ->with('serializer')
+ ->will($this->returnValue(true));
+
+ $container->expects($this->once())
+ ->method('findTaggedServiceIds')
+ ->with('serializer.normalizer')
+ ->will($this->returnValue(array()));
+
+ $this->setExpectedException('RuntimeException');
+
+ $serializerPass = new SerializerPass();
+ $serializerPass->process($container);
+ }
+
+ public function testThrowExceptionWhenNoEncoders()
+ {
+ $definition = $this->getMock('Symfony\Component\DependencyInjection\Definition');
+ $container = $this->getMock('Symfony\Component\DependencyInjection\ContainerBuilder');
+
+ $container->expects($this->once())
+ ->method('hasDefinition')
+ ->with('serializer')
+ ->will($this->returnValue(true));
+
+ $container->expects($this->any())
+ ->method('findTaggedServiceIds')
+ ->will($this->onConsecutiveCalls(
+ array('n' => array('serializer.normalizer')),
+ array()
+ ));
+
+ $container->expects($this->once())
+ ->method('getDefinition')
+ ->will($this->returnValue($definition));
+
+ $this->setExpectedException('RuntimeException');
+
+ $serializerPass = new SerializerPass();
+ $serializerPass->process($container);
+ }
+
+ public function testServicesAreOrderedAccordingToPriority()
+ {
+ $services = array(
+ 'n3' => array('tag' => array()),
+ 'n1' => array('tag' => array('priority' => 200)),
+ 'n2' => array('tag' => array('priority' => 100))
+ );
+
+ $expected = array(
+ new Reference('n1'),
+ new Reference('n2'),
+ new Reference('n3')
+ );
+
+ $container = $this->getMock('Symfony\Component\DependencyInjection\ContainerBuilder');
+
+ $container->expects($this->atLeastOnce())
+ ->method('findTaggedServiceIds')
+ ->will($this->returnValue($services));
+
+ $serializerPass = new SerializerPass();
+
+ $method = new \ReflectionMethod(
+ 'Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\SerializerPass',
+ 'findAndSortTaggedServices'
+ );
+ $method->setAccessible(TRUE);
+
+ $actual = $method->invoke($serializerPass, 'tag', $container);
+
+ $this->assertEquals($expected, $actual);
+ }
+}
Something went wrong with that request. Please try again.