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] Add an @Ignore annotation#28744

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

Merged
fabpot merged 1 commit intosymfony:masterfromdunglas:ignore-annot
Apr 24, 2020

Conversation

dunglas
Copy link
Member

@dunglasdunglas commentedOct 5, 2018
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#24071
LicenseMIT
Doc PRn/a

Add an@Ignore annotation to configureignored attributes in a convenient way, as well as the related XML and YAML loaders.

TODO:

  • Add tests

samnela, gmponos, Kocal, versh23, thomasbisignani, ktherage, maxhelias, agil-NUBBA, roincent, Zeryther, and 9 more reacted with thumbs up emojilocalheinz reacted with hooray emojiOskarStark and localheinz reacted with heart emojilocalheinz and OskarStark reacted with rocket emojilocalheinz reacted with eyes emoji
@@ -245,6 +245,7 @@ protected function getAllowedAttributes($classOrObject, array $context, $attribu
$name = $attributeMetadata->getName();

if (
!$attributeMetadata->getIgnore() &&
Copy link
Contributor

@ro0NLro0NLOct 6, 2018
edited
Loading

Choose a reason for hiding this comment

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

givenhttps://github.com/symfony/symfony/pull/28744/files#diff-7a86113d1484512f310d58e30eadf67dR56 is there any reason to not use$attributeMetadata->ignore here? That would actually implement the advertised perf benefit no?

edit: i see it's about serialization.. but still?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

If we change this, I propose to change this consistently for all attributes in another PR.

Copy link
Contributor

@GuilhemNGuilhemN left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@OskarStarkOskarStark left a comment

Choose a reason for hiding this comment

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

I love your work on improving this component 💪

@Koc
Copy link
Contributor

Koc commentedOct 6, 2018
edited
Loading

What about providing@ExclusionPolicy in JMS-serializer manner? There is opened PR#19374 for that.

@fbourigault
Copy link
Contributor

The plans to improve the Serializer are in#19374 (comment).

@fabpot
Copy link
Member

@dunglas Do you plan to finish this PR before 4.3 feature freeze?

@dunglas
Copy link
MemberAuthor

Yes I'll try to finish it tomorrow

@joelwurtz
Copy link
Contributor

Can this annotation be aPropertyInfo component feature instead ? Which remove the property from the getProperties method if needed ?

So other components / library can profit from this metadata as well ?

@fbourigault
Copy link
Contributor

IMHO, the@ignore annotation should be owned by the Serializer component because we may want to exclude the property for the Serializer but to include it for anything else.

And about properties extraction, after trying to finish#28775, I figured that the interface about extracting properties to (un-)serialize must belong to the Serializer component and we could also ship a default implementation built on top of the PropertyInfo component.

Then, we could use decorators, to implement@Ignore,@ExclusionPolicy,@Expose,@Since,@Until, etc... We also be able to move the@Groups into an implementation of this interface.

This would move the logic of getting which properties must be (un-)serialize out of Normalizer and improve a lot the S.O.L.I.D.ity of the Serializer component and help to deprecate theGetSetMethodNormalizer, thePropertyNormalizer, theAbstractObjectNormalizer and theAbstractNormalizer.

@joelwurtz,@dunglas let's talk about this at Symfony Live :)

@joelwurtz
Copy link
Contributor

👍 for talking about this at SymfonyLive, as for the annotation, i don't see a problem on having it on Serializer only by using a dedicated PropertyInfo extractor chain ?

A lot of the annotations that you propose can be also useful in the context of theAutomapper if this component is wanted, i would prefer having those in the symfony property info component as decorator, so other components can also profit of this.

Also if this gets merged#30704 and there is a direct link between PropertyAccess and ProperyInfo component in the future, those annotation + decorators could be used in many place where we want to restrict property. Like in theForm /Validator /Workflow compoenent, this would allow to restrict some properties to be used when using a property path, and avoid exposing some data.

but let's discuss this at symfony live like you said,

Copy link
Contributor

@dmaicherdmaicher left a comment

Choose a reason for hiding this comment

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

I could try to finalize this one within the EUFOSSA hackathon if I get some feedback 😊

dunglas reacted with heart emoji
@joelwurtz
Copy link
Contributor

Just talk with@dmaicher and i think it would be wise to do/finish#30818 (or at least the basics) first before doing this. WDYT@dunglas ?

fbourigault and dmaicher reacted with thumbs up emoji

@raress96
Copy link

Any updates regarding this?

@dunglasdunglasforce-pushed theignore-annot branch 2 times, most recently from90b2874 to383ea22CompareJanuary 15, 2020 17:57
@dunglas
Copy link
MemberAuthor

Tests added and comments resolved. This PR should be good to be merged now.
As the refactoring work on the Serializer is stuck for months (#30818), I propose to move forward regarding this PR, we'll be able to refactor this later anyway if needed.

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.

LGTM, just one wording question

@nicolas-grekas
Copy link
Member

(and fixed deps=low)

Copy link
Member

@weaverryanweaverryan left a comment

Choose a reason for hiding this comment

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

This generally makes sense to me.

@dunglas
Copy link
MemberAuthor

This one is ready to be merged.

Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

Misses a CHANGELOG entry

@fabpot
Copy link
Member

Thank you@dunglas.

@fabpotfabpot merged commit734a006 intosymfony:masterApr 24, 2020
@nicolas-grekasnicolas-grekas modified the milestones:next,5.1May 4, 2020
@fabpotfabpot mentioned this pull requestMay 5, 2020
@vasilvestre
Copy link

Doc issue theresymfony/symfony-docs#13585 I am actually tring to add the documentation

dunglas and faizanakram99 reacted with heart emoji

javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull requestSep 3, 2020
…lentin, vasilvestre)This PR was merged into the master branch.Discussion----------[Serializer] Add an@ignore annotation #28744Completesymfony/symfony#28744Commits-------dcd5b4c Update serializer.rst56be700 Update serializer.rst6a3b355 [Serializer] Add an@ignore annotation #28744
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@weaverryanweaverryanweaverryan left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@dmaicherdmaicherdmaicher left review comments

@ro0NLro0NLro0NL left review comments

@norkunasnorkunasnorkunas left review comments

@fabpotfabpotfabpot approved these changes

@OskarStarkOskarStarkOskarStark approved these changes

@GuilhemNGuilhemNGuilhemN approved these changes

@chalasrchalasrchalasr approved these changes

@maxheliasmaxheliasmaxhelias approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

17 participants
@dunglas@Koc@fbourigault@fabpot@joelwurtz@raress96@nicolas-grekas@vasilvestre@weaverryan@dmaicher@OskarStark@ro0NL@norkunas@GuilhemN@chalasr@maxhelias@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp