[PropertyInfo] Allow nested collections #28012

jderusse · 2018-07-20T09:36:23Z

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	NA
License	MIT
Doc PR	NA

When a multidimentional collection is defined (in a docblock) the extractor does not resolve the className deeply

#input
class Foo {
  /**
   * @var Baz[][]
   */
  public $bar;
}

# current result
builtinType: array
collectionValueType: 
  builtinType: object
  class: Baz[]

# FIX
builtinType: array
collectionValueType: 
  builtinType: array
  collectionValueType: 
    builtinType: object
    class: Baz

The 2.8 version has also that bug, but the methods have been moved to another class. Should I create an other PR for 2.8?

ostrolucky · 2018-07-21T14:11:31Z

There is another change you left out

For:

/** @var mixed[][] */

before:

builtintype: array
collectionKeyType: null
collectionValueType: null

after:

builtintype: array
collectionKeyType:
  builtintype: int
collectionValueType: 
  builtintype: array

jderusse · 2018-07-21T18:39:59Z

Hi @ostrolucky ,

I agreed, that I didn't handle the mixed[] the same way than before, but I think the new return is

builtintype: array
collectionKeyType:
  builtintype: int
collectionValueType: null

(because of line 92) can you please double check?

Which led to a different behavior for the field collectionKeyType because, this is what I would expect. And I though it was a bug too: Why int[], bool[], object[] will not return the same collectionKeyType than mixed[]?
Should I consider it has a BC break?

ostrolucky · 2018-07-21T19:14:34Z

I double checked. Please write unit test for this.

nicolas-grekas

(for 2.8, PR welcome if the bug exists there also I suppose)

javiereguiluz · 2018-07-23T18:48:14Z

src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php

@@ -38,6 +38,7 @@ public function testGetProperties()
                'bal',
                'parent',
                'collection',
+                'deepCollection',


A very minor comment: is there any reason for calling this deepCollection instead of nestedCollection? The nested one sounds more natural to me. Thanks!

ostrolucky · 2018-07-23T19:58:59Z

mixed[][] behavior not clarified. What's the consensus? Seems there is different behavior currently than expected by author

ostrolucky · 2018-07-23T21:24:56Z

Also, not sure it's right to always use int for keyType, as there is no way to specify via docblock otherwise

jderusse · 2018-07-24T15:17:57Z

I reverted changes on mixed[] as PhpDocumentor cast mixed[] to generic array => https://github.com/phpDocumentor/TypeResolver/blob/0.4.0/src/Types/Array_.php#L80-L82.

now mixed[] will return (as before)

builtintype: array
collectionKeyType: null
collectionValueType: null

This PR was merged into the 2.8 branch. Discussion ---------- [PropertyInfo] Allow nested collections | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Duplicate of #28012 for the 2.8 branche (as both code and test have been refactored between 2.8 and 3.x Commits ------- 6331687 Allow multidimensional collection in property info

fabpot · 2018-08-02T09:26:20Z

Thank you @jderusse.

@var

This PR was merged into the 3.4 branch. Discussion ---------- [PropertyInfo] Allow nested collections | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | NA | License | MIT | Doc PR | NA When a multidimentional collection is defined (in a docblock) the extractor does not resolve the className deeply ``` #input class Foo { /** * @var Baz[][] */ public $bar; } ``` ``` # current result builtinType: array collectionValueType: builtinType: object class: Baz[] ``` ``` # FIX builtinType: array collectionValueType: builtinType: array collectionValueType: builtinType: object class: Baz ``` The 2.8 version has also that bug, but the methods have been moved to another class. Should I create an other PR for 2.8? Commits ------- ce49036 Allow multidimensional collection in property info

nicolas-grekas · 2018-08-03T08:58:29Z

@jderusse or anyone else: PropertyInfo 3.4 is red currently, could you have a look please?

jderusse · 2018-08-03T09:06:10Z

cheking it

jderusse · 2018-08-03T09:09:22Z

@nicolas-grekas shouldn't it be fixed by fe482cc ?

nicolas-grekas · 2018-08-03T09:12:08Z

Looks like so. It wasn't passing locally, but I suppose that's because I didn't run composer update before... Sorry for the false alarm.

nicolas-grekas · 2018-08-03T12:35:43Z

When merging into master, I added this commit to make tests pass: a5516fc

Please check and submit a PR if the patch should be improved.

carsonbot added Status: Needs Review Bug labels Jul 20, 2018

nicolas-grekas changed the title ~~Allow multidimensional collection in property info~~ [PropertyInfo] Allow multidimensional collection Jul 23, 2018

nicolas-grekas added this to the 3.4 milestone Jul 23, 2018

nicolas-grekas approved these changes Jul 23, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jul 23, 2018

javiereguiluz reviewed Jul 23, 2018

View reviewed changes

jderusse force-pushed the fix-deep-array branch from e693459 to 597f5c8 Compare July 24, 2018 15:13

jderusse force-pushed the fix-deep-array branch from 597f5c8 to bdb2b21 Compare July 24, 2018 15:29

jderusse mentioned this pull request Jul 24, 2018

[PropertyInfo] Allow nested collections #28055

Merged

jderusse changed the title ~~[PropertyInfo] Allow multidimensional collection~~ [PropertyInfo] Allow nested collections Jul 24, 2018

nicolas-grekas requested a review from dunglas July 25, 2018 08:22

carsonbot added Status: Needs Review and removed Status: Reviewed labels Jul 25, 2018

Allow multidimensional collection in property info

ce49036

jderusse force-pushed the fix-deep-array branch from bdb2b21 to ce49036 Compare July 25, 2018 10:57

fabpot merged commit ce49036 into symfony:3.4 Aug 2, 2018

This was referenced Aug 28, 2018

Release v3.4.15 #28290

Merged

Release v4.1.4 #28291

Merged

jderusse deleted the fix-deep-array branch August 2, 2019 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PropertyInfo] Allow nested collections #28012

[PropertyInfo] Allow nested collections #28012

jderusse commented Jul 20, 2018

ostrolucky commented Jul 21, 2018 •

edited

Loading

jderusse commented Jul 21, 2018 •

edited by lyrixx

Loading

ostrolucky commented Jul 21, 2018 •

edited

Loading

nicolas-grekas left a comment

javiereguiluz Jul 23, 2018

jderusse Jul 24, 2018

ostrolucky commented Jul 23, 2018 •

edited

Loading

ostrolucky commented Jul 23, 2018

jderusse commented Jul 24, 2018 •

edited

Loading

fabpot commented Aug 2, 2018

nicolas-grekas commented Aug 3, 2018

jderusse commented Aug 3, 2018

jderusse commented Aug 3, 2018

nicolas-grekas commented Aug 3, 2018 •

edited

Loading

nicolas-grekas commented Aug 3, 2018

[PropertyInfo] Allow nested collections #28012

[PropertyInfo] Allow nested collections #28012

Conversation

jderusse commented Jul 20, 2018

ostrolucky commented Jul 21, 2018 • edited Loading

jderusse commented Jul 21, 2018 • edited by lyrixx Loading

ostrolucky commented Jul 21, 2018 • edited Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

javiereguiluz Jul 23, 2018

Choose a reason for hiding this comment

jderusse Jul 24, 2018

Choose a reason for hiding this comment

ostrolucky commented Jul 23, 2018 • edited Loading

ostrolucky commented Jul 23, 2018

jderusse commented Jul 24, 2018 • edited Loading

fabpot commented Aug 2, 2018

nicolas-grekas commented Aug 3, 2018

jderusse commented Aug 3, 2018

jderusse commented Aug 3, 2018

nicolas-grekas commented Aug 3, 2018 • edited Loading

nicolas-grekas commented Aug 3, 2018

ostrolucky commented Jul 21, 2018 •

edited

Loading

jderusse commented Jul 21, 2018 •

edited by lyrixx

Loading

ostrolucky commented Jul 21, 2018 •

edited

Loading

ostrolucky commented Jul 23, 2018 •

edited

Loading

jderusse commented Jul 24, 2018 •

edited

Loading

nicolas-grekas commented Aug 3, 2018 •

edited

Loading