Allow serializer context configuration #28927

soyuka · 2018-10-19T11:25:53Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	todo if we go with it

This allow to configure the context for normalizer's on a high level:

serializer:
    default_context:
        enable_max_depth: true

This feature was asked in api-platform/core#1764 (comment) and I thought it could be in symfony instead. The messenger component has the same configuration.

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

soyuka · 2018-10-23T07:59:39Z

Updated to reflect the changes from #28709

ro0NL · 2018-10-31T09:22:08Z

also see #19456 btw :)

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

soyuka · 2018-11-13T15:08:57Z

@dunglas updated, lmk if this isn't what you had in mind.

src/Symfony/Component/Serializer/DependencyInjection/SerializerPass.php

src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd

src/Symfony/Component/Serializer/DependencyInjection/SerializerPass.php

weaverryan · 2019-01-10T02:47:41Z

Ping! This is a pretty important feature, right? Because in 4.2, for example, there is no way to set up a circular reference handler globally, except by using the deprecated circular_reference_handler option (correct me if I'm wrong).

@soyuka it looks like a few changes were requested - could you make this?

Cheers!

soyuka · 2019-01-29T09:00:18Z

I rebased only for now.

I think the best practice would be to:

A) For core services, wire it up manually in the services XML file - don't use binding.
B) For user-implemented normalizers, I can see 2 options:

1. Adding some sort of `DefaultContextAwareNormalizerInterface` with a `setDefaultContext()` method (we check for the interface, then add this call)
   or

2. Wrap the normalizer context into a service (e.g. `serializer.default_context`) and document that people can DI this into their custom normalizers. We could even add an autowireable alias for this service so that simply type-hinting the service class will pass it to them.

@dunglas any opinion about this? I still have issues with the default_context argument because it's not declared in the services file (serializer.xml). I also this that having the context in a service might be a good idea.

dunglas

Almost done!

dunglas · 2019-02-06T13:33:16Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd

@@ -236,6 +236,7 @@
    <xsd:complexType name="serializer">
        <xsd:choice minOccurs="0" maxOccurs="unbounded">
            <xsd:element name="mapping" type="file_mapping" />
+            <xsd:element name="default_context" type="metadata" minOccurs="0" maxOccurs="1" />


default-context, for consistency

dunglas · 2019-02-06T13:34:22Z

src/Symfony/Component/Serializer/DependencyInjection/SerializerPass.php

+        $this->addDefaultContextParameterBinding($encoders, $container);
+    }
+
+    private function addDefaultContextParameterBinding($services, $container)


Suggested change

private function addDefaultContextParameterBinding($services, $container)

private function addDefaultContextParameter($services, $container)

Binding is confusing, let's remove it

dunglas

LGTM

weaverryan · 2019-02-19T15:30:52Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

+                            ->useAttributeAsKey('name')
+                            ->defaultValue([])
+                            ->prototype('variable')->end()
+                        ->end()


This doesn't allow for things like the circular_reference_handler. I mean, you can set it, but there's no way to set this to a service, for example - none of the handler stuff will work. We need to check for the handler options explicitly in FrameworkExtension and convert them to References. In the docs, I guess we'll need to document these handlers as services that should use the __invoke() method.

Maybe could be also do something more generic: check if a service corresponding to a given string exists, if it's the case, convert it to a reference. WDYT?

weaverryan

This is a really important one - thanks for the work! But I don't think it's quite done yet - handlers aren't possible, so it's still not really possible to handle the circular reference in the serializer in the framework.

weaverryan · 2019-02-19T15:33:00Z

src/Symfony/Component/Serializer/DependencyInjection/SerializerPass.php

+                    $definition->setArgument('$defaultContext', '%serializer.default_context%');
+                }
+            }
+        }


I don't love this level of magic. I'd prefer:

A) Just documenting the parameter and allowing people to wire it up manually
B) Creating a DefaultContext service that holds the config and passing that around. For the existing classes that are expecting an array, that's fine - we can pass the array to those classes, but make this service available.

I prefer B) because it will work with projects providing extra context options such as API Platform, and doing it manually is tedious.

What we could do:

Introduce a new ContextInterface (that can be extended)

Allow all parameters that currently accept an array to also accept this new interface, and deprecacate passing an array

Use it both for the "default context" and the context you can pass to methods (it means that this class MUST be safely serializable)

Other better option: create a context builder that returns an array. Then it will be consistent with the builder introduced in HttpClient.

Discussion about this in #30888

fabpot · 2019-03-31T09:03:29Z

@soyuka Can you take into account the last comments from @weaverryan?

soyuka · 2019-11-27T07:35:35Z

src/Symfony/Component/Serializer/Normalizer/ConstraintViolationListNormalizer.php

+        if(!$defaultContext instanceof ContextInterface && is_array($defaultContext)){
+            $defaultContext = new DefaultContext($defaultContext);
+            @trigger_error("you must pass an object of type ". ContextInterface::class .", the passage of an array is depreciated", E_USER_DEPRECATED);
+        }


@weaverryan @dunglas is this what you had in mind? We (me and @flug) are working actively on this, we changed only one normalizer for now and wanted to check the implementation with you before going forward.

No it's not. As I've described in the other comments, I think we should keep an array or an iterable here. Using an interface will make the component hard to extend, and it will be a pain to migrate the current API Platform context options to this new mechanism. All other Symfony components use plain arrays for this kind of things, for consistency, and because it's easier to extend, I think we should keep the array but just provide an helper function to build this context instead.

Neirda24 · 2019-11-27T09:33:55Z

src/Symfony/Component/Serializer/Normalizer/ConstraintViolationListNormalizer.php

@@ -32,8 +34,12 @@ class ConstraintViolationListNormalizer implements NormalizerInterface, Cacheabl
    private $defaultContext;
    private $nameConverter;

-    public function __construct($defaultContext = [], NameConverterInterface $nameConverter = null)
+    public function __construct(/** ContextInterface */ $defaultContext = null, NameConverterInterface $nameConverter = null)


is null a good idea ? It does not seems to be allowed before.

It should be an iterable defaulting to an empty array IMHO

dunglas · 2019-11-29T10:02:39Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

+                            ->useAttributeAsKey('name')
+                            ->defaultValue([])
+                            ->prototype('variable')->end()
+                        ->end()


Maybe could be also do something more generic: check if a service corresponding to a given string exists, if it's the case, convert it to a reference. WDYT?

dunglas · 2019-11-29T10:27:00Z

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/full.xml

-        <framework:serializer enabled="true" enable-annotations="true" name-converter="serializer.name_converter.camel_case_to_snake_case" circular-reference-handler="my.circular.reference.handler" max-depth-handler="my.max.depth.handler" />
+        <framework:serializer enabled="true" enable-annotations="true" name-converter="serializer.name_converter.camel_case_to_snake_case" circular-reference-handler="my.circular.reference.handler" max-depth-handler="my.max.depth.handler">
+            <framework:default-context>
+                <framework:enable_max_depth>true</framework:enable_max_depth>


Shouldn't this key allow any "array-like" structure? Shouldn't the XSD be updated accordingly?

dunglas · 2019-11-29T10:28:21Z

src/Symfony/Component/Serializer/Context/ContextInterface.php

+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+interface ContextInterface


Do we need this interface? Cannot we use the builder pattern as done in HttpClient (the HttpOptions helper) instead? This new class should contain a method for every available options for autocompletion, as in HttpClient.

dunglas · 2019-11-29T10:28:58Z

src/Symfony/Component/Serializer/Context/DefaultContext.php

+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+class DefaultContext implements ContextInterface


final, but IMHO we should get read of this class and use an array or an iterable instead.

dunglas · 2019-11-29T10:31:01Z

src/Symfony/Component/Serializer/Normalizer/ConstraintViolationListNormalizer.php

@@ -32,8 +34,12 @@ class ConstraintViolationListNormalizer implements NormalizerInterface, Cacheabl
    private $defaultContext;
    private $nameConverter;

-    public function __construct($defaultContext = [], NameConverterInterface $nameConverter = null)
+    public function __construct(/** ContextInterface */ $defaultContext = null, NameConverterInterface $nameConverter = null)


