[Serializer] Refactor and uniformize the config by introducing a default context #28709

dunglas · 2018-10-03T15:45:48Z

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

This PR uniformizes how the Serializer's configuration is handled:

As currently, configuration options can be set using the context (options that weren't configurable using the context have been refactored to leverage it)
Normalizers and encoders' constructors now accept a "default context"
All existing global configuration flags (constructor parameters) have been deprecated in favor of this new default context
the stateless context is always tried first, then is the default context

Some examples:

// Configuring groups globally
// Before: not possible
// After
$normalizer = new ObjectNormalizer(/* deps */, ['groups' => 'the_default_group']);

// Escaping Excel-like formulas in CSV files
// Before
$encoder = new CsvEncoder(',', '"', '\\', '.', true);
// After
$encoder = new CsvEncoder(['csv_escape_formulas' => true]);
$encoder->normalize($data, 'csv', ['csv_escape_formulas' => false]); // Override for this call only

Benefits:

The DX is dramatically improved, configuration is always handled in similar way
The serializer can be used in fully stateless way
Every options can be configured globally
Classes that had constructors with a lot of parameters (like CsvEncoder) are now much easier to use
We'll be able to improve the documentation by adding a dictionary of all available context options for the whole component
Everything can be configured the same way

TODO in subsequent PRs:

Add a new option in framework bundle to configure the context globally
Uniformize the constants name (sometimes the name if FOO, sometimes FOO_KEY)
Fix the "bug" regarding the format configuration in DateTimeNormalizer::denormalize() (see comments)
Maybe: move $defaultContext as the first parameter (before required services?)
Make XmlEncoder stateless

dunglas · 2018-10-03T15:49:58Z

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

@@ -76,7 +94,7 @@ public function supportsNormalization($data, $format = null)
     */
    public function denormalize($data, $class, $format = null, array $context = array())
    {
-        $dateTimeFormat = isset($context[self::FORMAT_KEY]) ? $context[self::FORMAT_KEY] : null;
+        $dateTimeFormat = $context[static::FORMAT_KEY] ?? null;


Even before this PR, $this->format was ignored when denormalizing (but not when normalizing).
Changing this behavior introduces a BC break, and breaks the tests.

An alternative could be to introduce a new key, like denormalize_datetime_format, and to deprecate using datetime_format when denormalizing.

Anyway, it's out of this PR scope.

nicolas-grekas

Makes a lot of sense!

nicolas-grekas · 2018-10-03T16:04:26Z

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php

+            $val = ${$arg[0]};
+            if (${$arg[0]} !== $arg[2]) {
+                $this->defaultContext[$arg[1]] = $val;
+                @trigger_error(sprintf('The "%s" parameter is deprecated since Symfony 4.2, use the "%s" key of the context instead.', $arg[0], $arg[1]), E_USER_DEPRECATED);


instead of this complex logic, I'd suggest to now deal with mixed signatures: either the 1st arg is an array - or not and then we trigger one deprecation and we build the defaultcontext out of the arguments (like done in JsonDecode)

nicolas-grekas · 2018-10-03T16:07:39Z

src/Symfony/Component/Serializer/Encoder/XmlEncoder.php

+            $val = ${$arg[0]};
+            if (${$arg[0]} !== $arg[2]) {
+                $this->defaultContext[$arg[1]] = $val;
+                @trigger_error(sprintf('The "%s" parameter is deprecated since Symfony 4.2, use the "%s" key of the context instead.', $arg[0], $arg[1]), E_USER_DEPRECATED);


same note as for CsvEncoder

nicolas-grekas · 2018-10-03T16:08:57Z

src/Symfony/Component/Serializer/Encoder/XmlEncoder.php

        );
        foreach ($xmlOptions as $xmlOption => $documentProperty) {
-            if (isset($context[$xmlOption])) {
-                $document->$documentProperty = $context[$xmlOption];
+            if ($contextOption = ($context[$xmlOption] ?? $this->defaultContext[$xmlOption] ?? false)) {


no need for the brackets

nicolas-grekas · 2018-10-03T16:09:46Z

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

+
+    /**
+     * @deprecated since Symfony 4.2
+     *
     * @var int


let's drop the type meanwhile (same below :) )

nicolas-grekas · 2018-10-03T16:10:57Z

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

@@ -388,8 +441,8 @@ protected function instantiateObject(array &$data, $class, array &$context, \Ref
                    // Don't run set for a parameter passed to the constructor
                    $params[] = $parameterData;
                    unset($data[$key]);
-                } elseif (isset($context[static::DEFAULT_CONSTRUCTOR_ARGUMENTS][$class][$key])) {
-                    $params[] = $context[static::DEFAULT_CONSTRUCTOR_ARGUMENTS][$class][$key];
+                } elseif (null !== $param = ($context[static::DEFAULT_CONSTRUCTOR_ARGUMENTS][$class][$key] ?? $this->defaultContext[static::DEFAULT_CONSTRUCTOR_ARGUMENTS][$class][$key] ?? null)) {


extra brackets

nicolas-grekas · 2018-10-03T16:11:43Z

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

@@ -32,16 +32,19 @@
 */
 abstract class AbstractObjectNormalizer extends AbstractNormalizer
 {
-    const ENABLE_MAX_DEPTH = 'enable_max_depth';


lets keep the original and safe some merge conflicts

nicolas-grekas · 2018-10-03T16:12:16Z

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

                continue;
            }

            $attributeValue = $this->getAttributeValue($object, $attribute, $format, $context);
            if ($maxDepthReached) {
-                $attributeValue = \call_user_func($this->maxDepthHandler, $attributeValue, $object, $attribute, $format, $context);
+                $attributeValue = $maxDepthHandler($attributeValue, $object, $attribute, $format, $context);


lets keep the original and safe some merge conflicts

nicolas-grekas · 2018-10-03T16:14:59Z

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

@@ -49,17 +61,17 @@ public function normalize($object, $format = null, array $context = array())
        }

        $result = array(
-            'type' => $context['type'] ?? 'https://symfony.com/errors/validation',
-            'title' => $context['title'] ?? 'Validation Failed',
+            'type' => $context[static::TYPE] ?? $this->defaultContext[static::TYPE] ?? 'https://symfony.com/errors/validation',


self:: instead of static::?
generally we should use self:: when using constants when possible IMHO

nicolas-grekas · 2018-10-03T16:15:46Z

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

     *
     * @return object|null an object if things check out, null otherwise
     */
    protected function extractObjectToPopulate($class, array $context, $key = null)
    {
-        $key = $key ?: 'object_to_populate';
-
+        $key = $key ?? 'object_to_populate';


I'd suggest reverting all changes in this file

nicolas-grekas · 2018-10-03T16:16:08Z

src/Symfony/Component/Serializer/Tests/Encoder/JsonDecodeTest.php

@@ -50,7 +50,7 @@ public function decodeProvider()
        $assoc = array('foo' => 'bar');

        return array(
-            array('{"foo": "bar"}', $stdClass, array()),
+            //array('{"foo": "bar"}', $stdClass, array()),


to be cleaned up I suppose?

dunglas · 2018-10-03T20:25:29Z

@nicolas-grekas thanks for the review! All comments resolved.
The fabbot's failure is a false positive (applying its patch triggers an error in PHPStorm).

src/Symfony/Component/Serializer/Encoder/XmlEncoder.php

nicolas-grekas · 2018-10-03T21:14:03Z

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

@@ -32,20 +32,36 @@
    use ObjectToPopulateTrait;
    use SerializerAwareTrait;

-    const CIRCULAR_REFERENCE_LIMIT = 'circular_reference_limit';


why changing the order? (same above btw)

nicolas-grekas · 2018-10-03T21:15:15Z

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

+        }
+
+        if (null !== $timezone) {
+            @trigger_error(sprintf('The "timezone" parameter is deprecated since Symfony 4.2, use the "%s" key of the context instead.', self::TIMEZONE_KEY), E_USER_DEPRECATED);


lets merge with previous "if" as done in other cases

nicolas-grekas

That will be much cleaner! Ready on my side, thanks.

ostrolucky · 2018-10-04T22:26:04Z

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php

+            @trigger_error('Passing configuration options directly to the constructor is deprecated since Symfony 4.2, use the default context instead.', E_USER_DEPRECATED);
+
+            $defaultContext = array(
+                self::DELIMITER_KEY => $defaultContext,


all of these for first argument should use casting, in this case (string)

ostrolucky · 2018-10-04T22:31:54Z

src/Symfony/Component/Serializer/Encoder/XmlEncoder.php

-        $this->loadOptions = null !== $loadOptions ? $loadOptions : LIBXML_NONET | LIBXML_NOBLANKS;
-        $this->decoderIgnoredNodeTypes = $decoderIgnoredNodeTypes;
-        $this->encoderIgnoredNodeTypes = $encoderIgnoredNodeTypes;
+        $this->defaultContext = array(


Not sure why is this whole array not defined as default directly in property definition, like rest

Because the default values contain expressions (it's not allowed in properties definitions).

which one? LIBXML_NONET | LIBXML_NOBLANKS works also as it's resolved at compile time if that's what you have in mind

You're right it works! I guess it's because I was using static:: in a previous commit.

ostrolucky · 2018-10-04T22:41:59Z

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

            !isset($attributesMetadata[$attribute]) ||
            null === $maxDepth = $attributesMetadata[$attribute]->getMaxDepth()
        ) {
            return false;
        }

-        $key = sprintf(static::DEPTH_KEY_PATTERN, $class, $attribute);
+        $key = sprintf(self::DEPTH_KEY_PATTERN, $class, $attribute);


This is unrelated change and might break BC

It was actually not working in some cases. It's safer this way and will prevent people from relying on overriding such constants.

nicolas-grekas · 2018-10-05T08:20:05Z

src/Symfony/Component/Serializer/CHANGELOG.md

+   are deprecated, use the default context instead.
+ * passing configuration options directly to the constructor of `CsvEncoder`, `JsonDecode` and
+   `XmlEncoder` is deprecated since Symfony 4.2, use the default context instead.
+>>>>>>> [Serializer] Refactor and uniformize the config by introducing a default context


extra line here

nicolas-grekas · 2018-10-05T08:20:54Z

src/Symfony/Component/Serializer/Encoder/JsonDecode.php

-    private $associative;
-    private $recursionDepth;
+    /**
+     * True to return the result associative array, false for a nested stdClass hierarchy.


missing "as an" before "associative"?

the error was already is already present in the original docs but yes

nicolas-grekas · 2018-10-05T08:22:02Z

src/Symfony/Component/Serializer/Encoder/JsonEncode.php


-    public function __construct(int $bitmask = 0)
+    private $defaultContext = array(
+        'json_encode_options' => 0,


should use the constant instead I suppose?

It looks like it doesn't exist: https://github.com/php/php-src/blob/49a4e695845bf55e059e7f88e54b1111fe284223/ext/json/php_json.h#L61

I meant s/'json_encode_options'/self::JSON_ENCODE_OPTIONS/

ro0NL · 2018-10-20T10:00:45Z

src/Symfony/Component/Serializer/Encoder/XmlEncoder.php

     * @param string $name Root node name
     */
    public function setRootNodeName($name)
    {
-        $this->rootNodeName = $name;
+        @trigger_error(sprintf('The "%s()" method is deprecated since Symfony 4.1, use the context instead.', __METHOD__), E_USER_DEPRECATED);


ro0NL · 2018-10-20T10:02:17Z

src/Symfony/Component/Serializer/Encoder/XmlEncoder.php

     * @return string
     */
    public function getRootNodeName()
    {
-        return $this->rootNodeName;
+        @trigger_error(sprintf('The "%s()" method is deprecated since Symfony 4.1, use the context instead.', __METHOD__), E_USER_DEPRECATED);


ro0NL · 2018-10-20T10:03:17Z

src/Symfony/Component/Serializer/Encoder/XmlEncoder.php

    }

    /**
     * Returns the root node name.
     *
+     * @deprecated


@deprecated since Symfony 4.2

nicolas-grekas · 2018-10-20T19:42:39Z

(rebase needed + some comments to fix)

…ult context

…roducing a default context (dunglas) This PR was squashed before being merged into the 4.2-dev branch (closes #28709). Discussion ---------- [Serializer] Refactor and uniformize the config by introducing a default context | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes  | BC breaks? | no  | Deprecations? | yes  | Tests pass? | yes  | Fixed tickets | n/a  | License | MIT | Doc PR | todo  This PR uniformizes how the Serializer's configuration is handled: * As currently, configuration options can be set using the context (options that weren't configurable using the context have been refactored to leverage it) * Normalizers and encoders' constructors now accept a "default context" * All existing global configuration flags (constructor parameters) have been deprecated in favor of this new default context * the stateless context is always tried first, then is the default context Some examples: ```php // Configuring groups globally // Before: not possible // After $normalizer = new ObjectNormalizer(/* deps */, ['groups' => 'the_default_group']); // Escaping Excel-like formulas in CSV files // Before $encoder = new CsvEncoder(',', '"', '\\', '.', true); // After $encoder = new CsvEncoder(['csv_escape_formulas' => true]); $encoder->normalize($data, 'csv', ['csv_escape_formulas' => false]); // Override for this call only ``` Benefits: * The DX is dramatically improved, configuration is always handled in similar way * The serializer can be used in fully stateless way * Every options can be configured globally * Classes that had constructors with a lot of parameters (like `CsvEncoder`) are now much easier to use * We'll be able to improve the documentation by adding a dictionary of all available context options for the whole component * Everything can be configured the same way TODO in subsequent PRs: * Add a new option in framework bundle to configure the context globally * Uniformize the constants name (sometimes the name if `FOO`, sometimes `FOO_KEY`) * Fix the "bug" regarding the format configuration in `DateTimeNormalizer::denormalize()` (see comments) * Maybe: move `$defaultContext` as the first parameter (before required services?) * Make `XmlEncoder` stateless Commits ------- 52b186a [Serializer] Refactor and uniformize the config by introducing a default context

symfony/symfony#28709 PR had changed the parameter of some constructor in the serializer component therefore PSALM started to report InvalidArgument errors.

… default context solely (ogizanagi) This PR was squashed before being merged into the 5.0-dev branch (closes #31699). Discussion ---------- [Serializer] Unified normalizers/encoders config through default context solely | Q | A | ------------- | --- | Branch? | master  | Bug fix? | no | New feature? | no  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | #28709  | License | MIT | Doc PR | N/A As planned in #28709. Split into two commits to ease review. Commits ------- 914577e [Serializer] Unified normalizers/encoders config through default context solely

…anagi) This PR was merged into the 5.0-dev branch. Discussion ---------- [Serializer] Remove last deprecated/obsolete paths | Q | A | ------------- | --- | Branch? | master  | Bug fix? | no | New feature? | no  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | #28316, #28709, #31030, #27020, #29896, 16f8a13#r201060750  | License | MIT | Doc PR | N/A  This should fix the last deprecations & obsolete code paths for the Serializer component. Commits ------- c703b35 [Serializer] Remove last deprecated/obsolete paths

dunglas requested a review from nicolas-grekas October 3, 2018 15:45

carsonbot added Status: Needs Review Serializer Feature Deprecation labels Oct 3, 2018

dunglas force-pushed the new-serializer-config branch from add8f3b to a451444 Compare October 3, 2018 15:47

dunglas commented Oct 3, 2018

View reviewed changes

nicolas-grekas reviewed Oct 3, 2018

View reviewed changes

dunglas force-pushed the new-serializer-config branch 3 times, most recently from f1de005 to 6c2b902 Compare October 3, 2018 20:23

dunglas force-pushed the new-serializer-config branch 2 times, most recently from e63a99c to 83463d2 Compare October 3, 2018 21:05

nicolas-grekas reviewed Oct 3, 2018

View reviewed changes

src/Symfony/Component/Serializer/Encoder/XmlEncoder.php Show resolved Hide resolved

nicolas-grekas reviewed Oct 3, 2018

View reviewed changes

dunglas force-pushed the new-serializer-config branch 3 times, most recently from b6a1369 to 108eb0e Compare October 3, 2018 21:25

nicolas-grekas approved these changes Oct 3, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 3, 2018

ostrolucky suggested changes Oct 4, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Oct 4, 2018

dunglas force-pushed the new-serializer-config branch from 108eb0e to e9f4632 Compare October 5, 2018 08:11

nicolas-grekas reviewed Oct 5, 2018

View reviewed changes

ogizanagi mentioned this pull request Oct 8, 2018

[Serializer] Encoder options from the framework bundle #28771

Closed

dunglas mentioned this pull request Oct 10, 2018

[Serializer] Encode empty objects as objects, not arrays #28363

Merged

ogizanagi mentioned this pull request Oct 16, 2018

[Serializer] Add options to JsonDecode and JsonEncode to wrap/unwrap json data #28887

Closed

ro0NL mentioned this pull request Oct 19, 2018

Allow serializer context configuration #28927

Closed

soyuka mentioned this pull request Oct 19, 2018

Issue / Question about serializer @MaxDepth annotation api-platform/core#1764

Closed

ogizanagi approved these changes Oct 20, 2018

View reviewed changes

ro0NL reviewed Oct 20, 2018

View reviewed changes

dunglas force-pushed the new-serializer-config branch from e61e0f7 to cb48949 Compare October 21, 2018 10:37

[Serializer] Refactor and uniformize the config by introducing a defa…

52b186a

…ult context

dunglas force-pushed the new-serializer-config branch from cb48949 to 52b186a Compare October 23, 2018 06:25

dunglas merged commit 52b186a into symfony:master Oct 23, 2018

dunglas deleted the new-serializer-config branch October 23, 2018 06:27

weaverryan mentioned this pull request Oct 23, 2018

Default context for serializer symfony/symfony-docs#10542

Closed

This was referenced Oct 25, 2018

[Serializer] Refactor and uniformize the config by introducing a default context symfony/symfony-docs#10566

Closed

[Serializer] Refactor and uniformize the config by introducing a default context symfony/symfony-docs#10613

Closed

ro0NL mentioned this pull request Oct 30, 2018

DateTimeNormalizer ignores format when denormalize #29030

Closed

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

dunglas mentioned this pull request Nov 16, 2018

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

Merged

alanpoulain mentioned this pull request Mar 5, 2019

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

Closed

wouterj mentioned this pull request May 24, 2019

Update serializer.rst: deprecate the setCallbacks symfony/symfony-docs#11584

Merged

ogizanagi mentioned this pull request May 29, 2019

[Serializer] Unified normalizers/encoders config through default context solely #31699

Merged

ogizanagi mentioned this pull request May 31, 2019

[Serializer] Remove last deprecated/obsolete paths #31771

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] Refactor and uniformize the config by introducing a default context #28709

[Serializer] Refactor and uniformize the config by introducing a default context #28709

dunglas commented Oct 3, 2018 •

edited

Loading

dunglas Oct 3, 2018 •

edited by ogizanagi

Loading

nicolas-grekas left a comment

nicolas-grekas Oct 3, 2018

nicolas-grekas Oct 3, 2018

nicolas-grekas Oct 3, 2018

nicolas-grekas Oct 3, 2018

nicolas-grekas Oct 3, 2018

nicolas-grekas Oct 3, 2018

nicolas-grekas Oct 3, 2018

nicolas-grekas Oct 3, 2018

nicolas-grekas Oct 3, 2018

nicolas-grekas Oct 3, 2018

dunglas commented Oct 3, 2018

nicolas-grekas Oct 3, 2018

nicolas-grekas Oct 3, 2018

nicolas-grekas left a comment

ostrolucky Oct 4, 2018

ostrolucky Oct 4, 2018

dunglas Oct 5, 2018

nicolas-grekas Oct 5, 2018

dunglas Oct 5, 2018

ostrolucky Oct 4, 2018

dunglas Oct 5, 2018

nicolas-grekas Oct 5, 2018

nicolas-grekas Oct 5, 2018

dunglas Oct 5, 2018

nicolas-grekas Oct 5, 2018

dunglas Oct 5, 2018

nicolas-grekas Oct 5, 2018

ro0NL Oct 20, 2018

ro0NL Oct 20, 2018

ro0NL Oct 20, 2018

nicolas-grekas commented Oct 20, 2018

[Serializer] Refactor and uniformize the config by introducing a default context #28709

[Serializer] Refactor and uniformize the config by introducing a default context #28709

Conversation

dunglas commented Oct 3, 2018 • edited Loading

dunglas Oct 3, 2018 • edited by ogizanagi Loading

Choose a reason for hiding this comment

nicolas-grekas 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

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

dunglas commented Oct 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas 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

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

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

nicolas-grekas commented Oct 20, 2018

dunglas commented Oct 3, 2018 •

edited

Loading

dunglas Oct 3, 2018 •

edited by ogizanagi

Loading