PHPStan Level 2 #509
PHPStan Level 2 #509
Conversation
| @@ -39,6 +39,9 @@ abstract class Node | |||
| /** @var Location */ | |||
| public $loc; | |||
|
|
|||
| /** @var string */ | |||
| public $kind; | |||
There was a problem hiding this comment.
This will be very useful for other libraries using graphql-php, see https://github.com/rebing/graphql-laravel/blob/a458a79f7bad6406f181ac4d1ab371d65aa12c7f/phpstan.neon#L37
| @@ -10,6 +10,13 @@ parameters: | |||
| - "~(Method|Property) .+::.+(\\(\\))? (has parameter \\$\\w+ with no|has no return|has no) typehint specified~" | |||
| - "~Variable property access on .+~" | |||
| - "~Variable method call on static\\(GraphQL\\\\Server\\\\ServerConfig\\)~" # TODO get rid of | |||
| - "~Only booleans are allowed in .*~" # TODO https://github.com/phpstan/phpstan-strict-rules/issues/2 | |||
There was a problem hiding this comment.
I'm working on this in small PRs just FYI (eg. https://github.com/webonyx/graphql-php/pull/503/files)
|
@spawnia I think I am ready to merge this, but can you resolve conflicts, please? |
# Conflicts: # src/Executor/ReferenceExecutor.php
|
I actually went ahead and "expanded" the union types into the PHPDocs. This works out to satisfy PHPStan. While a bit tedious, i don't see the GraphQL type system changing much in the near future, so in terms of maintenance, this seems fine. |
|
Merging this broke the build (I assume after #501). Can you take a look? |
|
Damn, you're fast %) Thanks! |
|
Thanks guys :) |
I added in the simple fixes and added an exception/TODO for the strict rule that forbids loose boolean comparisons.
Discussion
A whole class of errors in PHPStan is a result of PHP's lack of union types.
This commonly happens in the parts of the code that deal with the GraphQL
type system where we can currently use interfaces and lose type safety.
There are about ~30 individual errors caused by that. Given that we want to keep leveling up PHPStan to reap the benefits of the higher static analysis levels, there are a few ways of going forward.
The simples workaround would be to add the individual errors we know to be false positives resulting from the lacking union types to the PHPStan config
ignoreErrorssection in a dedicated block.Another workaround which would maybe be a bit cleaner would be to add comments to tell PHPStan to ignore the following line, this is not yet possible but we might make it so phpstan/phpstan#786
Finally, we could think about ways to actually cleanly type this thing. This might require breaking changes of this library.