Skip to content

PHPStan Level 2#509

Merged
vladar merged 14 commits intowebonyx:masterfrom
spawnia:phpstan-level-up
Aug 2, 2019
Merged

PHPStan Level 2#509
vladar merged 14 commits intowebonyx:masterfrom
spawnia:phpstan-level-up

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.

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.

@spawnia spawnia changed the title Phpstan level up PHPStan Level 2 Jun 23, 2019
- "~(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)

@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