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] Object class resolver#28669
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
[Serializer] Object class resolver#28669
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| publicfunction__construct(ClassMetadataFactoryInterface$classMetadataFactory =null,NameConverterInterface$nameConverter =null,PropertyTypeExtractorInterface$propertyTypeExtractor =null,ClassDiscriminatorResolverInterface$classDiscriminatorResolver =null) | ||
| /** | ||
| * @var ObjectClassResolverInterface|null |
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.
can't we make it a callable and remove the interface instead?
| /** | ||
| * @var ObjectClassResolverInterface|null | ||
| */ | ||
| protected$objectClassResolver; |
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.
should be private
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.
And the docblock can be removed
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.
Can't it be useful to have it in the child classes?
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.
it's still catchable by overriding the constructor in child classes
but we don't want to promise BC regarding any changes that could happen after instantiation.
| /** | ||
| * @var ObjectClassResolverInterface|null | ||
| */ | ||
| protected$objectClassResolver; |
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.
And the docblock can be removed
| */ | ||
| protected$objectClassResolver; | ||
| publicfunction__construct(ClassMetadataFactoryInterface$classMetadataFactory =null,NameConverterInterface$nameConverter =null,PropertyTypeExtractorInterface$propertyTypeExtractor =null,ClassDiscriminatorResolverInterface$classDiscriminatorResolver =null,ObjectClassResolverInterface$objectClassResolver =null) |
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 you switch to acallable, you can just set'\get_class' as default value, then you can remove the ternary operator later.
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.
Although that might be slower
fc6cb7d toeb5d8d4Comparealanpoulain commentedOct 2, 2018
I have used a callable instead of an interface. |
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.
(with minor comment)
| $stack =array(); | ||
| $attributes =$this->getAttributes($object,$format,$context); | ||
| $class =\get_class($object); | ||
| $class =$this->objectClassResolver ?\call_user_func($this->objectClassResolver,$object) :\get_class($object); |
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.
could be written as($this->objectClassResolver)($object)
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've copied what was done for themaxDepthHandler.
| $attributeValue =\call_user_func($this->maxDepthHandler,$attributeValue,$object,$attribute,$format,$context); |
It seems to me there is no important difference between the two ways of doing it:https://stackoverflow.com/questions/1596221/php-call-user-func-vs-just-calling-function/35031466
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->objectClassResolver)($object) is more elegant, but indeed being consistent is good
eb5d8d4 to18d2143Comparedunglas commentedOct 3, 2018
Thanks@alanpoulain for working on this feature, this is much appreciated. |
This PR was squashed before being merged into the 4.2-dev branch (closes#28669).Discussion----------[Serializer] Object class resolver| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |When normalizing an object, it could be useful to use a custom method to resolve the class name of it instead of using `get_class`.For instance, Doctrine is using proxy classes for lazy-loading and we usually want the real class and not the proxied one.That's why we are using this trait in API Platform:https://github.com/api-platform/core/blob/master/src/Util/ClassInfoTrait.phpWith this feature, we could solve an issue in API Platform with the JSON-LD normalizer when the eager fetching is disabled.Commits-------18d2143 [Serializer] Object class resolver
| $classDiscriminatorResolver =newClassDiscriminatorFromClassMetadata($classMetadataFactory); | ||
| } | ||
| $this->classDiscriminatorResolver =$classDiscriminatorResolver; | ||
| $this->objectClassResolver =$objectClassResolver; |
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 know it's late, but how about$this->objectClassResolver = $objectClassResolver >: '\get_class'; or something like that ? So that we don't need to test if it is not null on each call
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.
@Taluu : You should provide a PR :)
Uh oh!
There was an error while loading.Please reload this page.
When normalizing an object, it could be useful to use a custom method to resolve the class name of it instead of using
get_class.For instance, Doctrine is using proxy classes for lazy-loading and we usually want the real class and not the proxied one.
That's why we are using this trait in API Platform:https://github.com/api-platform/core/blob/master/src/Util/ClassInfoTrait.php
With this feature, we could solve an issue in API Platform with the JSON-LD normalizer when the eager fetching is disabled.