-
-
Notifications
You must be signed in to change notification settings - Fork 862
[1.x] Fix running phpstan with newer PHP versions (e.g. 8.4) #4338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 1.x
Are you sure you want to change the base?
Conversation
imorland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this @n-peugnet. I've not ran this locally yet, but yes I think this will probably work out!
Can you please rebase this, and target the 1.x branch 🙏
phpstan 1.8 cannot run properly with PHP 8.4, but newer versions in the
1.x branch do, thus we change the requirement to phpstan ^1.9, to allow
version up to 2.0 (excluding).
Newer phpstan versions now detect common errors in regexes:
Ignored error #Template type T of function resolve[()]{2} is not referenced in a parameter.# has an unescaped '()' which leads to ignoring all errors. Use '\(\)' instead.
Ignored error #^Unsafe usage of new static[()]{2}.$# has an unescaped '()' which leads to ignoring all errors. Use '\(\)' instead.
So these regexes are also fixed by using non-evaluated string litterals
and escaping the parenthesis as recommended instead of using a weird
trick with the brackets.
74dee0d to
3a16c74
Compare
|
@imorland: just rebased, sorry I didn't find this |
|
Thank you for this PR @n-peugnet! I've tested the changes locally with PHP 8.4 and unfortunately discovered a compatibility issue that prevents us from merging this. The ProblemWhile PHPStan 1.9+ works perfectly with PHP 8.4, it's incompatible with the version of Larastan we're using. Specifically:
When running PHPStan 1.9+ with Larastan 1.x, we get this error: TestingI confirmed that:
Path ForwardFor Flarum 1.x, we'll need to keep PHPStan pinned to This should be revisited for Flarum 2.x when we upgrade to Laravel 10+, which will allow us to use Larastan 2.x with PHPStan 1.9+. I think the only option is to close this PR for now, but I really appreciate you taking the time to investigate and submit this fix! 🙏 |
|
I understand, then what about setting This would allow extension developers that do not use larastan to still use phpstan with PHP >= 8.4, and users of larastan to still be able to use it with PHP < 8.4. |
Changes proposed in this pull request:
phpstan 1.8 cannot run properly with PHP 8.4, but newer versions in the 1.x branch do, thus we change the requirement to phpstan ^1.9, to allow version up to 2.0 (excluding).
Newer phpstan versions now detect common errors in regexes:
So these regexes are also fixed by using non-evaluated string literals and escaping the parenthesis as recommended instead of using a weird trick with the brackets.
Reviewers should focus on:
???
Screenshot
Before:
After:

Necessity
Confirmed
composer test).