It should be an iterable defaulting to an empty array IMHO

dunglas · 2019-11-29T10:32:55Z

src/Symfony/Component/Serializer/Normalizer/ConstraintViolationListNormalizer.php

+        if(!$defaultContext instanceof ContextInterface && is_array($defaultContext)){
+            $defaultContext = new DefaultContext($defaultContext);
+            @trigger_error("you must pass an object of type ". ContextInterface::class .", the passage of an array is depreciated", E_USER_DEPRECATED);
+        }


No it's not. As I've described in the other comments, I think we should keep an array or an iterable here. Using an interface will make the component hard to extend, and it will be a pain to migrate the current API Platform context options to this new mechanism. All other Symfony components use plain arrays for this kind of things, for consistency, and because it's easier to extend, I think we should keep the array but just provide an helper function to build this context instead.

soyuka · 2019-11-29T15:15:43Z

Okay I've give up the Context class, I could add some builder like the HttpOptions but, according to the different normalizers, we have different constants that don't necessarily have the same meaning. To do something that corresponds to

B) Creating a DefaultContext service that holds the config and passing that around. For the existing classes that are expecting an array, that's fine - we can pass the array to those classes, but make this service available.

We'd need to choose which of the options we would declare in this DefaultContext.

fabpot · 2020-10-07T15:56:03Z

We've just moved away from master as the main branch to use 5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to 5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

… context configuration (soyuka) This PR was merged into the 5.4 branch. Discussion ---------- [FrameworkBundle][Serializer] Allow serializer default context configuration | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | yes  | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | todo if we go with it  This allow to configure the `context` for normalizer's on a high level: ```yaml serializer: default_context: enable_max_depth: true ``` This feature was asked in api-platform/core#1764 (comment) and I thought it could be in symfony instead. The messenger component has the same configuration. This re-opens #28927 (for discussion). I'm still unsure about the discussion in #28927 (comment) and what solution I should implement. Please let me know if you come to an agreement. Commits ------- 43d1ca5 Allow serializer default context configuration

… context configuration (soyuka) This PR was merged into the 5.4 branch. Discussion ---------- [FrameworkBundle][Serializer] Allow serializer default context configuration | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | yes  | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | todo if we go with it  This allow to configure the `context` for normalizer's on a high level: ```yaml serializer: default_context: enable_max_depth: true ``` This feature was asked in api-platform/core#1764 (comment) and I thought it could be in symfony instead. The messenger component has the same configuration. This re-opens symfony/symfony#28927 (for discussion). I'm still unsure about the discussion in symfony/symfony#28927 (comment) and what solution I should implement. Please let me know if you come to an agreement. Commits ------- 43d1ca5f86 Allow serializer default context configuration

carsonbot added Status: Needs Review Feature labels Oct 19, 2018

ro0NL reviewed Oct 19, 2018

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php Outdated Show resolved Hide resolved

ogizanagi added Serializer FrameworkBundle labels Oct 20, 2018

nicolas-grekas added this to the next milestone Oct 20, 2018

dunglas requested changes Oct 23, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 23, 2018

soyuka force-pushed the configure-serializer-context branch 2 times, most recently from 32c07e3 to 2d373bd Compare October 23, 2018 07:59

soyuka force-pushed the configure-serializer-context branch from 2d373bd to fc79c72 Compare October 23, 2018 08:21

dunglas requested changes Nov 8, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated Show resolved Hide resolved

soyuka force-pushed the configure-serializer-context branch 3 times, most recently from 606f504 to dbe12fe Compare November 13, 2018 16:45

soyuka commented Nov 14, 2018

View reviewed changes

src/Symfony/Component/Serializer/DependencyInjection/SerializerPass.php Outdated Show resolved Hide resolved

dunglas requested changes Nov 14, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd Outdated Show resolved Hide resolved

src/Symfony/Component/Serializer/DependencyInjection/SerializerPass.php Outdated Show resolved Hide resolved

dunglas mentioned this pull request Nov 16, 2018

