Skip to content
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] Fix denormalize constructor arguments #52578

Merged

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Nov 14, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #52499, Fix #52422
License MIT

Since 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.

@mtarld
Copy link
Contributor Author

mtarld commented Nov 14, 2023

fabbot errors aren't related.

@mtarld mtarld force-pushed the fix/serializer-constructor-arguments branch from 7be2684 to 1ad0ecc Compare November 14, 2023 07:39
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after minor changes

@mtarld mtarld force-pushed the fix/serializer-constructor-arguments branch from 1ad0ecc to 8f7c7ae Compare November 20, 2023 03:15
@nicolas-grekas
Copy link
Member

Thank you @mtarld.

@nicolas-grekas nicolas-grekas merged commit b558912 into symfony:5.4 Nov 20, 2023
8 of 11 checks passed
@mtarld mtarld deleted the fix/serializer-constructor-arguments branch November 20, 2023 09:30
@nicolas-grekas
Copy link
Member

@mtarld could you please have a look at the current failures on branch 6.3 for Serializer? I need help to resolve them after the merge up 🙏

@mtarld
Copy link
Contributor Author

mtarld commented Nov 21, 2023

Sorry @nicolas-grekas I'm in a different timezone, so I hadn't time to take of it.

But the fix provided by @xabbuh in #52665 is good (it's the one I already had on my local machine)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants