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] Provide context information from attribute for promoted properties#46680
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
Failing tests seems unrelated. |
src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Thanks for submitting this ❤️ I think it should target 6.2 however, as a new feature rather than a bug fix |
Well i guess its debatable if its a bug or feature. Since the Attribute supports to be set on properties and 5.4 is supporting php 8.0 you could say it's a bug. But I'm fine with saying this is a new feature 😅 |
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| /** | ||
| * @requires PHP 8 |
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@requires won't be necessary if we merge this to 6.2
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.
Feel free to merge it either in 4.4 (as i saw this is latest maintained version) or 6.2. Or do you need me to update this here?
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 attribute is not available in 4.4 yet.
Since@dunglas seems to agree with me this should be considered a new feature, as of PHP 8 specific feature, we'll merge it to 6.2. The rebase and changing this line is not necessary, we can do it while merging, but updating the PR to the right branch might be helpful to detect unexpected issues on the CI.
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.
For 6.2
1f3d272 toc51c4e6CompareThank you@DanielBadura. |
This PR was merged into the 6.2 branch.Discussion----------[Serializer] fix AbstractObjectNormalizer| Q | A| ------------- | ---| Branch? | 6.2| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Fixes#46680Commits-------fdb42bb [Serializer] fix AbstractObjectNormalizer
…cedNameConverterInterface::denormalize (ogizanagi)This PR was merged into the 6.2 branch.Discussion----------[Serializer] Cannot use Context attribute context in AdvancedNameConverterInterface::denormalize| Q | A| ------------- | ---| Branch? | 6.2 <!-- see below -->| Bug fix? | yes| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets |Fix#46680 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->| License | MIT| Doc PR | N/AI realized we can't use the `Context` attribute context in an `AdvancedNameConverterInterface::denormalize`, same as spotted already in 5.4 in#46525.Also adds the missing CHANGELOG entry for#46680Commits-------c99082c [Serializer] Cannot use@context context in AdvancedNameConverterInterface::denormalize
This PR was merged into the 5.4 branch.Discussion----------[Serializer] Fix constructor deserialization path| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues |Fix#44925| License | MITThis fix doesn't have to be upmerged because it already has been covered by#46680.Commits-------272bc28 [Serializer] Fix constructor deserialization path
This PR was merged into the 5.4 branch.Discussion----------[Serializer] Fix constructor deserialization path| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues | Fix #44925| License | MITThis fix doesn't have to be upmerged because it already has been covered bysymfony/symfony#46680.Commits-------272bc28763d [Serializer] Fix constructor deserialization path
…ext value twice (xabbuh)This PR was merged into the 6.3 branch.Discussion----------[Serializer] do not detect the deserialization_path context value twice| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | no| New feature? | no| Deprecations? | no| Issues || License | MIT#52713 must not be applied on 6.3+ as the logic is already part of the `getAttributeDenormalizationContext()` method introduced in#46680.Commits-------9b027a5 do not detect the deserialization_path context value twice
I refactored some classes to using property promotion wiht
#[Context]attributes and my tests started to fail since the context was missing then. So i tried to retrieve this context at theAbstractNormalizerand added a testcase for that.I'm not sure if this is the right place for getting this metadata information and if i have overseen something.