Skip to content

[Serializer] Marking some Normalizer classes as final #49950

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
Apr 9, 2023
Merged

[Serializer] Marking some Normalizer classes as final #49950

merged 1 commit into from
Apr 9, 2023

Conversation

tucksaun
Copy link
Contributor

@tucksaun tucksaun commented Apr 5, 2023 •
edited
Loading

Q A
Branch? 6.3
Bug fix? no
New feature? no
Deprecations? yes
Tickets #49291 (review)
License MIT
Doc PR n/a

As mentioned in #49291 (review).

Making those classes final will allow to get rid of all the __CLASS__ === static::class for cacheability checks in 7.0

@tucksaun tucksaun requested a review from dunglas as a code owner April 5, 2023 17:01
@carsonbot carsonbot added this to the 6.3 milestone Apr 5, 2023
@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "6.3" but it seems your PR description refers to branch "6.3 for features".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

tucksaun
tucksaun commented Apr 5, 2023
View reviewed changes
nicolas-grekas
nicolas-grekas reviewed Apr 5, 2023
View reviewed changes
@tucksaun tucksaun force-pushed the serializer/final-normalizer branch from 6e733e8 to 2b5f936 Compare April 5, 2023 18:53
@tucksaun tucksaun mentioned this pull request Apr 5, 2023
Merged
@tucksaun tucksaun closed this Apr 5, 2023
@tucksaun tucksaun force-pushed the serializer/final-normalizer branch from 2b5f936 to ba0fdc0 Compare April 5, 2023 21:50
@tucksaun tucksaun reopened this Apr 5, 2023
fabpot added a commit that referenced this pull request Apr 8, 2023
@fabpot
feature #49953 [Serializer] Mark ObjectNormalizer as final for 7.0 …
fd9151d 
…(tucksaun)

This PR was merged into the 6.3 branch.

Discussion
----------

[Serializer] Mark `ObjectNormalizer` as final for 7.0

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | #49291 (review)
| License       | MIT
| Doc PR        | none

Extracted from #49950 and related to #49291 (review)

Commits
-------

b2828e9 [Serializer] Mark ObjectNormalizer as final for 7.0
@tucksaun
[Serializer] Marking some Normalizer classes as final
d976fc5 
| Q             | A
| ------------- | ---
| Branch?       | 6.3 for features
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | symfony#49291 (review)
| License       | MIT
| Doc PR        | n/a

As mentioned in symfony#49291 (review).
@tucksaun tucksaun force-pushed the serializer/final-normalizer branch from 12851dd to d976fc5 Compare April 8, 2023 23:55
@fabpot
Copy link
Member

fabpot commented Apr 9, 2023

Thank you @tucksaun.

@fabpot fabpot mentioned this pull request Apr 9, 2023
Closed
@fabpot fabpot merged commit 1f4a1bc into symfony:6.3 Apr 9, 2023
@tucksaun tucksaun deleted the serializer/final-normalizer branch April 9, 2023 19:15
@larowlan
Copy link
Contributor

larowlan commented Sep 5, 2023 •
edited
Loading

I've got a question about this one, in the docs we say

The custom name converter can be used by passing it as second parameter of any class extending AbstractNormalizer, including GetSetMethodNormalizer and PropertyNormalizer:

But now we've marked GetSetMethodNormalizer as final, I assume this will become final class in symfony 7.

So what is the new recommended method on that front? And should we update the docs?

@tucksaun
Copy link
Contributor Author

tucksaun commented Sep 5, 2023

I believe the "including" wording from the sentence is not here to meant "any classes extending GetSetMethodNormalizer or PropertyNormalizer" but more to mention that this statement is true for any classes extending AbstractNormalizer and that GetSetMethodNormalizer and PropertyNormalizer happen to extends AbstractNormalizer and so you can pass the name converter to those specific classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

Assignees
No one assigned
Labels
Deprecation Serializer Status: Reviewed
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.
6 participants
@tucksaun @carsonbot @fabpot @larowlan @nicolas-grekas @stof