PHPStan level 8 #857
PHPStan level 8 #857
Conversation
|
Please note that we have to support PHP 7.2, so mixed and Union types are not supported there... You should keep a look at your GitHub Action here: https://github.com/grimmdude/simplepie/actions/runs/7536994157/job/20515243462 |
| @@ -90,11 +90,11 @@ public function test_get_relationship(string $data, string $expected): void | |||
|
|
|||
| if ($enclosure = $item->get_enclosure()) { | |||
| $this->assertInstanceOf(Enclosure::class, $enclosure); | |||
There was a problem hiding this comment.
Weird, this should already ensure $enclosure is !== null.
There was a problem hiding this comment.
Here's the error that this change fixes:
------ -------------------------------------------------------------------
Line tests/Unit/RestrictionTest.php
------ -------------------------------------------------------------------
95 Cannot call method get_restriction() on SimplePie\Enclosure|null.
------ -------------------------------------------------------------------
Problem was that $restriction = $enclosure->get_restriction(); was originally outside of this conditional.
| @@ -199,7 +202,8 @@ public static function absolutizeDataProvider(): array | |||
| public function testAbsolutizeString(string $base, string $relative, string $expected): void | |||
| { | |||
| $base = new IRI($base); | |||
| $this->assertSame($expected, IRI::absolutize($base, $relative)->get_iri()); | |||
| $absolutized = IRI::absolutize($base, $relative); | |||
| $this->assertSame($expected, ($absolutized ? $absolutized->get_iri() : null)); | |||
There was a problem hiding this comment.
These tests all assume $absolutized !== null so it might be more explicit to turn the assumption to a separate asserting. But maybe it does not make much difference to bother.
| } | ||
|
|
||
| $element->parentNode->replaceChild($fragment, $element); | ||
| if ($parentNode = $element->parentNode) { |
There was a problem hiding this comment.
I think it is generally preferred to avoid implicit conversions of truthy/falsy values and only allow bool expressions in conditionals:
| if ($parentNode = $element->parentNode) { | |
| if (($parentNode = $element->parentNode) !== null) { |
Though currently we do not enforce this, it would require PHPStan strict rules.
src/Parser.php
Outdated
| @@ -190,7 +190,7 @@ public function parse(string &$data, string $encoding, string $url = '') | |||
| } else { | |||
| $attrName = $xml->localName; | |||
| } | |||
| $attributes[$attrName] = $xml->value; | |||
| $attributes[(string) $attrName] = (string) $xml->value; | |||
There was a problem hiding this comment.
What error causes this cast? Neither localName or value are supposed to be anything but string https://www.php.net/manual/en/class.xmlreader.php
There was a problem hiding this comment.
@jtojnar, below is the error that casting these to string resolves.
------ --------------------------------------------------------------------------------------------------------------------------------
Line src/Parser.php
------ --------------------------------------------------------------------------------------------------------------------------------
195 Parameter #3 $attributes of method SimplePie\Parser::tag_open() expects array<string, string>, array<int|string, mixed> given.
------ --------------------------------------------------------------------------------------------------------------------------------
There was a problem hiding this comment.
I've just pushed bb89c21 which moves this fix more upstream by only casting localName to string. This also seems to fix this error 🤷♂️
Work in progress, 4 errors remaining...
phpdoc_to_param_typePHPCS rule since union types aren't supported in PHP 7.2 (ci: Fix php-cs-fixer #862).#850, #856