-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Serializer] Provide context information from attribute for promoted properties #46680
Conversation
Failing tests seems unrelated. |
src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php
Outdated
Show resolved
Hide resolved
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 😅 |
src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php
Outdated
Show resolved
Hide resolved
@@ -123,6 +123,14 @@ public function testLoadContexts() | |||
$this->assertLoadedContexts($this->getNamespace().'\ContextDummy', $this->getNamespace().'\ContextDummyParent'); | |||
} | |||
|
|||
/** | |||
* @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
to
c51c4e6
Compare
Thank 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 #46680 Commits ------- 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/A I 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 #46680 Commits ------- 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 | MIT This 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 | MIT This fix doesn't have to be upmerged because it already has been covered by symfony/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 theAbstractNormalizer
and 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.