Make resource constructor parameters writables #1749

Nek- · 2018-03-05T16:25:21Z

This is motivated because other tools are already constructor friendly (ie Symfony serializer and Doctrine).

Also using constructors must be recommended and not supporting them is a serious feature missing.

Q	A
Bug fix?	somehow
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1747
License	MIT
Doc PR	N/A

soyuka · 2018-03-06T08:23:53Z

src/Metadata/Property/Factory/ConstructorPropertyMetadataFactory.php

+        $constructorParameters = $ref->getConstructor()->getParameters();
+        foreach ($constructorParameters as $constructorParameter) {
+            if ($constructorParameter->getName() === $property) {
+                return $propertyMetadata->withWritable(true);


Okay but this doesn't mean that the PropertyAccess component will be able to write to these properties right (denormalization process)?

The PropertyAccessor component directly no, but the Serializer component knows how to use constructors, and constructor support has been dramatically improved in 4.1.

👼 symfony/symfony#25493 👼

Constructors are already supported in the serializer for a while.

Cool then :).

soyuka · 2018-03-06T10:46:08Z

src/Metadata/Property/Factory/ConstructorPropertyMetadataFactory.php

+        }
+
+        // Constructor arguments are obviously accessible only on post operation, put will result in an error.
+        if (!isset($options['collection_operation_name']) || 'post' !== $options['collection_operation_name']) {


What if we have custom collection operations that don't have the post name?

I don't understand the use case, can you be more specific? It doesn't make sense to allow constructor in another request type than post.

He meant this:

collectionOperations={ "foo 10000 " = {"method" = "POST"} }

I guess the best way to fix this is to add the method to the context, what do you think about?

Huum, ResourceMetadata should do the job. For example:

$this->resourceMetadataFactory->create($resourceClass)->getCollectionOperationAttribute($options['collection_operation_name'], 'method')

Fixed. Thanks for the tip, I was not aware of that :) .

soyuka · 2018-03-06T10:47:54Z

src/Bridge/Symfony/Bundle/Resources/config/metadata/metadata.xml

@@ -69,6 +69,10 @@
            <argument type="service" id="api_platform.metadata.property.metadata_factory.serializer.inner" />
        </service>

+        <service id="api_platform.metadata.property.metadata_factory.constructor" class="ApiPlatform\Core\Metadata\Property\Factory\ConstructorPropertyMetadataFactory" decorates="api_platform.metadata.property.metadata_factory" decoration-priority="20" public="false">


Could we use 30 as a priority as well?

Actually 35 makes even more sense (as the serializer SerializerPropertyMetadataFactory will potentially modify this accessibility) !

Nice catch.

Nek- · 2018-03-07T13:20:58Z

In what cases is this possible?

$method = $this->resourceMetadataFactory
    ->create($resourceClass)
    ->getCollectionOperationAttribute($options['collection_operation_name'], 'method')
;
// $method === null

meyerbaptiste · 2018-03-07T13:42:29Z

There is a better way to do that with the OperationMethodResolver class:

$this->operationMethodResolver->getCollectionOperationMethod($resourceClass, $options['collection_operation_name']);

Sorry, I missed this interface...

Nek- · 2018-03-07T13:55:09Z

Indeed. Fixed!

dunglas · 2018-03-08T06:57:07Z

I thought it would be harder! Good job!

Would you mind adding an integration test to be sure we support this new feature everywhere?

soyuka · 2018-03-08T08:03:18Z

src/Metadata/Property/Factory/ConstructorPropertyMetadataFactory.php

+        // Constructor arguments are obviously accessible only on post operation, put will result in an error.
+        try {
+            $method = $this->operationMethodResolver->getCollectionOperationMethod($resourceClass, $options['collection_operation_name']);
+        } catch (RuntimeException $e) {


when does this throw a RuntimeException?

I also asked :) : #1749 (comment)

Here:

core/src/Bridge/Symfony/Routing/OperationMethodResolver.php

Line 96 in 4fa2e4a

throw new RuntimeException(sprintf('Either a "route_name" or a "method" operation attribute must exist for the operation "%s" of the resource "%s".', $operationName, $resourceClass));

It was about how the method can be null (well, ok, the route is null, but it's not the answer).

soyuka · 2018-03-08T08:07:31Z

tests/Metadata/Property/Factory/ConstructorPropertyMetadataFactoryTest.php

+    }
+}
+
+class DummyObjectWithConstructor


Wasn't best practice to put these in another file?

TL;DR: I don't think it's a big deal.

Well, that depends on your POV. IMO re-using dummy classes in many tests is not a good practice because it means your test is dependent on something outside (see FIRST). I do it only in case I need doctrine entities to avoid configuration duplication.

I agree but it's how we choose to do in this project and we have to follow the project rules :).

Is it actually a rule or just something people did without taking care?

I know for a fact that I did the same as you and I was told to move my classes out of the test file that's all :p.

So I guess it's a rule :) .

It's a rule, because it's what is recommended by the PSRs. Anyway, we try to enforce not reusing dummy classes, because of first and - especially - because it's a pain to maintain such tests.

Both can be done together: a test = a dummy class in a separate file. It's what we should enforce.

Couldn't an anonymous class be used ?

Putting them outside doesn't mean reusing them. We should not have more than 1 class in a file, as that wouldn't work for PSR-4 autoloading.

Taluu · 2018-03-10T08:14:01Z

src/Metadata/Property/Factory/ConstructorPropertyMetadataFactory.php

+     */
+    private $operationMethodResolver;
+
+    public function __construct(OperationMethodResolverInterface $operationMethodResolver, PropertyMetadataFactoryInterface $decorated = null)


Why would the decorated object be null ?

so that it can be used without decoration

IMO it's either you decorate something or you don't, Not both at the same time (S from Solid, anyone ?).

Having a basic decorator that just returns a new PropertyMetadata() would be simpler.

@Taluu loaders can be chained in any order (and sometimes you have to change the order, to change the priority). The first registered loader will not be a decorator, but you cannot guess which one will be the first when writing the code (it depends of the config).

Then it means it is the wrong design pattern for these. Shouldn't a strategy be better for that ? of course, nothing prevents some elements in this pattern to be decorators...

Or a chain of responsability would also do it

They was tried, and they introduce a lof of unneeded complexity. They have been a lot of iterations on this part, I don't want to change the again. It's mostly internal, and it works well as is. Please don't over engineer.

+using optional decoration makes it very easy to configure with the Symfony DI component since it supports service decoration and priorities. Using a different pattern such as chain (as it was in API Platform 1) will require to add a lot of specific code to have the same flexibility.

meyerbaptiste · 2018-03-13T11:49:38Z

src/Metadata/Property/Factory/ConstructorPropertyMetadataFactory.php

+            return $propertyMetadata;
+        }
+
+        if ('post' !== strtolower($method)) {


With #1752, 'POST' !== $method is enough. You just have to rebase.

dunglas · 2018-03-14T13:17:47Z

src/Metadata/Property/Factory/ConstructorPropertyMetadataFactory.php

+final class ConstructorPropertyMetadataFactory implements PropertyMetadataFactoryInterface
+{
+    /**
+     * @var PropertyMetadataFactoryInterface


Useless (documented by the typehint), to remove.

Not all editors / etc uses the constructor typehint for properties, so this looks adequate ?

We do that everywhere in the project, and in Symfony as well (we follow most of Symfony's CS). It's less maintenance and PHPStorm supports this so... 😀

yeah, that's why it was more a question, but then I opened a ticket on phpactor. :D

dunglas · 2018-03-14T13:17:53Z

src/Metadata/Property/Factory/ConstructorPropertyMetadataFactory.php

+    private $decorated;
+
+    /**
+     * @var OperationMethodResolverInterface


Useless (documented by the typehint), to remove.

dunglas · 2018-03-14T13:23:07Z

tests/Metadata/Property/Factory/ConstructorPropertyMetadataFactoryTest.php

+    }
+}
+
+class DummyObjectWithConstructor


It's a rule, because it's what is recommended by the PSRs. Anyway, we try to enforce not reusing dummy classes, because of first and - especially - because it's a pain to maintain such tests.

Both can be done together: a test = a dummy class in a separate file. It's what we should enforce.

teohhanhui · 2018-03-14T13:42:15Z

src/Metadata/Property/Factory/ConstructorPropertyMetadataFactory.php

+        }
+
+        foreach ($constructor->getParameters() as $constructorParameter) {
+            if ($constructorParameter->getName() === $property) {


I think we could just do $constructorParameter->name? It seems to be the same.

Nek- · 2018-03-19T14:46:39Z

Review done. Rebased. Behat tests added. 😄

dunglas

Almost good, great job! (but Travis is red currently).

dunglas · 2018-03-20T21:11:12Z

src/Metadata/Property/Factory/ConstructorPropertyMetadataFactory.php

+        }
+
+        foreach ($constructor->getParameters() as $constructorParameter) {
+            if ($constructorParameter->name === $property) {


We must also check that the current value of writable is null (to avoid overwriting a manual setting tru @ApiProperty).

Hum. I think modifying the order of decoration would be better because the property accessor will set this value to "false".

Actually, this issue is also present in the ExtractorPropertyMetadataFactory: if the property accessor decided to set the property to "not accessible" (aka false), no manual configuration can reverse the value because it fills it only if the current value is null.

What do you think of that?

TL;DR: If I do it, this class is useless.

The user's manual configuration must always be able to override anything. And yes, I think this decorator should run after the PropertyInfo decorator.

Ok, so this answer means that there's an issue inside the ExtractorPropertyMetadataFactory.

It also means as I understand that I can indeed override the writable value (so I don't modify this line).

Do you want me to fix everything in this PR ?

Nek- · 2018-03-26T09:24:45Z

I see that the problem I noticed does not generate eruption of enthusiasm... So here is a schema of what's happen (as far as I understand). The problem is more obvious if you look at it. (you may also see some other issues in it)

Nek- · 2018-03-27T14:26:19Z

I fixed my first commit and added a new commit for the new things pointed out. (waiting for feedback)

bendavies · 2018-05-31T08:13:16Z

this can't be merged without tests for relations. let's get them in somehow! @komik966 can PR to @Nek-'s branch?

dunglas · 2018-05-31T08:50:52Z

src/Bridge/Symfony/PropertyInfo/Metadata/Property/PropertyInfoPropertyMetadataFactory.php

+                $propertyMetadata = $propertyMetadata->withInitializable($initializable);
+            }
+        } else {
+            // BC layer for Symfony < 4.1


Symfony 4.2 actually.

dunglas · 2018-05-31T08:52:38Z

src/Metadata/Property/PropertyMetadata.php


-    public function __construct(Type $type = null, string $description = null, bool $readable = null, bool $writable = null, bool $readableLink = null, bool $writableLink = null, bool $required = null, bool $identifier = null, string $iri = null, $childInherited = null, array $attributes = null, SubresourceMetadata $subresource = null)
+    public function __construct(Type $type = null, string $description = null, bool $readable = null, bool $writable = null, bool $readableLink = null, bool $writableLink = null, bool $required = null, bool $identifier = null, string $iri = null, $childInherited = null, array $attributes = null, SubresourceMetadata $subresource = null, $initializable = null)


bool $initializable = null

soyuka · 2018-05-31T09:33:09Z

@Nek- @komik966 if you want me to take care of adding the tests written in #1843 in this patch let me know

Nek- · 2018-05-31T09:46:34Z

@soyuka I prefer to not add those tests in this MR if you don't mind.

teohhanhui · 2018-05-31T13:12:52Z

src/Bridge/Symfony/PropertyInfo/Metadata/Property/PropertyInfoPropertyMetadataFactory.php

+            }
+        } else {
+            // BC layer for Symfony < 4.2
+            // To be removed in EOF of Symfony 3.4


I don't understand this comment. If it's a BC layer for Symfony < 4.2, shouldn't it only be removed when we drop support for Symfony < 4.2?

Yes we can just drop the last line

Yes my bad!

komik966 · 2018-05-31T13:35:45Z

added tests in #1991
without modifications in ApiPlatform\Core\Serializer\AbstractItemNormalizer relations will not work

teohhanhui · 2018-05-31T16:58:09Z

The tests should be in this PR.

komik966 · 2018-06-01T06:08:46Z

to add tests here @Nek- can rebase from my branch

This is motivated because other tools are already constructor friendly (ie Symfony serializer and Doctrine). Also using constructors must be recommended and not supporting them is a serious feature missing.

Nek- · 2018-06-01T08:08:37Z

Okok guys, I was more in favor of merging PRs separately. But well, if you disagree then please review tests in #1991 first because they are not ok to me (and I assume also not for you).

dunglas · 2018-06-08T12:02:46Z

Thank you very much for this @Nek-! It's very appreciated.

bendavies · 2018-06-08T12:30:59Z

great, can't wait to try this.

@komik966 can you work on the tests now?

komik966 · 2018-06-08T12:43:26Z

In June I've limited time, so work may be little slow. But I think we are very close to fully working relations. Partially it was implemented in my PR (without handling circular references).

teohhanhui · 2018-06-08T12:47:00Z

This is great work, but I think it's unfortunate that we've merged this without tests.

Nek- · 2018-06-08T14:16:34Z

@teohhanhui it is actually tested.

bendavies · 2018-06-11T10:16:42Z

Can we just clarify what needs to be done here so we don't leave work to be done, as i'm a little confused.

From reading above @Nek-, you didn't want @komik966's tests in your branch, but now you say it is actually tested?

Thanks!

Nek- · 2018-06-11T10:21:13Z

@bendavies this is a test: https://github.com/api-platform/core/pull/1749/files#diff-b63a56b1101fe1091a9cf488e2b0b383R1

The branch of komik966 is nice because it adds many other tests, but it was not mergeable as it to me (and not only to me as I understand).

What's the problem? I don't understand. (If you ask if you can use the feature, yes you can)

bendavies · 2018-06-11T10:26:49Z

thanks @Nek-.
i was trying to work out if there are tests in @komik966's branch that covered cases not in your tests.

soyuka · 2018-06-11T12:35:04Z

@komik966's tests are covering relations from what I can see and there might be an issue there. Also, because of the red coverage, I think that this could be more tested then it is already :).

… 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

Nek- force-pushed the feature/make-constructor-parameters-writable branch from 6f74b32 to addec68 Compare March 5, 2018 16:26

soyuka reviewed Mar 6, 2018

View reviewed changes

Nek- force-pushed the feature/make-constructor-parameters-writable branch 2 times, most recently from 4e512a2 to 1ea3cca Compare March 6, 2018 10:11

soyuka reviewed Mar 6, 2018

View reviewed changes

Nek- force-pushed the feature/make-constructor-parameters-writable branch 2 times, most recently from ee9125a to eb06080 Compare March 7, 2018 13:07

Nek- force-pushed the feature/make-constructor-parameters-writable branch 2 times, most recently from e9cfcd8 to 05c4a15 Compare March 7, 2018 13:41

Nek- force-pushed the feature/make-constructor-parameters-writable branch 2 times, most recently from 3960779 to 3c55e3d Compare March 7, 2018 13:55

Nek- force-pushed the feature/make-constructor-parameters-writable branch from 3c55e3d to c0bbcaf Compare March 7, 2018 17:14

soyuka reviewed Mar 8, 2018

View reviewed changes

Taluu reviewed Mar 10, 2018

View reviewed changes

meyerbaptiste requested changes Mar 13, 2018

View reviewed changes

dunglas requested changes Mar 14, 2018

View reviewed changes

teohhanhui reviewed Mar 14, 2018

View reviewed changes

Nek- force-pushed the feature/make-constructor-parameters-writable branch from c0bbcaf to d6b1556 Compare March 19, 2018 14:46

dunglas requested changes Mar 20, 2018

View reviewed changes

dunglas mentioned this pull request Mar 20, 2018

setup API platform msgphp/symfony-demo-app#28

Merged

10 tasks

Nek- force-pushed the feature/make-constructor-parameters-writable branch from d6b1556 to 53f9ba3 Compare March 27, 2018 14:25

dunglas requested changes May 31, 2018

View reviewed changes

Nek- force-pushed the feature/make-constructor-parameters-writable branch from 992c1f5 to aac5d55 Compare May 31, 2018 09:44

Nek- force-pushed the feature/make-constructor-parameters-writable branch from aac5d55 to d542e8c Compare May 31, 2018 09:46

teohhanhui reviewed May 31, 2018

View reviewed changes

komik966 mentioned this pull request May 31, 2018

tests for: Make resource constructor parameters writables #1991

Closed

Make resource constructor parameters writables

da500b2

This is motivated because other tools are already constructor friendly (ie Symfony serializer and Doctrine). Also using constructors must be recommended and not supporting them is a serious feature missing.

Nek- force-pushed the feature/make-constructor-parameters-writable branch from d542e8c to da500b2 Compare June 1, 2018 07:30

dunglas merged commit c81543a into api-platform:master Jun 8, 2018

Nek- deleted the feature/make-constructor-parameters-writable branch June 8, 2018 14:15

teohhanhui mentioned this pull request Jun 20, 2018

Cannot pass an IRI in JSON for a constructor parameter #2036

Closed

bendavies mentioned this pull request Aug 23, 2018

Allow Constructors with Relationships #2169

Closed

This was referenced Aug 25, 2018

Constructor relations writtable #2178

Merged

[Serializer] add method for preparing constructor argument symfony/symfony#28263

Closed


		public function __construct(Type $type = null, string $description = null, bool $readable = null, bool $writable = null, bool $readableLink = null, bool $writableLink = null, bool $required = null, bool $identifier = null, string $iri = null, $childInherited = null, array $attributes = null, SubresourceMetadata $subresource = null)
		public function __construct(Type $type = null, string $description = null, bool $readable = null, bool $writable = null, bool $readableLink = null, bool $writableLink = null, bool $required = null, bool $identifier = null, string $iri = null, $childInherited = null, array $attributes = null, SubresourceMetadata $subresource = null, $initializable = null)

Uh oh!

Make resource constructor parameters writables #1749

Make resource constructor parameters writables #1749

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

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!

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!