value object as ApiResource #1843
Conversation
|
Very nice. This is what I'd be looking to use |
| } | ||
|
|
||
| $constructor = $this->getConstructor($data, $class, $context, $reflectionClass, $allowedAttributes); | ||
| if ($constructor) { |
| if ($constructor) { | ||
| $constructorParameters = $constructor->getParameters(); | ||
|
|
||
| $params = array(); |
There was a problem hiding this comment.
you can use short array syntax
| } | ||
|
|
||
| /** | ||
| * @throws ReflectionException |
There was a problem hiding this comment.
we do not document exception not thrown directly by the method
|
|
||
| class ConstructorExtractor implements PropertyAccessExtractorInterface | ||
| { | ||
| public function isReadable($class, $property, array $context = array()) |
There was a problem hiding this comment.
you can use short array syntax
features/main/value_object.feature
Outdated
| And the JSON should be equal to: | ||
| """ | ||
| { | ||
| "@context": "\/contexts\/VoDummyCar", |
There was a problem hiding this comment.
you should'nt need to escape the json
|
from my POV, sounds like a great feature, consistent with doctrine’s recommendation on using rich entities (https://www.doctrine-project.org/projects/doctrine-orm/en/latest/tutorials/getting-started.html#adding-behavior-to-entities) |
| use ReflectionException; | ||
| use Symfony\Component\PropertyInfo\PropertyAccessExtractorInterface; | ||
|
|
||
| class ConstructorExtractor implements PropertyAccessExtractorInterface |
There was a problem hiding this comment.
I guess yes, this could be contributed directly in symfony, but before doing it, please wait for @dunglas POV.
There was a problem hiding this comment.
I don't think so. The fact that a parameter is writable is driven by the fact that you can write it for an instantiated object. So this would probably never be inside the Symfony component.
There was a problem hiding this comment.
I tend to agree, this a low level and common need, it should be in Symfony, not in API Platform (history: we moved the code PropertyInfo code from API Platform to a standalone library then to Symfony for this exact same reason).
There was a problem hiding this comment.
Adding property_info.initializable_extractor tag is better than using property_info.access_extractor, I like this approach. It also solves the problem which @Nek- pointed out
| $key = $this->nameConverter ? $this->nameConverter->normalize($paramName) : $paramName; | ||
|
|
||
| $allowed = false === $allowedAttributes || \in_array($paramName, $allowedAttributes); | ||
| $ignored = !$this->isAllowedAttribute($class, $paramName, $format, $context); |
There was a problem hiding this comment.
I guess this could be directly called as $ignored, or renamed ?
because following the codes path you are using ! two times on this variable.
| $allowed = false === $allowedAttributes || \in_array($paramName, $allowedAttributes); | ||
| $ignored = !$this->isAllowedAttribute($class, $paramName, $format, $context); | ||
| if ($constructorParameter->isVariadic()) { | ||
| if ($allowed && !$ignored && (isset($data[$key]) || array_key_exists($key, $data))) { |
There was a problem hiding this comment.
why using both isset and array_key_exists?
There was a problem hiding this comment.
For performance I guess (isset is faster)
|
Thanks for review, I'll fix it. |
|
@Simperfit fixed, I leaved |
| use ReflectionException; | ||
| use Symfony\Component\PropertyInfo\PropertyAccessExtractorInterface; | ||
|
|
||
| class ConstructorExtractor implements PropertyAccessExtractorInterface |
There was a problem hiding this comment.
I tend to agree, this a low level and common need, it should be in Symfony, not in API Platform (history: we moved the code PropertyInfo code from API Platform to a standalone library then to Symfony for this exact same reason).
| $allowed = false === $allowedAttributes || \in_array($paramName, $allowedAttributes); | ||
| $ignored = !$this->isAllowedAttribute($class, $paramName, $format, $context); | ||
| if ($constructorParameter->isVariadic()) { | ||
| if ($allowed && !$ignored && (isset($data[$key]) || array_key_exists($key, $data))) { |
There was a problem hiding this comment.
For performance I guess (isset is faster)
| if ($constructorParameter->isVariadic()) { | ||
| if ($allowed && !$ignored && (isset($data[$key]) || array_key_exists($key, $data))) { | ||
| if (!\is_array($data[$paramName])) { | ||
| throw new RuntimeException(sprintf('Cannot create an instance of %s from serialized data because the variadic parameter %s can only accept an array.', |
There was a problem hiding this comment.
Can you add quotes around values in the message?
| { | ||
| $constructor = (new ReflectionClass($class))->getConstructor(); | ||
|
|
||
| if(!$constructor) { |
There was a problem hiding this comment.
You should test if the class is instantiable.
| } | ||
|
|
||
| $constructor = $this->getConstructor($data, $class, $context, $reflectionClass, $allowedAttributes); | ||
| if ($constructor) { |
There was a problem hiding this comment.
if (!$constructor = $this->getConstructor($data, $class, $context, $reflectionClass, $allowedAttributes)) {
return new $class();
}|
almost done in #1749 |
… is initializable (dunglas) This PR was squashed before being merged into the 4.2-dev branch (closes #26997). Discussion ---------- [PropertyInfo] Add an extractor to guess if a property is initializable | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | n/a <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | todo When dealing with value objects, being able to detect if a property can be initialized using the constructor is a very valuable information. It's mandatory to add a proper value object support in API Platform and in the Serializer component. See api-platform/core#1749 and api-platform/core#1843 for the related discussions, extended use cases and proof of concepts. This PR adds a new interface to guess if a property can be initialized through the constructor, and an implementation using the reflection (in `ReflectionExtractor`). Commits ------- 9d2ab9e [PropertyInfo] Add an extractor to guess if a property is initializable
I'm aware that this PR still needs work, but I opened it to discuss about value objects as ApiResource.
To get the idea check behat test: https://github.com/api-platform/core/compare/master...komik966:vo-denormalizer?expand=1#diff-6af974d3c9848f846d1ee1e58d88564a and entity: https://github.com/api-platform/core/compare/master...komik966:vo-denormalizer?expand=1#diff-e1fa2952eafb66484a8db3d626ddba31
If you agree with it I'll do further work.
Found (potential) bug:
I've not hardly investigated it but it seems that
PurgeHttpCacheListenerhas bug - i tries to get entity id before one is created, but not in every case - it not works on entities which I've added in this PR (VoDummy*). For now I commented the line which causes issue (https://github.com/api-platform/core/compare/master...komik966:vo-denormalizer?expand=1#diff-07581ee0a08a8e5193cb8a7ec890c3b7R83)Questions:
symfony/property-access?instantiateObject(https://github.com/api-platform/core/compare/master...komik966:vo-denormalizer?expand=1#diff-3e45e8e6ce08f8a7cbbbda1601d5ff29R170) is almost copied from symfony'sAbstractNormalizer- it only adds execution of new methodprepareAttributeValue(https://github.com/api-platform/core/compare/master...komik966:vo-denormalizer?expand=1#diff-3e45e8e6ce08f8a7cbbbda1601d5ff29R204) - shouldn't it be included insymfony/serializer?TODOS:
PurgeHttpCacheListenerissue