Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[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
@@ -245,6 +245,7 @@ protected function getAllowedAttributes($classOrObject, array $context, $attribu | |||
$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.
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.
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 |
The plans to improve the Serializer are in#19374 (comment). |
@dunglas Do you plan to finish this PR before 4.3 feature freeze? |
Yes I'll try to finish it tomorrow |
Can this annotation be a So other components / library can profit from this metadata as well ? |
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 :) |
👍 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, |
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.
raress96 commentedJan 3, 2020
Any updates regarding this? |
90b2874
to383ea22
CompareTests added and comments resolved. This PR should be good to be merged now. |
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
(and fixed deps=low) |
Uh oh!
There was an error while loading.Please reload this page.
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.
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.
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.
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
@Ignore
annotation to configureignored attributes in a convenient way, as well as the related XML and YAML loaders.TODO: