Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PropertyInfo] fix array types with keys (array<string, string>) #37559

Merged
merged 1 commit into from Sep 9, 2020

Conversation

digilist
Copy link
Contributor

@digilist digilist commented Jul 11, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets n/a
License MIT
Doc PR n/a

Array types with keys are currently not detected correctly from the PropertyInfo component:

@var array<string, string>
@var array<string, array<integer, string>>

Those are currently identified as object with class rray<string, string>.

This PR tries to fix it.

What I noticed while fixing this, is that union types in arrays are not supported in general at the moment, because the Type class supports only one collectionValueType and I do not see how to pass a Union Type there. But I guess that's a different issue and for those types I decided to return null as collection value type for now. (Or better throw on exception?)

@digilist digilist requested a review from dunglas as a code owner July 11, 2020 14:14
@digilist digilist changed the base branch from master to 4.4 July 11, 2020 14:14
@@ -109,19 +111,30 @@ private function createType(DocType $type, bool $nullable, string $docType = nul
}

if ('[]' === substr($docType, -2)) {
if ('mixed[]' === $docType) {
$collectionKeyType = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this code was never executed. in case of a mixed[] type on the variable, $docType was array, so I removed this code.

@digilist
Copy link
Contributor Author

digilist commented Jul 11, 2020

I just noticed that the build is failing for older PHP versions, please let me check first.

Update: Fixed it. It was because older versions of the phpdocumenter reflection package did not allow/parse spaces in array<string, string> The appveyor and travis builds are still red, but those builds are failing because of unrelated tests.

@digilist digilist force-pushed the property_info_array_keys branch 2 times, most recently from 77974e2 to 93882ea Compare July 11, 2020 17:39
@nicolas-grekas nicolas-grekas added this to the next milestone Jul 12, 2020
@nicolas-grekas
Copy link
Member

We would consider this as a new feature indeed. Can you rebase and target master please?

@MaSpeng
Copy link

MaSpeng commented Aug 20, 2020

How is the status of this merge request, I stumbled over this issue by myself right now?

@nicolas-grekas
Copy link
Member

/cc @dunglas could you maybe have a look?

@fabpot
Copy link
Member

fabpot commented Sep 9, 2020

Thank you @digilist.

@ghost
Copy link

ghost commented Sep 16, 2020

Hello, is there a known workaround to wait to next property-info patch release?

@pl-github
Copy link
Contributor

Hello, is there a known workaround to wait to next property-info patch release?

We used object<string, MyOwnClass> instead of array<string, MyOwnClass> in our ApiPlatform project as a workaround. I know it is very crappy and phpstan does not like it either, but it is the easiest way to bridge the time until the next minor release.

@Marmelatze
Copy link

You can use something like this:

@var MyOwnClass[]
@phpstan-var array<string, MyOwnClass>

That should satisfy phpstan and the serializer.

@pl-github
Copy link
Contributor

pl-github commented Sep 22, 2020

@Marmelatze Thank you for your suggestion.

The phpstan annotation works, but the normal @var annotation does not work, because the unfixed PropertyInfo version enforces arrays with a numeric index and the ArrayDenormalizer fails.

@aso824
Copy link

aso824 commented Sep 26, 2020

You could use @var aarray<string, MyOwnClass> which is also ugly hack but works with ArrayDenormalizer.

@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
@mdeboer
Copy link
Contributor

mdeboer commented Oct 6, 2020

You could use @var aarray<string, MyOwnClass> which is also ugly hack but works with ArrayDenormalizer.

Thank you, this worked perfectly! Looking forward to the 5.2 release though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet