Skip to content

PHPStan level 8#857

Merged
jtojnar merged 55 commits intosimplepie:masterfrom
grimmdude:phpstan_8
Jul 31, 2025
Merged

PHPStan level 8#857
jtojnar merged 55 commits intosimplepie:masterfrom
grimmdude:phpstan_8

Conversation

@grimmdude
Copy link
Contributor

@grimmdude grimmdude commented Jan 10, 2024

  • Increases PHPStan to level 8.

Depends on #892, #890 , #894, #869

@Art4 Art4 mentioned this pull request Jan 10, 2024
7 tasks
@Art4
Copy link
Contributor

Art4 commented Jan 16, 2024

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

$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));
Copy link
Member

Choose a reason for hiding this comment

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

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.

This will be redundant with the PHPStan extension.
@Art4
Copy link
Contributor

Art4 commented Jun 29, 2025

The dependent PRs #892, #890 and #894 are merged.

Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

Still need to review SimplePie and Parser.

jtojnar added 5 commits July 27, 2025 20:18
; Conflicts:
;	src/HTTP/FileClient.php
We do not use the $contextNode argument so that is really the only failure mode according to docs:
https://www.php.net/manual/en/domxpath.query.php

And these methods are internal but it is probably safer to throw an exception since the values might come from user config.
@jtojnar jtojnar mentioned this pull request Jul 27, 2025
Since we switched from `File::$success` property to `File::$error !== null` to have `FileClient` detect failure, we need to inject the value to tests. Otherwise, PHPStan would not be able to detect if the `File::$error` property is set and thus will complain about it potentially being none when passing it to `ClientException` constructor.
Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

Thank you very much. With the latest changes, I consider the PR good enough to merge.

Not much option other than squashing everything together.

@jtojnar jtojnar removed their assignment Jul 27, 2025
@jtojnar jtojnar requested a review from Art4 July 27, 2025 21:37
@jtojnar jtojnar marked this pull request as ready for review July 27, 2025 21:38
@@ -166,9 +166,9 @@ public function pass_cache_data(bool $enable_cache = true, string $cache_locatio
}

// BC: $cache_name_function could be a callable as string
Copy link
Contributor

@Art4 Art4 Jul 28, 2025

Choose a reason for hiding this comment

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

$cache_name_function was only allowed as string since 1.3 or longer, see https://github.com/simplepie/simplepie/blob/1.3/library/SimplePie/Sanitize.php

We switched to NameFilter in 1.8.0, but allowing string for BC.

Allowingcallable|NameFilter isntead of string|NameFilter would not be a BC break, but we should not widen the potential deprecated string type without a good reason.

@grimmdude
Copy link
Contributor Author

Thanks for the assists, @jtojnar and @Art4. Will be nice to close this one out.

; Conflicts:
;	src/File.php
;	src/Locator.php
;	src/Misc.php
;	src/Sanitize.php
;	src/SimplePie.php
@jtojnar jtojnar requested a review from Art4 July 29, 2025 22:15
Copy link
Contributor

@Art4 Art4 left a comment

Choose a reason for hiding this comment

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

Just a last small fix.

Co-authored-by: Artur Weigandt <[email protected]>
@jtojnar jtojnar merged commit 1374b15 into simplepie:master Jul 31, 2025
10 checks passed
Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Aug 1, 2025
Alkarex added a commit to FreshRSS/FreshRSS that referenced this pull request Aug 1, 2025
* Bump SimplePie with PHPStan Level 8
* FreshRSS/simplepie#45
SimplePie increased to PHPStan Level 8:
* simplepie/simplepie#857

* Merge upstream
Including my two PRs:
* simplepie/simplepie#932
* simplepie/simplepie#933

* Resolve upstream sync of Expose HTTP status
* FreshRSS/simplepie#47
Finalise merge, following:
* simplepie/simplepie#905 (comment)
* simplepie/simplepie#909
* #7038
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants