Skip to content

[Serializer] Add special '*' serialization group that allows any group #33540

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
Aug 25, 2020
Merged

[Serializer] Add special '*' serialization group that allows any group #33540

merged 1 commit into from
Aug 25, 2020

Conversation

nrobinaubertin
Copy link
Contributor

@nrobinaubertin nrobinaubertin commented Sep 10, 2019 •
edited
Loading

Q A
Branch? master for features
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #32622
License MIT
Doc PR symfony/symfony-docs#...

Hi,
I added support for a special serialization group: '*'.
This group lets any group serialize the attribute marked with it.

The BC break comes from the fact that someone could have used the '*' group in their code.

@nrobinaubertin nrobinaubertin mentioned this pull request Sep 10, 2019
Closed
@nicolas-grekas nicolas-grekas added this to the next milestone Sep 12, 2019
@fabpot fabpot requested a review from dunglas February 8, 2020 07:57
fabpot
fabpot approved these changes Aug 16, 2020
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

LGTM.

Can you change the base branch to master and rebase?

@nrobinaubertin nrobinaubertin force-pushed the any-group-serialization branch from 256aac1 to 905c7f3 Compare August 19, 2020 07:50
@nrobinaubertin nrobinaubertin changed the base branch from 4.4 to master August 19, 2020 07:50
@nrobinaubertin
Copy link
Contributor Author

I've implemented the change proposed by @stloyd based on the idea of @dunglas, rebased on master and fixed some tests.
There's still some that fail but I'm not used to symfony internals and I don't immediately see the changes I should make. If someone more knowledgeable can help, I would be thankful.

@fabpot
Copy link
Member

fabpot commented Aug 23, 2020

@nrobinaubertin Can you take @ogizanagi suggestion into account and fix the unit tests? Thank you.

@nrobinaubertin nrobinaubertin force-pushed the any-group-serialization branch 2 times, most recently from b2b6aaa to 370f2c6 Compare August 24, 2020 10:22
@nrobinaubertin
Copy link
Contributor Author

@nrobinaubertin Can you take @ogizanagi suggestion into account and fix the unit tests? Thank you.

Done. I added two tests but the previous ones were already working.
The fails of the CI are not related to this PR

@fabpot fabpot force-pushed the any-group-serialization branch from 3c765c9 to 54e24a8 Compare August 25, 2020 06:52
@fabpot
Copy link
Member

fabpot commented Aug 25, 2020

Thank you @nrobinaubertin.

@fabpot fabpot mentioned this pull request Aug 25, 2020
Closed
@fabpot fabpot merged commit efdc35c into symfony:master Aug 25, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
@ogizanagi ogizanagi mentioned this pull request Dec 9, 2020
Merged
4 tasks
@ToGo101
Copy link

ToGo101 commented Aug 17, 2021

Could it be, that this important note:

Symfony\Component\Serializer\Normalizer/AbstractNormalizer.php:250
// If you update this check, update accordingly the one in Symfony\Component\PropertyInfo\Extractor\SerializerExtractor::getProperties()

has been overseen an the change not made to the extractor?

@SergeyCherenkov SergeyCherenkov mentioned this pull request Nov 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogizanagi ogizanagi ogizanagi left review comments

@fabpot fabpot fabpot approved these changes

@dunglas dunglas dunglas approved these changes

@stloyd stloyd stloyd approved these changes

@jderusse jderusse Awaiting requested review from jderusse

@lyrixx lyrixx Awaiting requested review from lyrixx

@sroze sroze Awaiting requested review from sroze

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
BC Break Feature Serializer Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
8 participants
@nrobinaubertin @fabpot @ToGo101 @dunglas @stloyd @ogizanagi @nicolas-grekas @carsonbot