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] extract normalizer tests to traits#30888
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
50d6a38 to37adf7fCompareUh oh!
There was an error while loading.Please reload this page.
dunglas 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.
What a work! This looks very promising.
| * You can raise this value for special cases, e.g. in combination with the | ||
| * max depth setting of the object normalizer. | ||
| */ | ||
| publicconstCIRCULAR_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.
What do you think about moving all these constants in the interface, and to add a builder object for better autocompletion in IDEs (exactly as done in HttpClient)?
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 agree that they should be in the interface. otherwise the interface is a lie. and we should define that when an option is not known by the normalizer implementation, it has to throw an exception. otherwise the behaviour is very undefined and e.g. typos are not spotted.
i also think we could use the OptionsResolver on them to validate, instead of the case-by-case validation we do at the moment (see#30907,#30950, its too easy to forget to validate things)
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.
@joelwurtz should we move the constants in this PR? i feel we should probably do that separately, also reviewing if they all stay the same or we want some different options and only keep some for legacy support.
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.
Like explained in#30818 we should have a global politic about context and where to put const of this context. I would be in favor of@dunglas proposal in order to be consistent with how others components works.
But it's clearly not in the scope of this PR. And this is something that should be done once we are satisified with the overall implementation. First let's do a good implementation, then we focus on DX for that, WDYT@dunglas ?
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.
otherwise the behaviour is very undefined and e.g. typos are not spotted
It’s not really doable because most likely the context will be passed to the Serializer instance, that will call many normalizers (another normalizer can be used for relations for instance).
+1 to move these constants in another PR.
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.
could we then make the serializer validate the context? e.g. by collecting from all its registered normalizers what options they support?
i am just very unhappy with these kind of unvalidated array configuration, as its super hard to debug when you make mistakes.
is the context supposed to also contain custom options for specific normalizers or is it a closed list of features? if it is closed, we could change from array to a value object that does the validation and defines what keys are possible. then we could do the validation once in that object and all normalizers can rely on only getting valid information.
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.
is the context supposed to also contain custom options for specific normalizers or is it a closed list of features?
Yes, it is an extension point for custom normalizers and encoders too. It’s used (and sometimes abused) a lot by API Platform and many other tools using this component.
What we would to is to validate when using the new « context builder » as in HttpClient (when using this builder, we know the list of keys) and don’t validate anything (as currently) in the normalizers.
Another alternative would be to introduce a new parameter for « built-in » options that would be a class, but keep the context as an extension point (I’m less fond of this one because it will make the public API harder to learn).
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.
could we make the $context into a value object with explicit methods (and validation) for the known options, and a generic getExtra(string $name), setExtra(string $name, mixed $thing) method? this would make a good compromise to solve validation and the typo risk issue while keeping flexibility.
i dislike that every context validates the context parameters again, as they can be called repeatedly.
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 out of scope for this pull request. i don't have time for another pull request for it, but if somebody wants to do it, please do
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedApr 7, 2019
Please note that we will ask you to squash the git history of the PR before merging, to keep at most one commit per author. |
joelwurtz commentedApr 8, 2019
About squashing, if it's easier for you@dbu i don't mind having my name removed from the commit list |
dbu commentedApr 8, 2019
i squashed. i don't feel much of an author on those, mostly moved the tests from elsewhere :-D |
fabpot commentedApr 8, 2019
@dbu Is it still WIP? |
dbu commentedApr 8, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
…malizer context (dbu)This PR was merged into the 4.2 branch.Discussion----------[Serializer] Also validate callbacks when given in the normalizer context| Q | A| ------------- | ---| Branch? | 4.2 (callbacks are handled differently in 3.4)| Bug fix? | yes| New feature? | no| BC breaks? | no (unless somebody relied on this bug ignoring `null` as callback| Deprecations? | no| Tests pass? | yes| Fixed tickets | Related to#30888| License | MIT| Doc PR | -callbacks configuration for the normalizer is validated to be valid callbacks when using setCallbacks or using the callbacks field in the default options. however, it was not validated when using the callbacks field in a context passed to `normalize()`Commits-------3789152 [serializer] validate that the specified callbacks and max_depth_handler are actually callable
73d435f to3dfd83bCompareUh oh!
There was an error while loading.Please reload this page.
4fb71a7 to22c1092Comparedbu commentedApr 11, 2019
I now consider this ready for final review /cc@joelwurtz@dunglas the open points can be done in follow-up pull requests and i don't have any more time to start fresh things (but will of course wrap up if you have feedback on this PR so that we can merge it) |
… isAllowedAttribute (dbu)This PR was merged into the 3.4 branch.Discussion----------property normalizer should also pass format and context to isAllowedAttribute| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | found while working on#30888| License | MIT| Doc PR | -the context and format are optional parameters to `isAllowedAttribute`, but should be forwarded. due to this omission, the PropertyNormalizer was ignoring the 'attributes' context option (and does in version 4 also ignore the 'ignore_attributes' context option - that one is a property on the normalizer class in version 3 and therefore not ignored here)Commits-------13e2fb7 property normalizer should also pass format and context to isAllowedAttribute
22c1092 toc83ea84Comparedbu commentedApr 26, 2019
what do we need to be able to merge this? |
c83ea84 to2b6ebeaComparefabpot commentedApr 27, 2019
Thank you@dbu. |
This PR was squashed before being merged into the 4.3-dev branch (closes#30888).Discussion----------[serializer] extract normalizer tests to traitseufossa| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | Relates to#30818| License | MIT| Doc PR | -As discussed with@joelwurtz, extract normalizer functionality tests into traits to ensure consistent behaviour of all normalizers.* [x] Rebase when#30977,#30950 and#30907 are merged to master **blocker*** [x] Clean up order of trait inclusion and methods in the tests* [x] Clean up fixture classes of the traits. I started having one class named the same as the trait, where possibleStuff that we should do eventually, but can also do in separate pull requests, after this one has been merged:* [ ] Extract all features that we can (the existing normalizer tests should more or less only have the legacy tests in them, all functionality should be in trait)* [ ] Run test coverage and increase coverage so that we cover all important features and all relevant error cases.Commits-------2b6ebea [serializer] extract normalizer tests to traits
This PR was merged into the 3.4 branch.Discussion----------[Serializer] Fix tests| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | n/a| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/a#30888 added some tests on master which fail when `property-info <3.4.13` is installed.This bumps the constraint on 3.4 which will fix the low-deps build once merged up to master.Commits-------d25f650 [Serializer] Fix tests
joelwurtz commentedApr 29, 2019
Thanks you@dbu will update my prs with those test |
…c (dunglas)This PR was merged into the 4.3 branch.Discussion----------[Serializer] Fix BC break: DEPTH_KEY_PATTERN must be public| Q | A| ------------- | ---| Branch? | 4.3 Bug fix? | yes| 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 | n/a| License | MIT| Doc PR | n/aFix a BC break introduced by#30888. This constant is used by API Platform.Commits-------1bdf2af [Serializer] Fix BC break: DEPTH_KEY_PATTERN must be public
Uh oh!
There was an error while loading.Please reload this page.
eufossa
As discussed with@joelwurtz, extract normalizer functionality tests into traits to ensure consistent behaviour of all normalizers.
Stuff that we should do eventually, but can also do in separate pull requests, after this one has been merged: