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] Allow to access to the format and context in circular ref handler#27020
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
[Serializer] Allow to access to the format and context in circular ref handler#27020
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| * @param object $object | ||
| * @param object $object | ||
| * @param string|null $format | ||
| * @param array $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.
all these can be removed imho
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
making this method@Final
if that fits, then that's the best, as just adding the annotation will trigger a deprecation notice automatically
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.
+1 for final
| protectedfunctionhandleCircularReference($object/*, ?string $format = null, array $context = array()*/) | ||
| { | ||
| $format = @func_get_arg(1) ?:null; | ||
| $context = @func_get_arg(2) ?: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.
please use func_num_args instead of@
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.
done
736e1f8 tofa5b6fdCompare| * @throws CircularReferenceException | ||
| */ | ||
| protectedfunctionhandleCircularReference($object) | ||
| protectedfunctionhandleCircularReference($object/*, ?string $format = null, array $context = 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.
the? should be removed
don't miss adding@final also as discussed above
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 tob58cae4Compare
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.
LGTM, just one minor comment.
| * {@class CircularReferenceException} will be thrown. | ||
| * | ||
| * @param object $object | ||
| * @final |
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.
@Final since Symfony 4.2
b58cae4 tofc704cfComparefc704cf to99f829eComparenicolas-grekas commentedJul 7, 2018
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 <!-- seehttps://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 | todoSimilar to#27017 but for circular reference handlers.ping@meyerbaptisteCommits-------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 <!-- seehttps://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 | todoSimilar to#27017 and#27020 but for name converters.ping@meyerbaptisteCommits-------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 handlersymfony/symfony#27020symfony/symfony#27017Commits-------672b374 Minor reword7c3e620 Fix, and document setCircularReferenceHandler's new parametersa4dad43 [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 <!-- 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.
Similar to#27017 but for circular reference handlers.
ping@meyerbaptiste