[WIP] Use PHPStan strict rules #18
Conversation
| } | ||
|
|
||
| $this->filesystem->dumpFile("{$this->path}/{$id}/{$version->toInt()}.xml", $item->getContent()); | ||
| $contents = stream_get_contents($item->getContent()); |
There was a problem hiding this comment.
Caused by symfony/symfony#29661.
| } | ||
|
|
||
| if (!$content = @fopen($file = "{$this->path}/{$id}/{$version->toInt()}.xml", 'rb')) { | ||
| $content = @fopen($file = "{$this->path}/{$id}/{$version->toInt()}.xml", 'rb'); |
There was a problem hiding this comment.
the @ operator is really what sticks out in this line more than the condition/assignment mixing
|
|
||
| /** | ||
| * @param mixed $resource | ||
| * @param ?string $type |
There was a problem hiding this comment.
?string cannot be put directly into the signature because it's inherited I guess
There was a problem hiding this comment.
Yep. (Should be string|null here too.)
There was a problem hiding this comment.
Though the parent class defines these so shouldn't be needed?
There was a problem hiding this comment.
Reading phpstan/phpstan-strict-rules#51 (comment) {@inheritdoc} is required, but isn't in the next version (just tried and confirmed).
There was a problem hiding this comment.
{@inheritdoc} works for ResourceComparator/TraversableComparator but not Loader? 🤔
| } | ||
|
|
||
| /** | ||
| * @param mixed $resource |
There was a problem hiding this comment.
the purpose seems to be "declare all types"? But doesn't seem to add information as $resource can be any type?
There was a problem hiding this comment.
Like #18 (comment), this is on the parent so shouldn't be needed.
| ignoreErrors: | ||
| - '~^.*(?=.* Symfony\\Component\\Config\\Definition\\Builder)(?=.*( |:{2})end\(\)).*$~' | ||
| - '~^Call to method PHPUnit\\Framework\\Assert::assertInstanceOf\(\) with .+? and .+? will always evaluate to true\.$~' | ||
| - '~^Dynamic call to static method PHPUnit\\Framework\\Assert::assert[A-z]+\(\)\.$~' |
There was a problem hiding this comment.
surely there must be a way to disable/enable rules selectively like https://github.com/phpstan/phpstan-strict-rules#enabling-rules-one-by-one?
There was a problem hiding this comment.
My summary is:
- code becoming more verbose due to strict checks like booleans for all conditions
- docblocks with types mandatory for practically everything (even some local variables, no type inference?)
- indirections over standard library functions
All of these add a steep learning curve for contributing, but unclear what bugs they are finding. Other opinions?
|
Looks like theres a lot of good bug-catching rules in the strict ruleset, could enable a subset of them. In my opinion the volume of doc-block looks very PHP 5.4. For simple structures that have a typed constructor, it seems unnecessary. The strict boolean in conditionals also may be better as a warning, where some cases it may make sense to loosely check for truth-y values. In a perfect world PHP wouldn't return a mixture of |
Relates to libero/php-coding-standard#32 (and libero/php-coding-standard#40). I'm undecided still. I haven't really done it in the past as IDEs/my eyes are generally quite good at type inference. So is PHPStan, but there are cases where it doesn't work. It also helps validate assignment. PHP 7.4 will hopefully have typed properties too. |
3 participants
Investigating whether the required changes are all worthwhile.