Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] Refactor and uniformize the config by introducing a default context#28709
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
add8f3b toa451444Compare| publicfunctiondenormalize($data,$class,$format =null,array$context =array()) | ||
| { | ||
| $dateTimeFormat =isset($context[self::FORMAT_KEY]) ?$context[self::FORMAT_KEY] :null; | ||
| $dateTimeFormat =$context[static::FORMAT_KEY] ??null; |
dunglasOct 3, 2018 • edited by ogizanagi
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by ogizanagi
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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, likedenormalize_datetime_format, and to deprecate usingdatetime_format when denormalizing.
Anyway, it's out of this PR scope.
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Makes a lot of sense!
| $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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
| $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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same note as for CsvEncoder
| foreach ($xmlOptionsas$xmlOption =>$documentProperty) { | ||
| if (isset($context[$xmlOption])) { | ||
| $document->$documentProperty =$context[$xmlOption]; | ||
| if ($contextOption =($context[$xmlOption] ??$this->defaultContext[$xmlOption] ??false)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no need for the brackets
| /** | ||
| * @deprecated since Symfony 4.2 | ||
| * | ||
| * @var int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
let's drop the type meanwhile (same below :) )
| 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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
extra brackets
| */ | ||
| abstractclass AbstractObjectNormalizerextends AbstractNormalizer | ||
| { | ||
| constENABLE_MAX_DEPTH ='enable_max_depth'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lets keep the original and safe some merge conflicts
| $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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lets keep the original and safe some merge conflicts
| $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', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
self:: instead of static::?
generally we should use self:: when using constants when possible IMHO
| { | ||
| $key =$key ?:'object_to_populate'; | ||
| $key =$key ??'object_to_populate'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd suggest reverting all changes in this file
| returnarray( | ||
| array('{"foo": "bar"}',$stdClass,array()), | ||
| //array('{"foo": "bar"}', $stdClass, array()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
to be cleaned up I suppose?
f1de005 to6c2b902Comparedunglas commentedOct 3, 2018
@nicolas-grekas thanks for the review! All comments resolved. |
e63a99c to83463d2CompareUh oh!
There was an error while loading.Please reload this page.
| use ObjectToPopulateTrait; | ||
| use SerializerAwareTrait; | ||
| constCIRCULAR_REFERENCE_LIMIT ='circular_reference_limit'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why changing the order? (same above btw)
| } | ||
| 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lets merge with previous "if" as done in other cases
b6a1369 to108eb0eComparenicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That will be much cleaner! Ready on my side, thanks.
| @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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
all of these for first argument should use casting, in this case (string)
| $this->loadOptions =null !==$loadOptions ?$loadOptions :LIBXML_NONET |LIBXML_NOBLANKS; | ||
| $this->decoderIgnoredNodeTypes =$decoderIgnoredNodeTypes; | ||
| $this->encoderIgnoredNodeTypes =$encoderIgnoredNodeTypes; | ||
| $this->defaultContext =array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure why is this whole array not defined as default directly in property definition, like rest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because the default values contain expressions (it's not allowed in properties definitions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
which one?LIBXML_NONET | LIBXML_NOBLANKS works also as it's resolved at compile time if that's what you have in mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're right it works! I guess it's because I was usingstatic:: in a previous commit.
| } | ||
| $key =sprintf(static::DEPTH_KEY_PATTERN,$class,$attribute); | ||
| $key =sprintf(self::DEPTH_KEY_PATTERN,$class,$attribute); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is unrelated change and might break BC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It was actually not working in some cases. It's safer this way and will prevent people from relying on overriding such constants.
108eb0e toe9f4632Compare| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
extra line here
| private$associative; | ||
| private$recursionDepth; | ||
| /** | ||
| * True to return the result associative array, false for a nested stdClass hierarchy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing "as an" before "associative"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the error was already is already present in the original docs but yes
| publicfunction__construct(int$bitmask =0) | ||
| private$defaultContext =array( | ||
| 'json_encode_options' =>0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should use the constant instead I suppose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It looks like it doesn't exist:https://github.com/php/php-src/blob/49a4e695845bf55e059e7f88e54b1111fe284223/ext/json/php_json.h#L61
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I meant s/'json_encode_options'/self::JSON_ENCODE_OPTIONS/
| publicfunctionsetRootNodeName($name) | ||
| { | ||
| $this->rootNodeName =$name; | ||
| @trigger_error(sprintf('The "%s()" method is deprecated since Symfony 4.1, use the context instead.',__METHOD__),E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
4.2
| publicfunctiongetRootNodeName() | ||
| { | ||
| return$this->rootNodeName; | ||
| @trigger_error(sprintf('The "%s()" method is deprecated since Symfony 4.1, use the context instead.',__METHOD__),E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
4.2
| /** | ||
| * Returns the root node name. | ||
| * | ||
| * @deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@deprecated since Symfony 4.2
nicolas-grekas commentedOct 20, 2018
(rebase needed + some comments to fix) |
e61e0f7 tocb48949Comparecb48949 to52b186aCompare…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 <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | yes <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | n/a <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | todo <!-- required for new features -->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 contextSome 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 wayTODO 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` statelessCommits-------52b186a [Serializer] Refactor and uniformize the config by introducing a default context
symfony/symfony#28709 PR had changed theparameter of some constructor in the serializer component thereforePSALM started to report InvalidArgument errors.
symfony/symfony#28709 PR had changed theparameter of some constructor in the serializer component thereforePSALM started to report InvalidArgument errors.
symfony/symfony#28709 PR had changed theparameter of some constructor in the serializer component thereforePSALM 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 <!-- see below -->| Bug fix? | no| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets |#28709 <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/AAs 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 <!-- see below -->| Bug fix? | no| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets |#28316,#28709,#31030,#27020,#29896,16f8a13#r201060750 <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/A <!-- required for new features -->This should fix the last deprecations & obsolete code paths for the Serializer component.Commits-------c703b35 [Serializer] Remove last deprecated/obsolete paths
Uh oh!
There was an error while loading.Please reload this page.
This PR uniformizes how the Serializer's configuration is handled:
Some examples:
Benefits:
CsvEncoder) are now much easier to useTODO in subsequent PRs:
FOO, sometimesFOO_KEY)DateTimeNormalizer::denormalize()(see comments)$defaultContextas the first parameter (before required services?)XmlEncoderstateless