[PropertyInfo] Implement "Collection" types in PhpDocExtractor #26300

popy-dev · 2018-02-24T17:38:18Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#26299
License	MIT
Doc PR	todo

Here's a proposition of implementation of my feature request #26299 :
I added few tests covering the requested feature, and I had to change a few things in the Symfony\Component\PropertyInfo\Util\PhpDocTypeHelper class (createType no longer gets a string, but a phpDocumentor\Reflection\Type instance) to be able to detect properly Collections and their subtypes.

Of course a simpler implementation is possible, without changing the PhpDocTypeHelper internal behaviour, by matching the input string against /^([^>]+)</ and extracting the classname alone.

Simperfit · 2018-02-24T19:51:26Z

src/Symfony/Component/PropertyInfo/Util/PhpDocTypeHelper.php

@@ -49,16 +50,15 @@ public function getTypes(DocType $varType): array

        $varTypes = array();
        for ($typeIndex = 0; $varType->has($typeIndex); ++$typeIndex) {
-            $varTypes[] = (string) $varType->get($typeIndex);
-        }
+            $t = $varType->get($typeIndex);


WDYT of using $type instead, it will be better for the readability ?

I got the habbit of using one-char variable names for variables having such a small scope in the function. But you're right, $type would be better.

Simperfit · 2018-02-24T19:52:16Z

src/Symfony/Component/PropertyInfo/Util/PhpDocTypeHelper.php

+            // More than 1 type returned means it is a Compound type, which is
+            // not handled by Type, so better use a null value.
+            $key = 1 === count($key) ? $key[0] : null;
+            $value = 1 === count($value) ? $value[0] : null;


Simperfit · 2018-02-24T19:52:20Z

src/Symfony/Component/PropertyInfo/Util/PhpDocTypeHelper.php

+
+            // More than 1 type returned means it is a Compound type, which is
+            // not handled by Type, so better use a null value.
+            $key = 1 === count($key) ? $key[0] : null;


Simperfit · 2018-02-24T19:55:18Z

src/Symfony/Component/PropertyInfo/Util/PhpDocTypeHelper.php

@@ -39,7 +40,7 @@ public function getTypes(DocType $varType): array
                $nullable = true;
            }

-            $type = $this->createType((string) $varType, $nullable);
+            $type = $this->createType($varType, $nullable);
            if (null !== $type) {


you can inline this IMO

Yeah since you are not modifying it, forgot about this ;). It will be harder to merge things back to master.

Yes, I tried to modify as few lines as possible, to keep diffs shorts.

Simperfit · 2018-02-25T08:40:40Z

It seems that the broken tests are linked to this change, could you please check why ?

popy-dev · 2018-02-25T08:55:38Z

My bad : the added properties in the Fixture made ReflectionExtractor->getProperties test fail (obviously), and i didn't notice as I didn't run tests on the whole component. Lesson learned.

Strange enougth, the travis check on php 7.1 didn't fail.

popy-dev · 2018-02-25T09:21:11Z

Damn. I don't reproduce the issue on php 7.2
What I understand from the output is that somehow, one of these 2 lines retruns an array instead of a null

    $key = 1 === \count($key) ? $key[0] : null;
    $value = 1 === \count($value) ? $value[0] : null;

Simperfit · 2018-02-25T11:19:27Z

Its maybe because of the deps =high ill try to look when a get a computer ;) Le dim. 25 févr. 2018 à 10:21, Popy <notifications@github.com> a écrit :

…

Damn. I don't reproduce the issue on php 7.2 What I understand from the output is that somehow, one of these 2 lines retruns an array instead of a null $key = 1 === \count($key) ? $key[0] : null; $value = 1 === \count($value) ? $value[0] : null; — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#26300 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADSq8jaCjByDgkLOr3yaCSr4WgrAVsU3ks5tYSYSgaJpZM4SR-Qa> .

popy-dev · 2018-02-25T12:51:42Z

Indeed : the 7.2 is checked with "deps=low". I tested after loading lowest dependencies, and I reproduced.

popy-dev · 2018-02-25T13:14:43Z

There's a "fix" : I updated the "conflicts" section, forcing phpdocumentor/type-resolver to, at least, version 0.5.0 where the Collection have been included.

The issue is now that it bumps phpdocumentor/reflection-docblock version to dev-master, as each version of it depends on a fixed version (^0.4.0 for instance) of phpdocumentor/type-resolver.

nicolas-grekas · 2018-02-25T20:51:17Z

composer.json

