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

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

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	symfony/symfony-docs#10319

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

bendavies · 2018-04-21T12:58:21Z

src/Symfony/Component/PropertyInfo/Tests/Fixtures/NullExtractor.php

+     */
+    public function isInitializable(string $class, string $property, array $context = array()): ?bool
+    {
+        $this->assertIsString($class);


What's the point of this?

I added this for consistency with other methods, I'm not sure of the origin intent...

Here is the original intent: #19437

Right. So type hints are added now so these checks are redundant

sroze · 2018-04-21T14:24:33Z

src/Symfony/Component/PropertyInfo/PropertyInitializableExtractorInterface.php

+    /**
+     * Is the property initializable?
+     */
+    public function isInitializable(string $class, string $property, array $context = array()): ?bool;


What's the use-case for returning null? Also, shouldn't we explicit within the method name that we are talking about the constructor in here? isInitializableViaConstructor or something like that?

null means "we don't know"?

@teohhanhui is right. It means "this extractor cannot guess", try the next registered in the chain. It's mandatory to keep this behavior for consistency with existing interfaces and to provide an extension point.

@sroze I prefer the current name, isInitializableViaConstructor is too verbose and brings nothing much.

My 2cts are that I didn't understand what isInitializable meant before reading more about your PR. I can tell you that if I saw this interface I'd think "wait. What is that about?". While the isInitializableViaConstructor clarifies it and is not too verbose.

I wouldn't, the PhpDoc that you've put is enough (to me).

"Property initializable" on its own indeed makes no sense to me. Perhaps ConstructorInitializableExtractorInterface::isPropertyInitializable?

PropertyInitializableViaConstructorExtractorInterface::isInitializable?

I guess we could just stick with the current names. Lol...

@sroze @fabpot ok for you to keep the current names?

lyrixx

Looks good to me

Taluu · 2018-05-02T22:43:37Z

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php

-            if ($property !== $parameter->name) {
-                continue;
+            if ($property === $parameter->name) {
+                return array($this->extractFromReflectionType($parameter->getType()));
            }


what's the point of this change ? I know it does the same, but with a different style, but then ?

Removing 2 lines and making the code more explicit, but yes it's not mandatory.

meyerbaptiste · 2018-05-29T14:24:03Z

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php

+            return null;
+        }
+
+        if ($constructor = $reflectionClass->getConstructor()) {


Why don't you test if the constructor is instantiable first?

Good idea, I'll update the code.

nicolas-grekas · 2018-06-19T11:23:20Z

src/Symfony/Component/PropertyInfo/PropertyInitializableExtractorInterface.php

+interface PropertyInitializableExtractorInterface
+{
+    /**
+     * Is the property initializable?


not sure this is useful :)

It's for consistency with other interfaces.

nicolas-grekas · 2018-06-19T11:23:44Z

rebase needed (and some comments to address)

dunglas · 2018-06-30T09:00:37Z

rebased and comments addressed

sroze · 2018-06-30T09:21:06Z

src/Symfony/Component/PropertyInfo/PropertyInitializableExtractorInterface.php

+    /**
+     * Is the property initializable?
+     */
+    public function isInitializable(string $class, string $property, array $context = array()): ?bool;


My 2cts are that I didn't understand what isInitializable meant before reading more about your PR. I can tell you that if I saw this interface I'd think "wait. What is that about?". While the isInitializableViaConstructor clarifies it and is not too verbose.

src/Symfony/Bundle/FrameworkBundle/Resources/config/property_info.xml

dunglas · 2018-07-13T11:51:06Z

Rebased, I suggest to keep the current name as there are no consensus on another name.

fabpot · 2018-07-13T12:22:45Z

src/Symfony/Component/PropertyInfo/CHANGELOG.md

+4.2.0
+-----
+
+* added `PropertyInitializableExtractorInterface` to test if a property can be initialized through the constructor and an implementation is `ReflectionExtractor` 


if a property can be initialized through the constructor (implemented by ReflectionExtractor)

fabpot · 2018-07-13T12:24:29Z

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php

+
+        if ($constructor = $reflectionClass->getConstructor()) {
+            foreach ($constructor->getParameters() as $parameter) {
+                if ($property === $parameter->name) {


The fact that the constructor parameter name must match the property one should be documented somewhere.

fabpot · 2018-07-13T12:24:58Z

src/Symfony/Component/PropertyInfo/PropertyInitializableExtractorInterface.php

+interface PropertyInitializableExtractorInterface
+{
+    /**
+     * Is the property initializable?


This is probably where we need more docs about the naming convention.

fabpot · 2018-09-04T09:54:26Z

@dunglas Do you have time to have a look at my comments?

dunglas · 2018-09-04T11:01:14Z

@fabpot done

dunglas · 2018-09-04T11:11:07Z

CI errors look unrelated.

src/Symfony/Component/PropertyInfo/CHANGELOG.md

fabpot · 2018-09-04T15:18:05Z

Thank you @dunglas.

… 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

nicolas-grekas · 2018-09-05T06:59:58Z

This PR breaks the CI by breaking BC somehow, see deps=high failures (which mean that 4.1 is broken when using PropertyInfo master)
@dunglas could you have a look please?
(note that we should be careful when merging PRs which have failures)

nicolas-grekas · 2018-09-05T09:03:08Z

(fixed in 5557e38, we'll have to deal with this kind of BC breaks now that we put DI passes in components and not in bundles anymore.)

dunglas · 2018-09-05T14:24:32Z

Thanks for the fix @nicolas-grekas

javiereguiluz · 2018-09-07T13:48:32Z

I've created symfony/symfony-docs#10272 to document this new feature. Please, don't forget to create a doc issue for every new feature.

Kévin, if you don't have time to contribute the docs, we'll need some code examples of using this feature in action. Thanks!

…s initializable (dunglas, javiereguiluz) This PR was merged into the master branch. Discussion ---------- [PropertyInfo] Add an extractor to guess if a property is initializable symfony/symfony#26997 symfony/symfony#24571 Closes #10272. Commits ------- 8415f96 Minor tweaks eebca4a RST f7ed144 [PropertyInfo] Add an extractor to guess if a property is initializable

dunglas requested a review from lyrixx April 21, 2018 10:20

carsonbot added Status: Needs Review PropertyInfo Feature labels Apr 21, 2018

dunglas force-pushed the propertyinfo-constructor branch from a95d0a8 to 1f2103e Compare April 21, 2018 10:21

This was referenced Apr 21, 2018

Make resource constructor parameters writables api-platform/core#1749

Merged

value object as ApiResource api-platform/core#1843

Closed

bendavies reviewed Apr 21, 2018

View reviewed changes

sroze reviewed Apr 21, 2018

View reviewed changes

nicolas-grekas added this to the next milestone Apr 22, 2018

lyrixx reviewed Apr 23, 2018

View reviewed changes

Taluu reviewed May 2, 2018

View reviewed changes

Taluu approved these changes May 2, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels May 2, 2018

dunglas mentioned this pull request May 23, 2018

Non anemic doctrine models are not supported api-platform/core#1747

Closed

meyerbaptiste reviewed May 29, 2018

View reviewed changes

nicolas-grekas reviewed Jun 19, 2018

View reviewed changes

dunglas force-pushed the propertyinfo-constructor branch from 88b2286 to 17cf6a1 Compare June 30, 2018 08:59

sroze reviewed Jun 30, 2018

View reviewed changes

dunglas force-pushed the propertyinfo-constructor branch from fced167 to 6737e71 Compare July 13, 2018 11:48

fabpot requested changes Jul 13, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Jul 13, 2018

dunglas force-pushed the propertyinfo-constructor branch 2 times, most recently from d125f25 to 4a0483d Compare September 4, 2018 11:00

fabpot approved these changes Sep 4, 2018

View reviewed changes

src/Symfony/Component/PropertyInfo/CHANGELOG.md Outdated Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Work labels Sep 4, 2018

[PropertyInfo] Add an extractor to guess if a property is initializable

9d2ab9e

fabpot force-pushed the propertyinfo-constructor branch from a8fb40e to 9d2ab9e Compare September 4, 2018 15:17

fabpot merged commit 9d2ab9e into symfony:master Sep 4, 2018

dunglas deleted the propertyinfo-constructor branch September 5, 2018 14:23

javiereguiluz mentioned this pull request Sep 7, 2018

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

Closed

dunglas mentioned this pull request Sep 10, 2018

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

Merged

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

skalpa mentioned this pull request Dec 3, 2018

[PropertyInfo][BC Break] Attempted to call an undefined method named "isInitializable" #29440

Closed

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

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

Conversation

dunglas commented Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teohhanhui Jun 30, 2018 • edited Loading

Choose a reason for hiding this comment

dunglas Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

lyrixx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jun 19, 2018

dunglas commented Jun 30, 2018

Choose a reason for hiding this comment

dunglas commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Sep 4, 2018

dunglas commented Sep 4, 2018

dunglas commented Sep 4, 2018

fabpot commented Sep 4, 2018

nicolas-grekas commented Sep 5, 2018 • edited Loading

nicolas-grekas commented Sep 5, 2018 • edited Loading

dunglas commented Sep 5, 2018

javiereguiluz commented Sep 7, 2018

dunglas commented Apr 21, 2018 •

edited

Loading

dunglas Apr 22, 2018 •

edited

Loading

teohhanhui Jun 30, 2018 •

edited

Loading

dunglas Jul 3, 2018 •

edited

Loading

dunglas Jul 4, 2018 •

edited

Loading

nicolas-grekas commented Sep 5, 2018 •

edited

Loading

nicolas-grekas commented Sep 5, 2018 •

edited

Loading