Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
alamirault wants to merge1 commit intosymfony:7.4
base:7.4
Choose a base branch
Loading
fromalamirault:feature-30483-serialized-names-group

Conversation

alamirault
Copy link
Contributor

@alamiraultalamirault commentedMay 21, 2022•
edited by OskarStark
Loading

QA
Branch?7.2
Bug fix?no
New feature?yes
Deprecations?yes
TicketsFix#30483
LicenseMIT
Doc PRsymfony/symfony-docs#...

This PR allow to configure serialization groups for annotationSerializedName. 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 aor b#[SerializedName('nameThree', ['c'])]    <-- Applied if group is cpublic$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

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

Jibbarth, jvasseur, and darius-v reacted with thumbs up emojimaximmandrik, josep11, and konrad-czernecki-esky reacted with hooray emojiwRLSS and hafkenscheid reacted with eyes emoji
@carsonbot
Copy link

Hey!

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

Cheers!

Carsonbot

@alamirault
Copy link
ContributorAuthor

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

Copy link
Contributor

@mtarldmtarld 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!

Copy link
Contributor

@mtarldmtarld 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 and maximmandrik reacted with heart emoji
@alamiraultalamiraultforce-pushed thefeature-30483-serialized-names-group branch fromd01bf55 to83941cdCompareJuly 26, 2022 18:43
@alamirault
Copy link
ContributorAuthor

(Rebased on current 6.2)

Copy link
Member

@nicolas-grekasnicolas-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 reacted with thumbs up emoji
@alamiraultalamiraultforce-pushed thefeature-30483-serialized-names-group branch 2 times, most recently fromafb3e7a to035b0ddCompareJuly 29, 2022 20:31
@alamirault
Copy link
ContributorAuthor

alamirault commentedJul 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
publicfunctiontestInterface()    {$attributeMetadata =newAttributeMetadata('name');$this->assertInstanceOf(AttributeMetadataInterface::class,$attributeMetadata);    }

@chalasr
Copy link
Member

chalasr commentedJul 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
ContributorAuthor

Indeed, It's enough, thank you@chalasr

@alamirault
Copy link
ContributorAuthor

Friendly ping for re-review@nicolas-grekas

Copy link
Member

@nicolas-grekasnicolas-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.

*/
public $serializedName;
public $serializedName = [];

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
ContributorAuthor

@alamiraultalamiraultOct 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 ?

*
* @param string[] $groups
*/
public function getSerializedName(/* array $groups = [] */): ?string

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
ContributorAuthor

Choose a reason for hiding this comment

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

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

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

Should be done inMetadataAwareNameConverter ?

@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

darius-v reacted with thumbs up emoji

@alamiraultalamiraultforce-pushed thefeature-30483-serialized-names-group branch 2 times, most recently from922aabe to03d905eCompareOctober 26, 2022 20:10
@nicolas-grekasnicolas-grekas modified the milestones:6.2,6.3Nov 5, 2022
@qdequippe
Copy link
Contributor

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

mwansinck reacted with eyes emoji

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

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

@sigbits
Copy link

Will this one still be released?

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

Any plans to make it released?

konrad-czernecki-esky, AmineHouari98, nsknewbie, bxschmidt, DennisdeBest, and darius-v reacted with thumbs up emojimwansinck reacted with eyes emoji

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

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

Would it be hard to add toSerializedPath 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.

sophieham reacted with thumbs up emoji

@derrabus
Copy link
Member

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

@daniser
Copy link

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

@fabpotfabpot modified the milestones:7.2,7.3Nov 20, 2024
@fabpotfabpot modified the milestones:7.3,7.4May 26, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@mtarldmtarldmtarld approved these changes

@dunglasdunglasAwaiting requested review from dunglasdunglas is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.

[Serializer] SerializedName based on groups
16 participants
@alamirault@carsonbot@chalasr@zairigimad@qdequippe@Renrhaf@sigbits@andriusvo@nesl247@mikevrind@derrabus@daniser@nicolas-grekas@mtarld@fabpot@xabbuh
[8]ページ先頭
©2009-2025 Movatter.jp