Add a default context in concerned normalizers api-platform/core#2335

Merged

soyuka force-pushed the configure-serializer-context branch from dbe12fe to f872a36 Compare January 29, 2019 08:58

soyuka force-pushed the configure-serializer-context branch 5 times, most recently from 60c9bc8 to 1036834 Compare February 6, 2019 13:20

dunglas requested changes Feb 6, 2019

View reviewed changes

soyuka force-pushed the configure-serializer-context branch 2 times, most recently from fa568f1 to 8ae1e25 Compare February 6, 2019 13:41

dunglas approved these changes Feb 6, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Feb 6, 2019

weaverryan reviewed Feb 19, 2019

View reviewed changes

weaverryan requested changes Feb 19, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Feb 19, 2019

dunglas mentioned this pull request Mar 6, 2019

Document the use of ignored_attributes context option symfony/symfony-docs#11088

Closed

ogizanagi mentioned this pull request Apr 8, 2019

[Serializer] Encoder options from the framework bundle #28771

Closed

rimas-kudelis mentioned this pull request Sep 4, 2019

[Serializer] allow more default context configuration via yaml #33420

Closed

dunglas mentioned this pull request Nov 20, 2019

Improve Vulcain support api-platform/core#3275

Merged

1 task

soyuka force-pushed the configure-serializer-context branch from 8ae1e25 to ee29ffa Compare November 27, 2019 07:33

soyuka commented Nov 27, 2019

View reviewed changes

Neirda24 reviewed Nov 27, 2019

View reviewed changes

soyuka force-pushed the configure-serializer-context branch from ee29ffa to 97e8829 Compare November 29, 2019 09:06

dunglas requested changes Nov 29, 2019

View reviewed changes

Allow serializer default context configuration

b0a7d0e

soyuka force-pushed the configure-serializer-context branch from 97e8829 to b0a7d0e Compare March 5, 2020 09:22

fabpot closed this Oct 7, 2020

nicolas-grekas added the for 5.x label Oct 7, 2020

soyuka mentioned this pull request Oct 13, 2020

[FrameworkBundle][Serializer] Allow serializer default context configuration #38542

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow serializer context configuration #28927

Allow serializer context configuration #28927

soyuka commented Oct 19, 2018 •

edited

Loading

soyuka commented Oct 23, 2018

ro0NL commented Oct 31, 2018

soyuka commented Nov 13, 2018

weaverryan commented Jan 10, 2019

soyuka commented Jan 29, 2019

dunglas left a comment

dunglas Feb 6, 2019

dunglas Feb 6, 2019

dunglas left a comment

weaverryan Feb 19, 2019

dunglas Nov 29, 2019 •

edited

Loading

weaverryan left a comment

weaverryan Feb 19, 2019

dunglas Feb 27, 2019

dunglas Mar 31, 2019

maxhelias Apr 8, 2019

fabpot commented Mar 31, 2019

soyuka Nov 27, 2019

dunglas Nov 29, 2019

Neirda24 Nov 27, 2019

dunglas Nov 29, 2019

dunglas Nov 29, 2019 •

edited

Loading

dunglas Nov 29, 2019

dunglas Nov 29, 2019

dunglas Nov 29, 2019

dunglas Nov 29, 2019

dunglas Nov 29, 2019

soyuka commented Nov 29, 2019

fabpot commented Oct 7, 2020

	private function addDefaultContextParameterBinding($services, $container)
	private function addDefaultContextParameter($services, $container)

Allow serializer context configuration #28927

Allow serializer context configuration #28927

Conversation

soyuka commented Oct 19, 2018 • edited Loading

soyuka commented Oct 23, 2018

ro0NL commented Oct 31, 2018

soyuka commented Nov 13, 2018

weaverryan commented Jan 10, 2019

soyuka commented Jan 29, 2019

dunglas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Mar 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka commented Nov 29, 2019

fabpot commented Oct 7, 2020

soyuka commented Oct 19, 2018 •

edited

Loading

dunglas Nov 29, 2019 •

edited

Loading

dunglas Nov 29, 2019 •

edited

Loading