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] ObjectNormalizer#13257
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
dunglas commentedJan 6, 2015
If you want to play with this new normalizer (or hack it), I've created a branch including all Serializer PR waiting to be merged (including this one):https://github.com/dunglas/symfony/tree/new_serializer To use it in a standard edition install, add the following lines to your "repositories": [ {"type":"vcs","url":"https://github.com/dunglas/symfony" } ], Then replace with Symfony line to use this experimental branch: "symfony/symfony":"dev-new_serializer as 2.7.0", |
mickaelandrieu commentedJan 6, 2015
hi@dunglas, can you provide a practical sample of what this feature can do ? |
dunglas commentedJan 6, 2015
class MyObj{public$foo ='foo';public getBar() {return 'bar'; }publicisBaz() {returntrue; }}$normalizer =new \Symfony\Component\Serializer\Normalizer\PropertyAccessNormalizer();var_dump($normalizer->normalize(newMyObj()));// Should output something like ['foo' => 'foo', 'bar' => 'bar', 'baz' => true] |
dunglas commentedJan 6, 2015
This normalizer also supports denormalization (for gettters / settersand properties), serialization groups (http://symfony.com/blog/new-in-symfony-2-7-serialization-groups), name converters (symfony/symfony-docs#4692), circular references handlers (symfony/symfony-docs#4299) and existing object population (#13252). |
fabpot commentedJan 7, 2015
👍 The only "issue" I see is the name; I understand that you named your class after the property access component but I find it confusing as at first, I thought it was about "just" supporting class properties. If it becomes the default one (which I think is a good idea), we should find a better name. |
dunglas commentedJan 7, 2015
I agree, the name is confusing. What about |
fabpot commentedJan 7, 2015
@dunglas If this is the "definitive" serializer, that sounds much better to me. |
mickaelandrieu commentedJan 7, 2015
totaly agree with@fabpot , this should become the default serializer! |
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.
Shouldn't this be moved torequire if it becomes the default normalizer?
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 should be the default serializer in the full stack framework, but please don't add more "hard" dependencies between (standalone) components !
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.
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.
@mickaelandrieu good point
dunglas commentedJan 17, 2015
What do you think about moving the code listing accessible "properties" (trough public properties and methods) -https://github.com/dunglas/symfony/blob/seriaizer_property_access_normalizer/src/Symfony/Component/Serializer/Normalizer/ObjectNormalizer.php#L62-L89 - in the PropertyAccess Component? It can be useful in other contexts (in fact, I've an use case in a private project right now). cc@fabpot@webmozart |
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.
you should rely on autoloading instead
b344af7 to2f69022Compare7531332 toe1cf645Comparedunglas commentedFeb 4, 2015
Rebased. Should be ready for the merge 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.
👍 thanks a lot!
dunglas commentedFeb 22, 2015
I've fixed tests with |
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.
Is it on purpose you don't use anelse block on the next statement? It can avoid a call to thethis->propertyAccessor->getValue
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.
@jeremy-derusse I don't understand what you mean.
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.
You can replace
$attributeValue = $this->propertyAccessor->getValue($object, $attribute);if (array_key_exists($attribute, $this->callbacks)) { $attributeValue = call_user_func($this->callbacks[$attribute], $attributeValue);}by
if (array_key_exists($attribute, $this->callbacks)) { $attributeValue = call_user_func($this->callbacks[$attribute], $attributeValue);} else { $attributeValue = $this->propertyAccessor->getValue($object, $attribute);}and avoid to call$this->propertyAccessor->getValue($object, $attribute); whenarray_key_exists($attribute, $this->callbacks) is false.
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.
No because$attributeValue in$attributeValue = call_user_func($this->callbacks[$attribute], $attributeValue); will be undefined.
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.
Or worse, the value of the previous loop iteration.
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.
Ok I got it. But still no because the attribute value will never be retrieved. We need it even if a callback is not registered.
a562e42 toa4b6421Comparefca94f2 to6a12f1eComparedunglas commentedMar 3, 2015
Rebased. |
dunglas commentedMar 3, 2015
ping @symfony/deciders |
6a12f1e to0050bbbCompareThis PR was merged into the 2.7 branch.Discussion----------[Serializer] ObjectNormalizer| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| License | MIT| Doc PR | not yet`PropertyAccessNormalizer` is a new normalizer leveraging the PropertyAccess Component. It is able to handle classes containing both public properties and properties only accessibles trough getters / setters / issers / hassers...As it extends `AbstractNormalizer`, it supports circular reference handling, name converters and existing object population.What do you think about making this new normalizer the default one as it's the most convenient to use and the most consistent with the behavior of other components.#13120,#13252 and#13255 need to be merged to make this PR working.Commits-------0050bbb [Serializer] Introduce ObjectNormalizer
mickaelandrieu commentedMar 6, 2015
👍 thanks for this improvement@dunglas ! |
…nnotations (dunglas)This PR was merged into the 2.7 branch.Discussion----------[Serializer] Supports hassers and setters for groups annotations| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aFor more coherence with the new `ObjectNormalizer` (#13257), this PR allows to add `@Groups` annotations on hasser and setter methods too.Commits-------9c87ecc [Serializer] Supports hassers and setters for groups annotations
PropertyAccessNormalizeris a new normalizer leveraging the PropertyAccess Component. It is able to handle classes containing both public properties and properties only accessibles trough getters / setters / issers / hassers...As it extends
AbstractNormalizer, it supports circular reference handling, name converters and existing object population.What do you think about making this new normalizer the default one as it's the most convenient to use and the most consistent with the behavior of other components.
#13120,#13252 and#13255 need to be merged to make this PR working.