[Serializer] Allow to access to the format and context in circular ref handler #27020
Conversation
| * @param object $object | ||
| * @param object $object | ||
| * @param string|null $format | ||
| * @param array $context |
There was a problem hiding this comment.
all these can be removed imho
There was a problem hiding this comment.
It allows the IDE to discover those parameters. They could be dropped only when/if we'll uncomment the new parameters.
There was a problem hiding this comment.
ok, then next question for me is: how will we make these real args in 5.0?
shouldn't we throw a deprecation somehow?
There was a problem hiding this comment.
do we want them to be optional in 5.x ? I would say no (otherwise you would not be able to rely on them in the handler).
so I would trigger a deprecation in case they are not provided when calling the method.
and I suggest making this method @final, as the proper way to extend the handling of circular references is to register a handler, not to extend the method in child classes.
There was a problem hiding this comment.
making this method @Final
if that fits, then that's the best, as just adding the annotation will trigger a deprecation notice automatically
| { | ||
| $format = @func_get_arg(1) ?: null; | ||
| $context = @func_get_arg(2) ?: array(); |
There was a problem hiding this comment.
please use func_num_args instead of @
736e1f8 to
fa5b6fd
Compare
| * | ||
| * @return mixed | ||
| * | ||
| * @throws CircularReferenceException | ||
| */ | ||
| protected function handleCircularReference($object) | ||
| protected function handleCircularReference($object/*, ?string $format = null, array $context = array()*/) |
There was a problem hiding this comment.
the ? should be removed
don't miss adding @final also as discussed above
There was a problem hiding this comment.
I change it, but for reference:
Typed properties cannot have a null default value, unless the type is explicitly nullable (?Type). This is in contrast to parameter types, where a null default value automatically implies a nullable type. We consider this to be a legacy behavior, which we do not wish to support for newly introduced syntax.
There was a problem hiding this comment.
Maybe, but same as array() vs []: we prefer consistency across branches, and not all of them support this, so it's too early to consider.
fa5b6fd to
b58cae4
Compare
| @@ -190,16 +190,23 @@ protected function isCircularReference($object, &$context) | |||
| * If a circular reference handler is set, it will be called. Otherwise, a | |||
| * {@class CircularReferenceException} will be thrown. | |||
| * | |||
| * @param object $object | |||
| * @final | |||
There was a problem hiding this comment.
@Final since Symfony 4.2
b58cae4 to
fc704cf
Compare
fc704cf to
99f829e
Compare
|
Thank you @dunglas. |
… in circular ref handler (dunglas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Serializer] Allow to access to the format and context in circular ref handler | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- 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 Similar to #27017 but for circular reference handlers. ping @meyerbaptiste Commits ------- 99f829e [Serializer] Allow to access to the format and context in circular ref handler
…rters (dunglas) This PR was squashed before being merged into the 4.2-dev branch (closes #27021). Discussion ---------- [Serializer] Allow to access extra infos in name converters | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes<!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- 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 | License | MIT | Doc PR | todo Similar to #27017 and #27020 but for name converters. ping @meyerbaptiste Commits ------- 57fe017 [Serializer] Allow to access extra infos in name converters
…other infos in callbacks and max depth handler (dunglas, javiereguiluz) This PR was merged into the master branch. Discussion ---------- [Serializer] Allow to access to the context and various other infos in callbacks and max depth handler symfony/symfony#27020 symfony/symfony#27017 Commits ------- 672b374 Minor reword 7c3e620 Fix, and document setCircularReferenceHandler's new parameters a4dad43 [Serializer] Allow to access to the context and various other infos in callbacks and max depth handler
…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 <!-- see https://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
Similar to #27017 but for circular reference handlers.
ping @meyerbaptiste