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 unexpected allowed attributes #52917

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Dec 6, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #52673 #49710
License MIT

A more accurate approach than #52680

@mtarld
Copy link
Contributor Author

mtarld commented Dec 6, 2023

fabbot errors aren't related to that PR.

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

As discussed at SymfonyCon, the information (access type) would better transit through a method parameter than a context option since the need is specific to the abstract method at hand

@mtarld mtarld force-pushed the fix/allowed-attributes branch 2 times, most recently from 8f93524 to ee9ccde Compare December 12, 2023 13:27
@mtarld
Copy link
Contributor Author

mtarld commented Dec 12, 2023

Once this PR will be upmerged to 7.1, I'll create a new one to trigger the proper deprecations.

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.

The implementation fits the definition of a new feature to me: adding an argument on a constructor, and adding an argument on public/protected methods.
This should come with an @param annotation for methods, and this will yield a new deprecation.
Maybe another approach is possible? I don't know at all :)

@chalasr
Copy link
Member

chalasr commented Jan 26, 2024

That read/write mode do need to be passed throughout method calls to fix the bug at hand, so the plan here is to make it a parameter that's hidden at first on 5.4, then make it virtual on 7.1 and finally make it real on 8.0.
The only alternative I can think of is to put the info in $context instead.

@mtarld would that work conceptually and technically?

@mtarld mtarld force-pushed the fix/allowed-attributes branch 2 times, most recently from e5df998 to 46f78a9 Compare February 8, 2024 08:18
@mtarld
Copy link
Contributor Author

mtarld commented Feb 8, 2024

It appears that it works indeed, and it is much better thanks!

I'm wondering if we should make that read/write information explicit in 7.1, maybe a context value like here is enough (but it is less secure).

@fabpot
Copy link
Member

fabpot commented Apr 8, 2024

Thank you @mtarld.

@fabpot fabpot merged commit 49e9184 into symfony:5.4 Apr 8, 2024
9 of 12 checks passed
@mtarld mtarld deleted the fix/allowed-attributes branch April 8, 2024 11:20
This was referenced Apr 29, 2024
xabbuh added a commit to xabbuh/symfony that referenced this pull request Apr 30, 2024
The PropertyNormalizer does not handle a property info extractor. It looks
like the argument was accidentally added to instead of the ObjectNormalizer
in symfony#52917.
xabbuh added a commit to xabbuh/symfony that referenced this pull request Apr 30, 2024
The PropertyNormalizer does not handle a property info extractor. It looks
like the argument was accidentally added to instead of the ObjectNormalizer
in symfony#52917.
xabbuh added a commit to xabbuh/symfony that referenced this pull request Apr 30, 2024
The PropertyNormalizer does not handle a property info extractor. It looks
like the argument was accidentally added to instead of the ObjectNormalizer
in symfony#52917.
xabbuh added a commit to xabbuh/symfony that referenced this pull request Apr 30, 2024
The PropertyNormalizer does not handle a property info extractor. It looks
like the argument was accidentally added to instead of the ObjectNormalizer
in symfony#52917.
xabbuh added a commit to xabbuh/symfony that referenced this pull request Apr 30, 2024
The PropertyNormalizer does not handle a property info extractor. It looks
like the argument was accidentally added to instead of the ObjectNormalizer
in symfony#52917.
chalasr added a commit that referenced this pull request May 1, 2024
…or to the ObjectNormalizer (xabbuh)

This PR was merged into the 5.4 branch.

Discussion
----------

[FrameworkBundle] move wiring of the property info extractor to the ObjectNormalizer

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

The PropertyNormalizer does not handle a property info extractor. It looks like the argument was accidentally added to instead of the ObjectNormalizer in #52917.

Commits
-------

518bc28 move wiring of the property info extractor to the ObjectNormalizer
derrabus added a commit that referenced this pull request May 2, 2024
… setters with optional args (HypeMC)

This PR was merged into the 5.4 branch.

Discussion
----------

[Serializer] Fix `GetSetMethodNormalizer` not working with setters with optional args

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #54784
| License       | MIT

Prior to #52917 setters could have an optional argument or even multiple ones. This restores the previous behavior.

Commits
-------

74bc0eb [Serializer] Fix `GetSetMethodNormalizer` not working with setters with optional args
@stloyd
Copy link
Contributor

stloyd commented May 6, 2024

Those changes generate a big performance drop in one of the projects I work with where we extensively use attributes on DTOs.

Comparing Serializer in version 7.0.6 vs 7.0.7:
Zrzut ekranu 2024-05-6 o 16 27 33
Zrzut ekranu 2024-05-6 o 16 27 17

@nicolas-grekas
Copy link
Member

Please open a dedicated issue if you want any follow up.

fabpot added a commit that referenced this pull request Jun 16, 2024
…peMC)

This PR was merged into the 5.4 branch.

Discussion
----------

[Serializer] Fix `ObjectNormalizer` with property path

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #54983
| License       | MIT

Caused by #52917.

The `ObjectNormalizer::isAllowedAttribute()` method doesn't work with property paths, this is an attempt to fix the problem.

Commits
-------

3857545 [Serializer] Fix `ObjectNormalizer` with property path
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

8 participants