-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Serializer] symfony#36594 attributes cache breaks normalization #43469
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] symfony#36594 attributes cache breaks normalization #43469
Conversation
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
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 looks cleaner to me.
Some tests are broken. Can you have a look? |
@chalasr It turned out that So I fixed it but the check
and I'm quite sure it has nothing to do with my PR specifically.. So, I don't know what is the next step 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.
LGTM, thanks.
Don't worry about the failing appveyor build, it's unrelated.
@dunglas |
4adec04
to
55818c3
Compare
Thank you @ivannemets-sravniru. |
…s-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [Serializer] fix support for lazy/unset properties | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #44273 #44283 | License | MIT | Doc PR | - This basically backports #43469 into 4.4, which is the way to go to fix #44273. The code that exists to handle uninitialized properties is broken anyway (it was before the recent changes.) Commits ------- db043aa [Serializer] fix support for lazy/unset properties
The bug itself is explained in the following (simplified) example:
Though this PR created for 5.4 branch, it actually fixes a bug reproducible in 5.3. The reason why I use 5.4 for this fix is that 5.4 introduces a new feature related to the same problem. If this PR gets approved and merged, I can potentially implement the same fix for 5.3, but
SKIP_UNINITIALIZED_VALUES
will cause some merge conflicts in the future..As of v 5.3 symfony ignores uninitialized properties by default in
ObjectNormalizer::extractAttributes
andPropertyNormalizer::extractAttributes
(implemented in #38900 and #34791) but this approach is wrong -extractAttributes
method MUST return the same attributes list for any instance of the same class, otherwise cached attributes do not match actual attributes list for different instances having different set of initialized/uninitialized propertiesSo, this PR does a few things:
\Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::extractAttributes
so that even uninitialized attributes will be cached by normalizer. Instead, we ignore uninitialized attributes when trying to access them while calling\Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::getAttributeValue
true
as the default value forSKIP_UNINITIALIZED_VALUES
context setting as I believe this is the expected default behavior for any built-in object normalizerSKIP_UNINITIALIZED_VALUES
compatible with not justObjectNormalizer
, but with two other built-in normalizersPropertyNormalizer
andGetSetMethodNormalizer
as well