Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $name =$attributeMetadata->getName(); | ||
| if ( | ||
| !$attributeMetadata->getIgnore() && |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading.Please reload this page.
GuilhemN left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍, I only have one concern: I thinkhttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/PropertyInfo/Extractor/SerializerExtractor.php should be updated as well.
OskarStark left a comment
There was a problem hiding this 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 commentedOct 6, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
What about providing |
fbourigault commentedOct 6, 2018
The plans to improve the Serializer are in#19374 (comment). |
fabpot commentedMar 20, 2019
@dunglas Do you plan to finish this PR before 4.3 feature freeze? |
dunglas commentedMar 20, 2019
Yes I'll try to finish it tomorrow |
joelwurtz commentedMar 26, 2019
Can this annotation be a So other components / library can profit from this metadata as well ? |
fbourigault commentedMar 27, 2019
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 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 the @joelwurtz,@dunglas let's talk about this at Symfony Live :) |
joelwurtz commentedMar 27, 2019
👍 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 the 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 the but let's discuss this at symfony live like you said, |
dmaicher left a comment
There was a problem hiding this 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 😊
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
joelwurtz commentedApr 6, 2019
raress96 commentedJan 3, 2020
Any updates regarding this? |
90b2874 to383ea22Comparedunglas commentedJan 15, 2020
Tests added and comments resolved. This PR should be good to be merged now. |
nicolas-grekas left a comment
There was a problem hiding this 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 commentedFeb 2, 2020
(and fixed deps=low) |
Uh oh!
There was an error while loading.Please reload this page.
weaverryan left a comment
There was a problem hiding this 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 commentedMar 31, 2020
This one is ready to be merged. |
src/Symfony/Component/PropertyInfo/Extractor/SerializerExtractor.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/PropertyInfo/Extractor/SerializerExtractor.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
chalasr left a comment
There was a problem hiding this 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
Uh oh!
There was an error while loading.Please reload this page.
fabpot commentedApr 24, 2020
Thank you@dunglas. |
vasilvestre commentedMay 23, 2020
Doc issue theresymfony/symfony-docs#13585 I am actually tring to add the documentation |
Uh oh!
There was an error while loading.Please reload this page.
Add an
@Ignoreannotation to configureignored attributes in a convenient way, as well as the related XML and YAML loaders.TODO: