Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Serializer] Fix unexpected allowed attributes#52917
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
fabbot errors aren't related to that PR. |
6839596 to9a3d2beComparesrc/Symfony/Component/Serializer/Normalizer/GetSetMethodNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Normalizer/GetSetMethodNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Normalizer/GetSetMethodNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Normalizer/GetSetMethodNormalizer.php OutdatedShow resolvedHide resolved
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.
As discussed at SymfonyCon, the information (access type) would better transit through a method parameter than a context option since the need is specific to the abstract method at hand
8f93524 toee9ccdeCompareOnce this PR will be upmerged to 7.1, I'll create a new one to trigger the proper deprecations. |
ee9ccde to3a87ebeCompare
nicolas-grekas left a comment• 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.
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 implementation fits the definition of a new feature to me: adding an argument on a constructor, and adding an argument on public/protected methods.
This should come with an@param annotation for methods, and this will yield a new deprecation.
Maybe another approach is possible? I don't know at all :)
That read/write mode do need to be passed throughout method calls to fix the bug at hand, so the plan here is to make it a parameter that's hidden at first on 5.4, then make it virtual on 7.1 and finally make it real on 8.0. @mtarld would that work conceptually and technically? |
e5df998 to46f78a9CompareIt appears that it works indeed, and it is much better thanks! I'm wondering if we should make that read/write information explicit in 7.1, maybe a context value like here is enough (but it is less secure). |
46f78a9 tob48471bCompareThank you@mtarld. |
The PropertyNormalizer does not handle a property info extractor. It lookslike the argument was accidentally added to instead of the ObjectNormalizerinsymfony#52917.
The PropertyNormalizer does not handle a property info extractor. It lookslike the argument was accidentally added to instead of the ObjectNormalizerinsymfony#52917.
The PropertyNormalizer does not handle a property info extractor. It lookslike the argument was accidentally added to instead of the ObjectNormalizerinsymfony#52917.
The PropertyNormalizer does not handle a property info extractor. It lookslike the argument was accidentally added to instead of the ObjectNormalizerinsymfony#52917.
The PropertyNormalizer does not handle a property info extractor. It lookslike the argument was accidentally added to instead of the ObjectNormalizerinsymfony#52917.
…or to the ObjectNormalizer (xabbuh)This PR was merged into the 5.4 branch.Discussion----------[FrameworkBundle] move wiring of the property info extractor to the ObjectNormalizer| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues || License | MITThe PropertyNormalizer does not handle a property info extractor. It looks like the argument was accidentally added to instead of the ObjectNormalizer in#52917.Commits-------518bc28 move wiring of the property info extractor to the ObjectNormalizer
… setters with optional args (HypeMC)This PR was merged into the 5.4 branch.Discussion----------[Serializer] Fix `GetSetMethodNormalizer` not working with setters with optional args| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues |Fix#54784| License | MITPrior to#52917 setters could have an optional argument or even multiple ones. This restores the previous behavior.Commits-------74bc0eb [Serializer] Fix `GetSetMethodNormalizer` not working with setters with optional args
Those changes generate a big performance drop in one of the projects I work with where we extensively use attributes on DTOs. Comparing Serializer in version |
Please open a dedicated issue if you want any follow up. |
…peMC)This PR was merged into the 5.4 branch.Discussion----------[Serializer] Fix `ObjectNormalizer` with property path| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues |Fix#54983| License | MITCaused by#52917.The `ObjectNormalizer::isAllowedAttribute()` method doesn't work with property paths, this is an attempt to fix the problem.Commits-------3857545 [Serializer] Fix `ObjectNormalizer` with property path
The PropertyNormalizer does not handle a property info extractor. It lookslike the argument was accidentally added to instead of the ObjectNormalizerinsymfony#52917.
A more accurate approach than#52680