@@ -100,7 +100,7 @@
    },
    "conflict": {
        "phpdocumentor/reflection-docblock": "<3.0||>=3.2.0,<3.2.2",
-        "phpdocumentor/type-resolver": "<0.2.1",
+        "phpdocumentor/type-resolver": "<0.5.0",


no need to bump: instead, just skip the tests that cannot run when applicable

The tests are just new entries in an existing dataprovider. What would be the better way to skip them ? If thought of not adding them, but they won't show as skipped.

I'd suggest to just create a new test case with separate provide method, + skipping logic inside

nicolas-grekas

with minor comments
ping @dunglas for review, if you can

nicolas-grekas · 2018-03-19T16:50:53Z

src/Symfony/Component/PropertyInfo/Tests/Extractors/PhpDocExtractorTest.php

+     */
+    public function testExtractCollection($property, array $type = null, $shortDescription, $longDescription)
+    {
+        if (!class_exists('phpDocumentor\\Reflection\\Types\\Collection')) {


we should use Collection::class instead

nicolas-grekas · 2018-03-19T16:51:24Z

src/Symfony/Component/PropertyInfo/Tests/Extractors/PhpDocExtractorTest.php

+        $this->testExtract($property, $type, $shortDescription, $longDescription);
+    }
+
+    public function collectionsTypesProvider()


provideCollectionTypes

nicolas-grekas · 2018-03-19T16:51:48Z

src/Symfony/Component/PropertyInfo/Util/PhpDocTypeHelper.php

@@ -14,6 +14,7 @@
 use phpDocumentor\Reflection\Type as DocType;
 use phpDocumentor\Reflection\Types\Compound;
 use phpDocumentor\Reflection\Types\Null_;
+use phpDocumentor\Reflection\Types\Collection;


alpha order

dunglas · 2018-03-19T17:17:33Z

src/Symfony/Component/PropertyInfo/Util/PhpDocTypeHelper.php

+            $key = $this->getTypes($type->getKeyType());
+            $value = $this->getTypes($type->getValueType());
+
+            // More than 1 type returned means it is a Compound type, which is


Do you think we can improve the type VO (it's a final class exactly for this reason) to support this?

I guess it could be done without much shenanigans (adding a counpound internal type, and a property holding composing types).

But i'm afraid that would be a breaking change, as the component will start to return non-null types where it previously did.

nicolas-grekas · 2018-04-20T12:32:03Z

@popy-dev would you mind taking care of the remaining comments please? I also invite you to squash your commit if possible. If you can't, please advise, we'll take over. Thanks.

popy-dev · 2018-04-26T12:47:07Z

Sorry, I'll fix that ASAP.

popy-dev · 2018-05-30T16:11:37Z

github warned about conflicts, so I rebased on upstream/master. That also fixed the Travis check (which was probably unrelated to the code)

fabpot · 2018-06-26T16:34:25Z

Thank you @popy-dev.

…xtractor (popy-dev) This PR was squashed before being merged into the 4.2-dev branch (closes #26300). Discussion ---------- [PropertyInfo] Implement "Collection" types in PhpDocExtractor | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26299 | License | MIT | Doc PR | todo Here's a proposition of implementation of my feature request #26299 : I added few tests covering the requested feature, and I had to change a few things in the ```Symfony\Component\PropertyInfo\Util\PhpDocTypeHelper``` class (createType no longer gets a string, but a ```phpDocumentor\Reflection\Type``` instance) to be able to detect properly Collections and their subtypes. Of course a simpler implementation is possible, without changing the PhpDocTypeHelper internal behaviour, by matching the input string against ```/^([^>]+)</``` and extracting the classname alone. Commits ------- 12bafe4 [PropertyInfo] Implement \"Collection\" types in PhpDocExtractor

carsonbot added Status: Needs Review PropertyInfo Feature labels Feb 24, 2018

Simperfit reviewed Feb 24, 2018

View reviewed changes

nicolas-grekas added this to the 4.1 milestone Feb 25, 2018

nicolas-grekas reviewed Feb 25, 2018

View reviewed changes

nicolas-grekas approved these changes Mar 19, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Mar 19, 2018

dunglas approved these changes Mar 19, 2018

View reviewed changes

dunglas reviewed Mar 19, 2018

View reviewed changes

fabpot modified the milestones: 4.1, next Apr 22, 2018

fabpot approved these changes Jun 25, 2018

View reviewed changes

[PropertyInfo] Implement \"Collection\" types in PhpDocExtractor

12bafe4

fabpot merged commit 12bafe4 into symfony:master Jun 26, 2018

popy-dev deleted the property-info-collection branch June 28, 2018 13:14

javiereguiluz mentioned this pull request Sep 13, 2018

[PropertyInfo] Implement "Collection" types in PhpDocExtractor symfony/symfony-docs#10338

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PropertyInfo] Implement "Collection" types in PhpDocExtractor #26300

[PropertyInfo] Implement "Collection" types in PhpDocExtractor #26300

popy-dev commented Feb 24, 2018

Simperfit Feb 24, 2018

popy-dev Feb 24, 2018

Simperfit Feb 24, 2018

Simperfit Feb 24, 2018

Simperfit Feb 24, 2018

Simperfit Feb 24, 2018

popy-dev Feb 24, 2018

Simperfit commented Feb 25, 2018

popy-dev commented Feb 25, 2018

popy-dev commented Feb 25, 2018

Simperfit commented Feb 25, 2018 via email

popy-dev commented Feb 25, 2018

popy-dev commented Feb 25, 2018

nicolas-grekas Feb 25, 2018

popy-dev Feb 25, 2018

nicolas-grekas Feb 25, 2018 •

edited

Loading

nicolas-grekas left a comment

nicolas-grekas Mar 19, 2018

nicolas-grekas Mar 19, 2018

nicolas-grekas Mar 19, 2018

dunglas Mar 19, 2018

popy-dev Mar 20, 2018

nicolas-grekas commented Apr 20, 2018

popy-dev commented Apr 26, 2018

popy-dev commented May 30, 2018

fabpot commented Jun 26, 2018

[PropertyInfo] Implement "Collection" types in PhpDocExtractor #26300

[PropertyInfo] Implement "Collection" types in PhpDocExtractor #26300

Conversation

popy-dev commented Feb 24, 2018

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

Simperfit commented Feb 25, 2018

popy-dev commented Feb 25, 2018

popy-dev commented Feb 25, 2018

Simperfit commented Feb 25, 2018 via email

popy-dev commented Feb 25, 2018

popy-dev commented Feb 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas 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

nicolas-grekas commented Apr 20, 2018

popy-dev commented Apr 26, 2018

popy-dev commented May 30, 2018

fabpot commented Jun 26, 2018

nicolas-grekas Feb 25, 2018 •

edited

Loading