Skip to content

[PropertyInfo][Serializer] Fix default groups #58656

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

Closed
wants to merge 1 commit into from

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Oct 24, 2024

Q A
Branch? 7.1
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #58576, Fix #57350
License MIT

When introduced in #51514, the behavior of group selection was wrong and introduced BC breaks (see the two related issues).

This PR resolves them by making that behavior opt-in, hidden behind an enable_default_groups context value.

Plus, this PR handles (when opted-in) default groups as they should have been handled in the first place.
Let's say we have the following class:

class Foo
{
    public bool $noGroup = true;

    #[Groups('custom')]
    public bool $customGroup = true;

    #[Groups('Default')]
    public bool $defaultGroup = true;

    #[Groups('ExampleObject57350')]
    public bool $classGroup = true;
}

The SerializerExtractor will return properties:

group properties
null, *, [] noGroup, customGroup, defaultGroup, classGroup
custom customGroup
Default, Foo, [] noGroup, defaultGroup, classGroup

The AbstractNormalizer will allow attributes:

group properties
null, [], * noGroup, customGroup, defaultGroup, classGroup
custom customGroup
Default, Foo noGroup, defaultGroup, classGroup

The MetadataAwareNameConverter will allow attributes:

group properties
null, [] noGroup, defaultGroup, classGroup
* noGroup, customGroup, defaultGroup, classGroup
custom customGroup
Default, Foo defaultGroup, classGroup

On the contrary to SerializerExtractor and AbstractNormalizer, noGroup is not included when any group is specified, and customGroup is not included when no group is specified)


Another PR should be created in 7.3 in order to deprecate not setting the enable_default_groups context value, as it will default to true in 8.0.

@mtarld mtarld requested a review from dunglas as a code owner October 24, 2024 16:23
@carsonbot carsonbot added this to the 7.1 milestone Oct 24, 2024
@carsonbot carsonbot changed the title [Serializer][PropertyInfo] Fix default groups [PropertyInfo][Serializer] Fix default groups Oct 24, 2024
@mtarld mtarld force-pushed the fix/default-groups branch from b65a425 to 62f0ced Compare October 25, 2024 07:30
@chalasr
Copy link
Member

chalasr commented Oct 25, 2024

Given the attached patch enlarges the public API to fix a BC break introduced in 7.1 6 months ago, I wonder if we shouldn't just revert that feature entirely on 7.1 to reintroduce it on 7.3 the way we want it to be.

@mtarld
Copy link
Contributor Author

mtarld commented Oct 25, 2024

Agreed, I'd rather just revert the feature. It'll be cleaner and easier to introduce in 7.3.

@mtarld mtarld closed this Oct 28, 2024
fabpot added a commit that referenced this pull request Nov 9, 2024
This PR was merged into the 7.1 branch.

Discussion
----------

[Serializer] Revert Default groups

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #58576, Fix #57350
| License       | MIT

When introduced in #51514, the behavior of group selection was wrong and introduced BC breaks (see the two related issues).

This PR reverts this introduction so that they can be added properly in 7.3 (see #58656).

Commits
-------

ab3220f [Serializer] Revert default groups
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.

3 participants