8000 [Serializer] Prevent that bad Ignore method annotations lead to incorrect results by astepin · Pull Request #46947 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Serializer] Prevent that bad Ignore method annotations lead to incorrect results #46947

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 15, 2022
Merged

[Serializer] Prevent that bad Ignore method annotations lead to incorrect results #46947

merged 1 commit into from
Jul 15, 2022

Conversation

astepin
Copy link
Contributor
@astepin astepin commented Jul 14, 2022 •
edited
Loading

fix #45016

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #45016
License MIT
Doc PR na

By attaching the Ignore serializer annotation to a method that is not a set/get/has/is method, the property that was read before the method was marked as ignored.
I have tweaked the behavior here so that it matches the other annotations.

However, the change could now cause exceptions in older projects if they already have this bug built in.
From my point of view, however, this is justifiable, because at the moment data may not be output correctly.

@astepin astepin requested a review from dunglas as a code owner July 14, 2022 20:07
@carsonbot carsonbot added this to the 6.1 milestone Jul 14, 2022
@carsonbot
Copy link

It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you.

Cheers!

Carsonbot

@astepin astepin mentioned this pull request Jul 14, 2022
Closed
@astepin astepin changed the title Prevent that bad Ignore method annotations lead to incorrect results [Serializer] Prevent that bad Ignore method annotations lead to incorrect results Jul 14, 2022
dunglas
dunglas approved these changes Jul 14, 2022
View reviewed changes
Copy link
Member
@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

LGTM
@carsonbot carsonbot changed the title [Serializer] Prevent that bad Ignore method annotations lead to incorrect results Prevent that bad Ignore method annotations lead to incorrect results Jul 14, 2022
@chalasr
Copy link
Member
chalasr commented Jul 14, 2022

Thanks for the PR!
Can you check if 5.4 is also impacted? If yes, this PR should target 5.4.
Also, do other config formats suffer from the same issue? The behavior must be consistent across all formats so if it's the case the corresponding loaders should be patched as well.

@astepin astepin force-pushed the fix_45016 branch 2 times, most recently from d624202 to 59204a3 Compare July 15, 2022 07:15
@astepin astepin changed the base branch from 6.1 to 5.4 July 15, 2022 07:15
@astepin
Copy link
Contributor Author
astepin commented Jul 15, 2022

Hey @chalasr, the bug is in fact already present in 5.4. I have now rebased to 5.4.
Furthermore, I have modified the test so that it checks annotations and attributes, because the error behavior is identical.

@derrabus derrabus modified the milestones: 6.1, 5.4 Jul 15, 2022
@carsonbot carsonbot changed the title Prevent that bad Ignore method annotations lead to incorrect results [Serializer] Prevent that bad Ignore method annotations lead to incorrect results Jul 15, 2022
@chalasr
Copy link
Member
chalasr commented Jul 15, 2022

@astepin Alright, by "other config formats" I was thinking of yaml and xml. After a quick look to these loaders it turns out they have no single validation rule regarding the accessor/mutator, so I'd say we can ignore them and go ahead here.
It seems like we do have an inconsistency on that point though, so feel free to have a look and submit a PR to add such validation if it's relevant and possible.

@chalasr
Copy link
Member
chalasr commented Jul 15, 2022 •
edited
Loading

Thank you @astepin. And congratz on your first contrib!

@chalasr chalasr merged commit 8dd6bb7 into symfony:5.4 Jul 15, 2022
@astepin astepin deleted the fix_45016 branch July 15, 2022 23:49
This was referenced Jul 29, 2022
Merged
Merged
@fabpot fabpot mentioned this pull request Jul 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunglas dunglas dunglas approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Bug Serializer Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
5 participants
@astepin @carsonbot @chalasr @dunglas @derrabus
0