NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[PropertyInfo] Allow nested collections#28012

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:3.4fromjderusse:fix-deep-array

Aug 2, 2018

Merged

[PropertyInfo] Allow nested collections#28012

fabpot merged 1 commit intosymfony:3.4fromjderusse:fix-deep-array

Aug 2, 2018

Conversation

Copy link

Member

jderusse commentedJul 20, 2018

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

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

# current resultbuiltinType: arraycollectionValueType:   builtinType: object  class: Baz[]

# FIXbuiltinType: arraycollectionValueType:   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?

carsonbot added Status: Needs Review Bug labels

Jul 20, 2018

Copy link

Contributor

ostrolucky commentedJul 21, 2018•
edited
Loading

There is another change you left out

For:

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

before:

builtintype:arraycollectionKeyType:nullcollectionValueType:null

after:

builtintype:arraycollectionKeyType:builtintype:intcollectionValueType:builtintype:array

Copy link

MemberAuthor

jderusse commentedJul 21, 2018•
edited by lyrixx
Loading

Hi@ostrolucky ,

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

builtintype: arraycollectionKeyType:  builtintype: intcollectionValueType: null

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

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

Copy link

Contributor

ostrolucky commentedJul 21, 2018•
edited
Loading

I double checked. Please write unit test for this.

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

Jul 23, 2018

nicolas-grekas added this to the3.4 milestone

Jul 23, 2018

nicolas-grekas approved these changes

Jul 23, 2018

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Jul 23, 2018

javiereguiluz reviewed

Jul 23, 2018

View reviewed changes

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

		'bal',
		'parent',
		'collection',
		'deepCollection',

Copy link

Member

javiereguiluzJul 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

jderusseJul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

fixed

Copy link

Contributor

ostrolucky commentedJul 23, 2018•
edited
Loading

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

Copy link

Contributor

ostrolucky commentedJul 23, 2018

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

jderusse force-pushed thefix-deep-array branch frome693459 to597f5c8Compare

July 24, 2018 15:13

Copy link

MemberAuthor

jderusse commentedJul 24, 2018•
edited
Loading

I reverted changes onmixed[] as PhpDocumentor castmixed[] to genericarray =>https://github.com/phpDocumentor/TypeResolver/blob/0.4.0/src/Types/Array_.php#L80-L82.

nowmixed[] will return (as before)

builtintype: arraycollectionKeyType: nullcollectionValueType: null

jderusse force-pushed thefix-deep-array branch from597f5c8 tobdb2b21Compare

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 fromdunglas

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 thefix-deep-array branch frombdb2b21 toce49036Compare

July 25, 2018 10:57

fabpot added a commit that referenced this pull request

Aug 2, 2018

bug#28055[PropertyInfo] Allow nested collections (jderusse)

470ac26

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.xCommits-------6331687 Allow multidimensional collection in property info

Copy link

Member

fabpot commentedAug 2, 2018

Thank you@jderusse.

fabpot merged commitce49036 intosymfony:3.4

Aug 2, 2018

fabpot added a commit that referenced this pull request

Aug 2, 2018

bug#28012[PropertyInfo] Allow nested collections (jderusse)

5cf1bef

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        | NAWhen a multidimentional collection is defined (in a docblock) the extractor does not resolve the className deeply```#inputclass Foo {  /**   *@var Baz[][]   */  public $bar;}``````# current resultbuiltinType: arraycollectionValueType:  builtinType: object  class: Baz[]``````# FIXbuiltinType: arraycollectionValueType:  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