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] prevent mixup in normalizer of the object to populate#30977
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
| return$object; | ||
| } | ||
| // clean up even if no match | ||
| unset($context[static::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.
this means we keep silently ignoring a non-matching object_to_populate. should we instead throw an exception when object to populate is set in the options but does not match? it seems to me that it indicates a programmer error.
not sure about BC of throwing the exception though. its in some way a bugfix, but could break existing code when relying on the serializer silently ignoring the mistake.
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 would not throw an exception in 3.4 for BC reason. We can keep the code as is in 3.4, generate a deprecation notice on master, and throw an exception in 5.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.
great plan! tell me when this is merged to master, then i do the deprecation pull request.
4ed438e tofdb668eComparefabpot commentedApr 8, 2019
Thank you@dbu. |
…populate (dbu)This PR was merged into the 3.4 branch.Discussion----------[serializer] prevent mixup in normalizer of the object to populateEUFOSSA| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -OBJECT_TO_POPULATE is meant to specify the top level object. The implementation left the option in the context and it would be used whenever we have the first element that matches the class.#30607 (to master) introduces the feature to also keep the instances of attributes to deeply populate an existing object tree. In both cases, we do not want the mix up to happen with what the current OBJECT_TO_POPULATE is.Commits-------fdb668e prevent mixup of the object to populate
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
EUFOSSA
OBJECT_TO_POPULATE is meant to specify the top level object. The implementation left the option in the context and it would be used whenever we have the first element that matches the class.#30607 (to master) introduces the feature to also keep the instances of attributes to deeply populate an existing object tree. In both cases, we do not want the mix up to happen with what the current OBJECT_TO_POPULATE is.