Skip to content

[Serializer] Allow to add groups to SerializedName annotation/attribute #46432

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
Open
wants to merge 1 commit into
base: 7.4
Choose a base branch
from
Open

[Serializer] Allow to add groups to SerializedName annotation/attribute #46432

wants to merge 1 commit into from
+362 −54

Conversation

alamirault
Copy link
Contributor

@alamirault alamirault commented May 21, 2022 •
edited by OskarStark
Loading

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? yes
Tickets Fix #30483
License MIT
Doc PR symfony/symfony-docs#...

This PR allow to configure serialization groups for annotation SerializedName. Inspired by PR #37903.

Before we can only define #[SerializedName('nameOne')]. Now, name can be different depending of group

Attributes

#[SerializedName('nameOne')]             <-- Applied when no groups are provided or when group no match
#[SerializedName('nameTwo', ['a', 'b'])] <-- Applied if group is a or b
#[SerializedName('nameThree', ['c'])]    <-- Applied if group is c
public $foo;

Annotations

/**
 * @SerializedName("nameOne")
 * @SerializedName("nameTwo", groups={"a", "b"})
 * @SerializedName("nameThree", groups="c")
 */
public $foo;

YAML

attributes:
  foo:
    serialized_names:
      nameOne: ['a', 'b']
      nameTwo: 'c'

XML

<attribute name="foo">
    <serialized_name name="nameOne">
        <group>a</group>
        <group>b</group>
    </serialized_name>
    <serialized_name name="nameTwo">
        <group>c</group>
    </serialized_name>
</attribute>

@alamirault alamirault requested a review from dunglas as a code owner May 21, 2022 18:55
@carsonbot carsonbot added this to the 6.2 milestone May 21, 2022
alamirault
alamirault commented May 21, 2022
View reviewed changes
@carsonbot
Copy link

Hey!

I think @mtarld has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

mtarld
mtarld requested changes May 23, 2022
View reviewed changes
@alamirault
Copy link
Contributor Author

@mtarld I can't "Re request review" but changes are done

mtarld
mtarld requested changes May 24, 2022
View reviewed changes
Copy link
Contributor

@mtarld mtarld left a comment

Choose a reason for hiding this comment

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

Almost good!

mtarld
mtarld requested changes May 26, 2022
View reviewed changes
mtarld
mtarld approved these changes May 26, 2022
View reviewed changes
Copy link
Contributor

@mtarld mtarld left a comment

Choose a reason for hiding this comment

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

LGTM then! 🙂

@alamirault alamirault force-pushed the feature-30483-serialized-names-group branch from d01bf55 to 83941cd Compare July 26, 2022 18:43
@alamirault
Copy link
Contributor Author

(Rebased on current 6.2)

nicolas-grekas
nicolas-grekas reviewed Jul 29, 2022
View reviewed changes
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.

Thanks for the PR. Here are some comments to help move forward.

@alamirault alamirault force-pushed the feature-30483-serialized-names-group branch 2 times, most recently from afb3e7a to 035b0dd Compare July 29, 2022 20:31
@alamirault
Copy link
Contributor Author

alamirault commented Jul 29, 2022 •
edited
Loading

@nicolas-grekas I added layer and trigger deprecation (like urlMatcher)

However, deprecation seems trigger even if methods are not called

  1x: The "Symfony\Component\Serializer\Mapping\AttributeMetadata::setSerializedName()" method will require a new "string[] $groups" argument in the next major version of its interface "Symfony\Component\Serializer\Mapping\AttributeMetadataInterface", not defining it is deprecated.
    1x in AttributeMetadataTest::testInterface from Symfony\Component\Serializer\Tests\Mapping

    public function testInterface()
    {
        $attributeMetadata = new AttributeMetadata('name');
        $this->assertInstanceOf(AttributeMetadataInterface::class, $attributeMetadata);
    }

@chalasr
Copy link
Member

chalasr commented Jul 30, 2022 •
edited
Loading

Can you try documenting the param explicitly the same way as the interface? It may be enough to avoid the notice.

@alamirault
Copy link
Contributor Author

Indeed, It's enough, thank you @chalasr

nicolas-grekas
nicolas-grekas reviewed Aug 1, 2022
View reviewed changes
@alamirault
Copy link
Contributor Author

Friendly ping for re-review @nicolas-grekas

nicolas-grekas
nicolas-grekas reviewed Aug 31, 2022
View reviewed changes
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.

Here are some more BC notes. Please rebase also.

src/Symfony/Component/Serializer/Mapping/AttributeMetadata.php
*/
public $serializedName;
public $serializedName = [];
Copy link
Member

Choose a reason for hiding this comment

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

For maximum BC, I think we should keep this as a string when possible, and use arrays only when actually required.

Copy link
Contributor Author

@alamirault alamirault Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@nicolas-grekas , Tu be sure, somthing like this ?

/**
* string|array<string, string|null>
*/
public $serializedName;

And deal with string and array in every methods, is it right ?

src/Symfony/Component/Serializer/Mapping/AttributeMetadata.php
*
* @param string[] $groups
*/
public function getSerializedName(/* array $groups = [] */): ?string
Copy link
Member

Choose a reason for hiding this comment

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

Why pass an array here? Shouldn't we accept string|null instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used by MetadataAwareNameConverter
https://github.com/symfony/symfony/pull/46432/files#diff-87ffe277c0213fca753a8e0b26cc8f4436e255b9776e5e08852c403df247c5b9R121

Serializer group context can be multiple. So returned SerializerName is the first matching group list.

Should be done in MetadataAwareNameConverter ?

@nicolas-grekas nicolas-grekas mentioned this pull request Aug 31, 2022
Open
@zairigimad
Copy link
Contributor

Hello @alamirault, this is a great feature I was waiting for it for so long, do you you need help on this PR to resolve the latest feedbacks ? thanks

@alamirault alamirault force-pushed the feature-30483-serialized-names-group branch 2 times, most recently from 922aabe to 03d905e Compare October 26, 2022 20:10
@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@qdequippe
Copy link
Contributor

Is this PR will be included in 6.2.X? or in 6.3?

@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@Renrhaf
Copy link

Renrhaf commented Aug 4, 2023

thank you for the contribution !
would love to see this shipped in the next version :)

@sigbits
Copy link

sigbits commented Oct 23, 2023

Will this one still be released?

@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@andriusvo
Copy link

Any plans to make it released?

@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
@nesl247
Copy link

nesl247 commented Jul 16, 2024

We just ran into this need. We need to deserialize from xml we don't control, and serialize to json. Doing this any other way would be extremely complex as we're already dealing with a few hundred objects. Duplicating all of that to control it differently is not an option.

@nesl247
Copy link

nesl247 commented Aug 6, 2024

Would it be hard to add to SerializedPath as well?

@mikevrind
Copy link

I really appreciate the works that people have put into this feature. Would be valuable to see this being merged in the near future because this gives us more control (and less unwanted/unexpected behaviour) within de Serializer.

@derrabus
Copy link
Member

Somebody would need to pick up the work and rebase the changes onto 7.2.

@daniser
Copy link

daniser commented Sep 20, 2024

Sorry guys, I completely missed this PR and discussion and created my own PR (#58236) 🤦‍♂️.
On the good side, I've also implemented logic for SerializedPath grouping.

@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@fabpot fabpot modified the milestones: 7.3, 7.4 May 26, 2025
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

@mtarld mtarld mtarld approved these changes

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

Assignees
No one assigned
Labels
Deprecation Feature Serializer Status: Needs Review
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.

[Serializer] SerializedName based on groups