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

SerializedName based on Groups#37903

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

Conversation

jsamouh
Copy link
Contributor

@jsamouhjsamouh commentedAug 21, 2020•
edited
Loading

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

New Feature: Serialized Name based on Groups

/** * @SerializedName("bar")                       <-- Applied only when no groups are provided * @SerializedName("baz", groups={"a", "b"})    <-- Applied if group is a or b * @SerializedName("bat", groups="c")           <-- Applied if group is c */$foo// YAML  attributes:    foo:      serialized_names:         bar: ~         baz: ['a', 'b']         bat: ['c']//XML        <attribute name="foo" serialized-name="bar" />        <attribute name="foo" serialized-name="baz">             <group>a</group>             <group>b</group>       </attribute>        <attribute name="foo" serialized-name="bat">             <group>c</group>       </attribute>

deguif, OskarStark, ro0NL, lbassin, gunjeetkaur, and janvernieuwe reacted with thumbs up emoji
@jsamouhjsamouh changed the titleFirst try Serialized based on groups on AnnotationFirst try SerializedName based on groups on AnnotationAug 21, 2020
@jsamouhjsamouh marked this pull request as draftAugust 21, 2020 01:19
@jsamouhjsamouh changed the titleFirst try SerializedName based on groups on Annotation[Draft] SerializedName based on GroupsAug 21, 2020
@fabpotfabpot added this to thenext milestoneAug 23, 2020
@dunglas
Copy link
Member

dunglas commentedAug 24, 2020•
edited
Loading

What do you think about using the following signature:

/** * @SerializedName("bar")                       <-- Applied only when no groups are provided * @SerializedName("baz", groups={"a", "b"})    <-- Applied if group is a or b * @SerializedName("bat", groups="c")           <-- Applied if group is c */$foo;

If find this easier to read and write.

yceruto, jvasseur, OskarStark, and jsamouh reacted with thumbs up emoji

@jvasseur
Copy link
Contributor

The doctrine annotation reader is capable of handling the same annotation multiple time on a property so this shouldn't be a problem.

jsamouh reacted with thumbs up emoji

@jsamouhjsamouhforce-pushed theissue-30483-serialized-name-based-on-groups branch from1d61e0f to9620fe8CompareAugust 26, 2020 02:35
@jsamouh
Copy link
ContributorAuthor

cc@dunglas or other people for review

@jsamouhjsamouh changed the title[Draft] SerializedName based on GroupsSerializedName based on GroupsAug 26, 2020
@jsamouhjsamouh marked this pull request as ready for reviewAugust 26, 2020 16:09
@fabpotfabpot requested a review fromdunglasAugust 26, 2020 16:42
@fabpot
Copy link
Member

@jsamouh Can you squash your commits?

jsamouh reacted with thumbs up emoji

@jsamouhjsamouhforce-pushed theissue-30483-serialized-name-based-on-groups branch 2 times, most recently from56b6f1c tod77f3d0CompareSeptember 2, 2020 14:24
@jsamouhjsamouhforce-pushed theissue-30483-serialized-name-based-on-groups branch fromd77f3d0 to14beaecCompareSeptember 2, 2020 14:33
@@ -111,15 +109,51 @@ public function getMaxDepth()
*/
public function setSerializedName(string $serializedName = null)
{
$this->serializedName = $serializedName;
$this->addSerializedName($serializedName);
Copy link
Contributor

Choose a reason for hiding this comment

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

you cannot pass null here.

* Adds the serialization name for this attribute.
*/
public function addSerializedName(string $serializedName, array $groups = []);

/**
* Gets the serialization name for this attribute.
*/
public function getSerializedName(): ?string;
Copy link
Contributor

Choose a reason for hiding this comment

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

id remove/deprecate this one


foreach ($this->serializedNames as $serializedName => $groupsForSerializedName) {
if (!$groupsForSerializedName) {
$defaultSerializedName = $serializedName;
Copy link
Contributor

Choose a reason for hiding this comment

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

i dont think you can make this assumption :/ any name without groups could be considered default

@ro0NL
Copy link
Contributor

i think we should also validate that there can only be 1 default name (no groups)

as well as validating no overlap, e.g. 2 names with the same groups.

@jsamouh
Copy link
ContributorAuthor

i think we should also validate that there can only be 1 default name (no groups)

as well as validating no overlap, e.g. 2 names with the same groups.

I understand your point... I m shared between let the behavior like this and be strict on it. Need a second thought on that cc@dunglas@fabpot

Thanks for your feedback

@fabpot
Copy link
Member

I tend to agree with@ro0NL

serialized_name: 'full_name'
serialized_names: 'full_name'
Copy link
Contributor

Choose a reason for hiding this comment

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

seems to break user configs, thus BC

IMHO we can keep supporting 2 formats:

ser_name:'some'ser_names:some:~other:[grp1, grp2]

}
$attributeMetadata->addSerializedName($data['serialized_names']);
} elseif (\is_array($data['serialized_names'])) {
if (empty($data['serialized_names'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (!$data['serialized_names']) {

throw new MappingException(sprintf('The "serialized_names" value must be a non-empty array of serialized name/groups in "%s" for the attribute "%s" of the class "%s".', $this->file, $attribute, $classMetadata->getName()));
}
foreach ($data['serialized_names'] as $serializedName => $groups) {
if (!\is_string($serializedName) || empty($serializedName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

|| '' === trim($serializedName)

/**
* Adds the serialization name for this attribute.
*/
public function addSerializedName(string $serializedName, array $groups = []);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO should this one should besetSerializedNames (no need for an adder) - which simplifies resetting

setSerializedName(null) previous, vs setSerializedNames([]) now

@jsamouh
Copy link
ContributorAuthor

Thanks for your comments@ro0NL ,

To prevent back and forth, and because some of your comments are the opposite from previous comments suggested, I m waiting@dunglas to review...

@fabpotfabpot closed thisOct 7, 2020
@fabpot
Copy link
Member

We've just moved away frommaster as the main branch to use5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

@hafkenscheid
Copy link

Is there already a new PR for this?

@derrabus
Copy link
Member

I don't think so.

@jsamouh
Copy link
ContributorAuthor

will dot it next week

@janvernieuwe
Copy link
Contributor

This would still be immensely useful for refactoring/versioning things.

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

@fabpotfabpotfabpot approved these changes

@stloydstloydstloyd left review comments

@ro0NLro0NLro0NL left review comments

@dunglasdunglasAwaiting requested review from dunglas

@ycerutoycerutoAwaiting requested review from yceruto

Assignees
No one assigned
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

[Serializer] SerializedName based on groups
13 participants
@jsamouh@dunglas@jvasseur@fabpot@ro0NL@hafkenscheid@derrabus@janvernieuwe@stloyd@yceruto@nicolas-grekas@ogizanagi@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp