Fixes for PHPStan level 5 #282
Conversation
| @@ -41,7 +41,7 @@ | |||
|
|
|||
| "autoload-dev": { | |||
| "psr-4": { | |||
| "Tests\\Behat\\": "tests/Behat/" | |||
| "Tests\\": "tests/" | |||
There was a problem hiding this comment.
This helps:
- IDEs (e.g. PhpStorm or IDEA) correctly detect the tests directory as the "tests root"
- It decrease the wtf factor if someone adds test classes elsewhere which wouldn't load otherwise
- It may not be the best, performance-wise, but I think it's not so critical seeing as it only impacts tests (i.e.
composer --dev).
| protected $filterLine; | ||
|
|
||
| /** | ||
| * Initializes filter. | ||
| * | ||
| * @param string $filterLine Line of the scenario to filter on | ||
| * @param int|numeric-string $filterLine Line of the scenario to filter on |
There was a problem hiding this comment.
Passing a non-numeric-string is Doing It Wrong and deserves a PHPStan slap. ;)
| @@ -83,7 +85,7 @@ public function parse($input, $file = null) | |||
| while ('EOS' !== ($predicted = $this->predictTokenType())) { | |||
| $node = $this->parseExpression(); | |||
|
|
|||
| if ($node === null || $node === "\n") { | |||
There was a problem hiding this comment.
I checked and couldn't find evidence of any parser method returning a null (in addition, the PHPDocs never mentioned nulls either, hence the PHPStan warning).
| @@ -262,14 +264,12 @@ protected function parseFeature() | |||
| )); | |||
| } | |||
|
|
|||
| if (!$node instanceof ScenarioNode) { | |||
There was a problem hiding this comment.
This condition and the other cases below were flagged as impossible by PHPStan because:
- there is already an earlier branch unconditionally handling such instance
$nodeis never overwritten
| * @param string $filterMinLine Minimum line of a scenario to filter on | ||
| * @param string $filterMaxLine Maximum line of a scenario to filter on | ||
| * @param int|numeric-string $filterMinLine Minimum line of a scenario to filter on | ||
| * @param int|numeric-string|'*' $filterMaxLine Maximum line of a scenario to filter on |
There was a problem hiding this comment.
I verified that PHPStan handles this well: https://phpstan.org/r/bd2527e3-519d-491e-9eb6-7208761748dd
| @@ -31,6 +31,8 @@ | |||
| * $featuresArray = $parser->parse('/path/to/feature.feature'); | |||
| * | |||
| * @author Konstantin Kudryashov <[email protected]> | |||
| * | |||
| * @phpstan-type TParsedExpressionResult FeatureNode|BackgroundNode|ScenarioNode|OutlineNode|ExampleTableNode|TableNode|PyStringNode|StepNode|string|"\n"|'' | |||
There was a problem hiding this comment.
I verified that "\n" is the right way to go: https://phpstan.org/r/8283cfec-bff9-4375-be68-a046ec433b47
Note: even though string includes '' and "\n" I decided to mention them separately nevertheless.. hopefully it would be helpful.
tests/Behat/Gherkin/ParserTest.php
Outdated
| { | ||
| if (!extension_loaded('xdebug')) { | ||
| $this->markTestSkipped('xdebug extension must be enabled.'); | ||
| } | ||
| $defaultPHPSetting = 256; | ||
| $this->iniSet('xdebug.max_nesting_level', $defaultPHPSetting); | ||
| ini_set('xdebug.max_nesting_level', $defaultPHPSetting); |
There was a problem hiding this comment.
$this->iniSet was resetting the state at the end of the test. This is not done anymore.
There was a problem hiding this comment.
@stof I'm not sure what you meant. Did you mean that is dead/redundant code?
Note that that method has been deprecated. Internally, it seems it "just" calls ini_set (and updates PHPUnit's ini state).
Since this concerns tests, and tests seem to be running fine, I think this may be a non-issue?
There was a problem hiding this comment.
Hmm, I see that the GH workflow doesn't seem to run with xdebug, I'll try it later on at home and see what happens (since this seems to relate exclusively to xdebug).
PS: We should probably integrate codecov into this project. 🤔
There was a problem hiding this comment.
@uuf6429 this "updating PHPUnit's ini state" is precisely the part that will restore them on teardown
There was a problem hiding this comment.
Ah, I see what you mean now, something like:
ini_set(<new value>);
try {
<the test>
} finally {
ini_set(<old value>);
}I'll fix it later on today, thanks for pointing it out.
No description provided.