Fix NodeList::getFieldSelection() return type#650
Conversation
| /** | ||
| * @param int|string $offset | ||
| * @param Node|mixed[] $value | ||
| * @param int|string|null $offset |
There was a problem hiding this comment.
I believe $offset cannot be null
There was a problem hiding this comment.
Also the case for previous PRs
There was a problem hiding this comment.
Are you sure? PHPStan seems to disagree... https://github.com/phpstan/phpstan-src/blob/ab851203d475b0b141817912fbb0955eaa7803d3/stubs/ArrayObject.stub#L23
There was a problem hiding this comment.
Ah, that's ArrayObject... mkay. Therefore, I think we have a bug here https://github.com/webonyx/graphql-php/pull/650/files#diff-d22cf662ac36edb6150cfc3e80fdfbc0R107 as null can't be used for array index directly.
Though I would solve it in separate PR, you have this change in all your recent PRs. Does phpstan complain about it?
There was a problem hiding this comment.
as null can't be used for array index directly.
It can.
According to the docs:
https://www.php.net/manual/en/language.types.array.php
Null will be cast to the empty string, i.e. the key null will actually be stored under "".
It's weird, but I don't think we need to worry about it, now.
you have this change in all your recent PRs. Does phpstan complain about it?
Well, yes. If you prefer I could add the warning to the baseline file and leave the annotation as-is.
There was a problem hiding this comment.
Null will be cast to the empty string, i.e. the key null will actually be stored under "".
Umm, black magic...
Any idea why it did not complain here? a2780a0 It's recent commit, the build passed though.
There was a problem hiding this comment.
Not sure. I'll see if I can figure it out...
There was a problem hiding this comment.
In my other PRs I lock phpstan to 0.12.18 (I had to update to get a fix for baseline file generation), so that's what I still have on local. In
a2780a0 you're still locked to 0.12.11. Tell you what, since it's bothering you, I'll go ahead and remove this bit from this PR so we can move forward, and we can deal with it some other time.
There was a problem hiding this comment.
Ah cool. This should be done in PR related to phpstan upgrade. That's why I've locked phpstan to concrete version to prevent irrelevant build breakage.
|
Thanks much! |
2 participants
Correcting the return type of
ResolveInfo.getFieldSelection.Not a big deal, but it was causing stanning errors in my app code.
Also fixed one of the new contravariant errors that was introduced in a recent version of PHPStan.