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

Improve EventSubscriber return type docblock #30131

Closed
wants to merge 1 commit into
base: 3.4
from

Conversation

@ostrolucky
Copy link
Contributor

ostrolucky commented Feb 10, 2019

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

@xabbuh xabbuh added this to the 3.4 milestone Feb 10, 2019

@vudaltsov

This comment has been minimized.

Copy link
Contributor

vudaltsov commented Feb 10, 2019

When this might be helpful?
I mean, is it really an improvement?

@ostrolucky

This comment has been minimized.

Copy link
Contributor Author

ostrolucky commented Feb 10, 2019

It provides type information which is useful for static analysis.

@@ -40,7 +40,7 @@ interface EventSubscriberInterface
* * ['eventName' => ['methodName', $priority]]
* * ['eventName' => [['methodName1', $priority], ['methodName2']]]
*
* @return array The event names to listen to
* @return string[]|string[][]|int[][] The event names to listen to

This comment has been minimized.

@fabpot

fabpot Feb 11, 2019

Member

I'm not an expert in phpdocs, but it looks wrong to me. Something like map[string]string describes the first use case, and then it gets more complex :)

This comment has been minimized.

@ostrolucky

ostrolucky Feb 11, 2019

Author Contributor

Maybe you are confused because you try to model keys too? Official PHPDoc have no way to document these, it's values only.

So first use case is string[] because it's array of string values.
Second use case is string[][]|int[][] because it's array of arrays with string or int values.

This comment has been minimized.

@fabpot

fabpot Feb 12, 2019

Member

I fail to see how this is useful then. As you can have a mix of strings and ints, shouldn't it be something like (string|int)[][]?

This comment has been minimized.

@ostrolucky

ostrolucky Feb 12, 2019

Author Contributor

(string|int)[][] and string[][]|int[][] is identical (except latter is more standard and has better support). It's always union, there is no support for specifying array must contain both

This comment has been minimized.

@fabpot

fabpot Feb 12, 2019

Member

Are you sure they are identical? We are already using the first form in Symfony.

This comment has been minimized.

@muglug

muglug Feb 12, 2019

In other words, what exact problem this change solves?

Psalm complains on its strictest setting when it can't infer a return type: https://getpsalm.org/r/4fe8fcdbef

If it were me I'd probably use a stub file with a much more specific return type (supported by both Psalm and Phan, but not currently PHPStan):
array<string, string|array{string, int}|array<int, array{string,int}>>
(example in use here: https://getpsalm.org/r/a70f39dc6a)

this informs the consumer that it either returns an array of strings, an array of {string, int} tuple arrays, or an array of arrays of {string, int} tuples, all keyed by a string.

This comment has been minimized.

@ostrolucky

ostrolucky Feb 12, 2019

Author Contributor

New type gives almost no additional information about the return type.

Sure it does. SA tool can infer it's an error having int[] or having something else than integers or strings in an array, or when it's more dimensional than that.

what exact problem this change solves?

There are enforced style guides in projects which dictate every array must specify type of its members. By implementing interface without such docblock, IDE won't create docblock matching such requirements. This change is simple way how to solve this problem and additionally have more type safety thanks to SA tools.

Anyway, this return typehint string[]|string[][]|int[][] is already pretty complex

Yes, but it matches how this method works. It's not a fault of docblock that method accepts complex types, it reflects it.

I'd probably use a stub file

That would be fine when you are tied to Psalm, but there is zero support for such syntax outside of it. That's why I wrote there is no support for specifying array must contain both types. PHPDocumentor doesn't have such thing and that's the only official thing we have, so as far as I see what I used here is best we got now.

This comment has been minimized.

@stof

stof Feb 12, 2019

Member

Given that string[]|string[][]|int[][] is complex to read while still not accurately describing the return type (well, actually describing a wrong one), I don't think it make sense to use it.

Until we have a common agreement on the way to document a type like that (maybe the one Psalm and Phan support today, which indeed describes the right type), I don't think it makes sense to change the phpdoc in this interface.

This comment has been minimized.

@vudaltsov

vudaltsov Feb 12, 2019

Contributor

The first form ((string|int)[]) - each array item can be either a string or an int
The second form (string[]|int[]) - It's either an array of strings or an array of integers.

@ostrolucky , okay, then based on @ondrejmirtes comment we should pick (string|int)[][] as @fabpot proposed. It's true that each item of the subarray is either a string or an int, but it's false that method ever returns an array of arrays of ints.

This comment has been minimized.

@jvasseur

jvasseur Feb 12, 2019

Contributor

If I'm not mistaken, the correct type would be (string|(string|int)[]|(string|int)[][])[] which is arguably hard to read.

@ostrolucky ostrolucky closed this Feb 12, 2019

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