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

PHPStan Level 2 #509

Merged
merged 14 commits into from
Aug 2, 2019
Merged

PHPStan Level 2 #509

merged 14 commits into from
Aug 2, 2019

Conversation

spawnia
Copy link
Collaborator

@spawnia spawnia commented Jun 23, 2019

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 ignoreErrors section 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.

@@ -39,6 +39,9 @@ abstract class Node
/** @var Location */
public $loc;

/** @var string */
public $kind;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be very useful for other libraries using graphql-php, see https://github.com/rebing/graphql-laravel/blob/a458a79f7bad6406f181ac4d1ab371d65aa12c7f/phpstan.neon#L37

@spawnia spawnia changed the title Phpstan level up PHPStan Level 2 Jun 23, 2019
@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm working on this in small PRs just FYI (eg. https://github.com/webonyx/graphql-php/pull/503/files)

tests/Language/ParserTest.php Outdated Show resolved Hide resolved
tests/Type/DefinitionTest.php Outdated Show resolved Hide resolved
@vladar
Copy link
Member

vladar commented Jul 12, 2019

@spawnia I think I am ready to merge this, but can you resolve conflicts, please?

@spawnia
Copy link
Collaborator Author

spawnia commented Jul 16, 2019

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.

@vladar vladar merged commit e804ccf into webonyx:master Aug 2, 2019
@vladar
Copy link
Member

vladar commented Aug 2, 2019

Merging this broke the build (I assume after #501). Can you take a look?

@simPod
Copy link
Collaborator

simPod commented Aug 2, 2019

@vladar #526

@vladar
Copy link
Member

vladar commented Aug 2, 2019

Damn, you're fast %) Thanks!

@spawnia
Copy link
Collaborator Author

spawnia commented Aug 6, 2019

Thanks guys :)

@spawnia spawnia deleted the phpstan-level-up branch August 6, 2019 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants