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 default group in serialization context#44035

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

@noemi-salaun
Copy link
Contributor

@noemi-salaunnoemi-salaun commentedNov 13, 2021•
edited
Loading

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

The_default group can be use on the serialization context to explicitly tell the serializer to allow properties without defined@Groups

Exemple:

class User{public$name;/**     * @Groups({"private"})     */public$age;/**     * @Groups({"private", "moderator"})     */public$isBan;}
  • With no context groups:name,age,isBan
  • Withmoderator context group:isBan
  • With* context group:name,age,isBan
  • With_default context group:name
  • With_default andmoderator context groups:name,isBan

I use the_default keyword here but it can be easily changed for something else like I said in this issue#32622 (comment)

I don't know what could be the best option to reduce the BC break.

  • A special char, like for the*. Could be_. (not a fan of this solution)
  • A special word, likeDefault in the JMS Serializer (high chance of BC breaks with existing project)
  • Thenull value
  • The empty string value

null or empty string value cannot be use in@Groups annotation, so we are sure there is no BC break with this solution

odolbeau, jbgomond, ninsuo, anboo, 4rthem, and TerjeBr reacted with thumbs up emoji
@carsonbot
Copy link

Hey!

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

Cheers!

Carsonbot

@noemi-salaun
Copy link
ContributorAuthor

Anyone available to help me with this PR?

@fabpotfabpot modified the milestones:6.1,6.2May 20, 2022
@noemi-salaunnoemi-salaunforce-pushed thefeat32622-serializer-default-group branch fromcfdef73 to4efbc42CompareMay 23, 2022 08:05
@TerjeBr
Copy link

I am waiting on this. What is holding this up?

BafS and chadyred reacted with eyes emoji

@ninsuo
Copy link

Hello,
I'm coming back on this, can we somehow help on what blocks the PR?
Have a nice week,
Alain

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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


privatefunctiongetAttributeGroups(AttributeMetadataInterface$serializerAttributeMetadata):array
{
$groups =empty($serializerAttributeMetadata->getGroups()) ? [AbstractNormalizer::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] :$serializerAttributeMetadata->getGroups();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$groups =empty($serializerAttributeMetadata->getGroups()) ? [AbstractNormalizer::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] :$serializerAttributeMetadata->getGroups();
$groups =!$serializerAttributeMetadata->getGroups() ? [AbstractNormalizer::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] :$serializerAttributeMetadata->getGroups();

Copy link
Member

Choose a reason for hiding this comment

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

CallinggetGroups() two times isn't necessary. Storing the result of this function in a variable will improve the performance.


protectedfunctiongetAttributeGroups(AttributeMetadataInterface$attributeMetadata):array
{
$groups =empty($attributeMetadata->getGroups()) ? [self::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] :$attributeMetadata->getGroups();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$groups =empty($attributeMetadata->getGroups()) ? [self::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] :$attributeMetadata->getGroups();
$groups =!$attributeMetadata->getGroups() ? [self::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] :$attributeMetadata->getGroups();

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even

$groups =$attributeMetadata->getGroups() ?: [self::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS];

publicconstIGNORED_ATTRIBUTES ='ignored_attributes';

/**
* The default group can be use on the serialization context to explicitly
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The default group can beuse on the serialization context to explicitly
* The default group can beused on the serialization context to explicitly

@dunglas
Copy link
Member

Couldn't we usethe same conventions as for the Validator component for consistency?

HeahDude and mtarld reacted with thumbs up emoji

@TerjeBr
Copy link

Couldn't we usethe same conventions as for the Validator component for consistency?

By that you mean use the group name "Default" instead of "_default"? Yes, I am in favour of that. (The only problem is that it is a BC break for code that already uses the group name "Default")

@dunglas
Copy link
Member

Yes, I mean that, and also support using the name of the class as a group name (User in the example provided in the docs).

Regarding the BC break, isn't using_default a BC break too?


privatefunctiongetAttributeGroups(AttributeMetadataInterface$serializerAttributeMetadata):array
{
$groups =empty($serializerAttributeMetadata->getGroups()) ? [AbstractNormalizer::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] :$serializerAttributeMetadata->getGroups();
Copy link
Member

Choose a reason for hiding this comment

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

CallinggetGroups() two times isn't necessary. Storing the result of this function in a variable will improve the performance.

{
$groups =empty($serializerAttributeMetadata->getGroups()) ? [AbstractNormalizer::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] :$serializerAttributeMetadata->getGroups();

returnarray_merge($groups, ['*']);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
returnarray_merge($groups, ['*']);
$groups[] ='*';
return$groups;

This will also probably be a bit faster.

returnis_scalar($groups) ? (array)$groups :$groups;
}

protectedfunctiongetAttributeGroups(AttributeMetadataInterface$attributeMetadata):array
Copy link
Member

Choose a reason for hiding this comment

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

This is on the hot patch. Introducing a new function will have a significant performance impact when serializing a large set of data. Also, we try to not add new protected methods in this class (we're in the process of splitting it and using composition instead of inheritance).

Could you inline it? By the way, you should also inline the one in the extractor.


protectedfunctiongetAttributeGroups(AttributeMetadataInterface$attributeMetadata):array
{
$groups =empty($attributeMetadata->getGroups()) ? [self::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS] :$attributeMetadata->getGroups();
Copy link
Contributor

Choose a reason for hiding this comment

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

Or even

$groups =$attributeMetadata->getGroups() ?: [self::DEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS];

* The default group can be use on the serialization context to explicitly
* tell the serializer to allow properties without defined groups.
*/
publicconstDEFAULT_GROUP_FOR_ATTRIBUTE_WITHOUT_GROUPS ='_default';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to be overridden in the userland?
If not, we could mark it as final I guess, and maybe move it on top of L36 so that it's not considered as a context constant.
If yes, then why not store it in the context (and update theAbstractNormalizerContextBuilder)?

@nicolas-grekasnicolas-grekas modified the milestones:6.2,6.3Nov 5, 2022
@nicolas-grekasnicolas-grekas modified the milestones:6.3,6.4May 23, 2023
@nicolas-grekasnicolas-grekas modified the milestones:6.4,7.1Nov 15, 2023
@nicolas-grekas
Copy link
Member

Closing in favor of#51514, thanks for pushing this forward!

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

Reviewers

@fabpotfabpotfabpot requested changes

@dunglasdunglasdunglas requested changes

@mtarldmtarldmtarld requested changes

+1 more reviewer

@chadyredchadyredchadyred approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

7.1

Development

Successfully merging this pull request may close these issues.

Serializer default groups

10 participants

@noemi-salaun@carsonbot@TerjeBr@ninsuo@dunglas@nicolas-grekas@fabpot@mtarld@chadyred@ogizanagi
[8]ページ先頭
©2009-2025 Movatter.jp