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] Fix collecting only first missing constructor argument#51907
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
[Serializer] Fix collecting only first missing constructor argument#51907
Uh oh!
There was an error while loading.Please reload this page.
Conversation
y4roc commentedOct 9, 2023
Your solution looks great, but I see a conflict with RequestMapper. Do you tested your solution with PayloadMapper in 6.3? |
258ae0f tocd90ab7CompareHypeMC commentedOct 10, 2023
@y4roc Hi, you were right, my changes did cause an issue with the |
| ); | ||
| $context['not_normalizable_value_exceptions'][] =$exception; | ||
| return$reflectionClass->newInstanceWithoutConstructor(); |
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 now handled in thetry...catch block.
cd90ab7 to0f398ceComparenicolas-grekas commentedOct 17, 2023
Thank you@HypeMC. |
This PR was merged into the 5.4 branch.Discussion----------[Serializer] Fix denormalize constructor arguments| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues |Fix#52499,Fix#52422| License | MITSince this PR:#51907, objects with partial constructor parameters were wrongly instantiated.This PR fixes that issue by delegating the properties values assignment, by unsetting normalized data only when the constructor has been called properly.This might correct#50759 as well.Commits-------8f7c7ae [Serializer] Fix denormalize constructor arguments
Uh oh!
There was an error while loading.Please reload this page.
Alternative to#51866, sort of followup to#49832
Currently on 5.4 only the first exception is added to the
not_normalizable_value_exceptionsarray whenCOLLECT_DENORMALIZATION_ERRORSistrueor only the first argument is mentioned in theMissingConstructorArgumentsExceptionwhen it isfalse.On 6.3 however, the part with the
MissingConstructorArgumentsExceptionwas fix with#49832, but the part with thenot_normalizable_value_exceptionswas overlooked.IMO this is inconsistent behavior as the two cases are actually the same thing with the only difference being that in one case an exception is thrown while in the other the errors are collected.
I'm not sure if#51866 really qualifies as a bug or is actually more a feature, but the reason#49832 was merged onto 6.3 was because of the changes originally done in#49013, which itself was a feature.
If#51866 does qualify as a bug then it would make sense to backport#49832 to 5.4 for consistency, which is what this PR does.
The PR contains two commits:
If#51866 does not qualify as a bug, the first commit can be drooped and the second one can be rebased with 6.4.
PS I think it's easier to review the changes commit by commit.