value object as ApiResource by komik966 · Pull Request #1843 · api-platform/core

komik966 · 2018-04-11T09:15:16Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets	n/a
License	MIT
Doc PR	n/a

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 PurgeHttpCacheListener has 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:

ConstructorExtractor (https://github.com/api-platform/core/compare/master...komik966:vo-denormalizer?expand=1#diff-71d84e71877454b3e75f141ed3b2f784) shouldn't be included in symfony/property-access ?
method instantiateObject (https://github.com/api-platform/core/compare/master...komik966:vo-denormalizer?expand=1#diff-3e45e8e6ce08f8a7cbbbda1601d5ff29R170) is almost copied from symfony's AbstractNormalizer - it only adds execution of new method prepareAttributeValue (https://github.com/api-platform/core/compare/master...komik966:vo-denormalizer?expand=1#diff-3e45e8e6ce08f8a7cbbbda1601d5ff29R204) - shouldn't it be included in symfony/serializer?

TODOS:

lack of required param should show violations (like validation error), not generic error (https://github.com/api-platform/core/compare/master...komik966:vo-denormalizer?expand=1#diff-6af974d3c9848f846d1ee1e58d88564aR93)
add tests with IRI
handle OneToMany circular (https://github.com/api-platform/core/compare/master...komik966:vo-denormalizer?expand=1#diff-6af974d3c9848f846d1ee1e58d88564aR26, https://github.com/api-platform/core/compare/master...komik966:vo-denormalizer?expand=1#diff-cb3c4ced4e06b3eb1231ca0473a03903R43)
add documentation
solve PurgeHttpCacheListener issue

komik966 · 2018-04-13T08:12:56Z

symfony/symfony#25605

komik966 · 2018-04-13T08:59:31Z

#1749

bendavies · 2018-04-13T09:12:41Z

Very nice. This is what I'd be looking to use

Simperfit · 2018-04-13T09:15:03Z

+        }
+
+        $constructor = $this->getConstructor($data, $class, $context, $reflectionClass, $allowedAttributes);
+        if ($constructor) {


you can inline this

Simperfit · 2018-04-13T09:15:29Z

+        if ($constructor) {
+            $constructorParameters = $constructor->getParameters();
+
+            $params = array();


you can use short array syntax

Simperfit · 2018-04-13T09:18:19Z

+    }
+
+    /**
+     * @throws ReflectionException


we do not document exception not thrown directly by the method

Simperfit · 2018-04-13T09:18:46Z

+
+class ConstructorExtractor implements PropertyAccessExtractorInterface
+{
+    public function isReadable($class, $property, array $context = array())


you can use short array syntax

Simperfit · 2018-04-13T09:22:17Z

+    And the JSON should be equal to:
+    """
+     {
+         "@context": "\/contexts\/VoDummyCar",


you should'nt need to escape the json

gorghoa · 2018-04-13T09:22:38Z

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)

Simperfit · 2018-04-13T09:22:48Z

+use ReflectionException;
+use Symfony\Component\PropertyInfo\PropertyAccessExtractorInterface;
+
+class ConstructorExtractor implements PropertyAccessExtractorInterface


I guess yes, this could be contributed directly in symfony, but before doing it, please wait for @dunglas POV.

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.

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).

See symfony/symfony#26997.

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

Simperfit · 2018-04-13T09:24:16Z

+                $key = $this->nameConverter ? $this->nameConverter->normalize($paramName) : $paramName;
+
+                $allowed = false === $allowedAttributes || \in_array($paramName, $allowedAttributes);
+                $ignored = !$this->isAllowedAttribute($class, $paramName, $format, $context);


I guess this could be directly called as $ignored, or renamed ?

because following the codes path you are using ! two times on this variable.

Simperfit · 2018-04-13T09:24:59Z

+                $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))) {


why using both isset and array_key_exists?

For performance I guess (isset is faster)

Simperfit

This is really nice ! Thanks for contributing it to api-platform.

What was the main reason to develop this, I'm curious ?

I've left some comments.

komik966 · 2018-04-13T09:37:08Z

Thanks for review, I'll fix it.
The reason was the need for properties without setters - in some cases it simplifies our domain model.

komik966 · 2018-04-13T10:18:37Z

@Simperfit fixed, I leaved AbstractItemNormalizer::instantiateObject as is before we decide what to do with it (it is almost copied from https://github.com/symfony/serializer/blob/4.0/Normalizer/AbstractNormalizer.php#L312)

dunglas · 2018-04-20T10:14:03Z

+use ReflectionException;
+use Symfony\Component\PropertyInfo\PropertyAccessExtractorInterface;
+
+class ConstructorExtractor implements PropertyAccessExtractorInterface


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).

dunglas · 2018-04-20T10:16:35Z

+                $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))) {


For performance I guess (isset is faster)

dunglas · 2018-04-20T10:17:21Z

+                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.',


Can you add quotes around values in the message?

meyerbaptiste · 2018-04-20T14:30:29Z

+    {
+        $constructor = (new ReflectionClass($class))->getConstructor();
+
+        if(!$constructor) {


You should test if the class is instantiable.

meyerbaptiste · 2018-04-20T14:39:41Z

+        }
+
+        $constructor = $this->getConstructor($data, $class, $context, $reflectionClass, $allowedAttributes);
+        if ($constructor) {


if (!$constructor = $this->getConstructor($data, $class, $context, $reflectionClass, $allowedAttributes)) { return new $class(); }

komik966 · 2018-05-30T16:57:03Z

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  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | n/a  | 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

mysiar requested review from dunglas and meyerbaptiste April 12, 2018 05:54

komik966 mentioned this pull request Apr 13, 2018

Make resource constructor parameters writables #1749

Merged

Simperfit reviewed Apr 13, 2018

View reviewed changes

Simperfit suggested changes Apr 13, 2018

View reviewed changes

dunglas requested changes Apr 20, 2018

View reviewed changes

meyerbaptiste requested changes Apr 20, 2018

View reviewed changes

dunglas mentioned this pull request Apr 21, 2018

[PropertyInfo] Add an extractor to guess if a property is initializable symfony/symfony#26997

Merged

meyerbaptiste requested changes Apr 21, 2018

View reviewed changes

Amo mentioned this pull request Apr 26, 2018

XX-99 Github to Zapier to JIRA tests Amo/Collection#1

Closed

komik966 force-pushed the vo-denormalizer branch from aba17aa to 8bd2617 Compare May 17, 2018 13:26

dunglas mentioned this pull request May 23, 2018

Non anemic doctrine models are not supported #1747

Closed

vo-denormalizer

8438863

komik966 force-pushed the vo-denormalizer branch from 8bd2617 to 8438863 Compare May 29, 2018 08:06

komik966 closed this May 30, 2018

Uh oh!

Conversation

komik966 commented Apr 11, 2018 • edited by mysiar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

komik966 commented Apr 13, 2018

Uh oh!

komik966 commented Apr 13, 2018

Uh oh!

bendavies commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gorghoa commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nek- Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

komik966 Apr 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit left a comment

Choose a reason for hiding this comment

Uh oh!

komik966 commented Apr 13, 2018

Uh oh!

komik966 commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

komik966 commented May 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

komik966 commented Apr 11, 2018 •

edited by mysiar

Loading

Nek- Apr 13, 2018 •

edited

Loading

komik966 Apr 21, 2018 •

edited

Loading