Skip to content

Comments

Compatibility with PHP 8.4#103

Merged
dbrekelmans merged 1 commit intodbrekelmans:mainfrom
ddebin:fix/php84
Nov 3, 2025
Merged

Compatibility with PHP 8.4#103
dbrekelmans merged 1 commit intodbrekelmans:mainfrom
ddebin:fix/php84

Conversation

@ddebin
Copy link
Contributor

@ddebin ddebin commented Oct 29, 2025

  • Upgrade thecodingmachine/safe to ^3.0 to get compatibility with PHP 8.4
  • Force upgrade to PHPStan 2, fix new warnings
  • thecodingmachine/phpstan-strict-rules is not compatible with PHPStan 2 and has been removed
  • Remove useless PHP 8.0 polyfill as we target PHP 8.1+

… 8.4, force upgrade to PHPStan 2, fix new warnings
@ddebin ddebin changed the title Upgrade thecodingmachine/safe to ^3.0 Compatibility with PHP 8.4 Oct 30, 2025
@alex-ception
Copy link

would be nice to have it merged :)

@@ -1,5 +1,5 @@
parameters:
level: max
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific reason for lowering the level? Max always defaults to the highest level available in the release. (Being level 10 as of PHPStan 2.0)

Copy link
Contributor Author

@ddebin ddebin Nov 2, 2025

Choose a reason for hiding this comment

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

New level max is equivalent to 10 and needs a lot of warnings fixed (mostly dealing with mixed and untyped arrays), maybe outside the scope of this PR.
https://phpstan.org/blog/phpstan-2-0-released-level-10-elephpants#level-10

Copy link
Contributor

@KevinVanSonsbeek KevinVanSonsbeek Nov 2, 2025

Choose a reason for hiding this comment

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

Alternative would be to keep it on max, but for the time being baseline the new warnings.

That way the new errors are atleast tracked, and the new checks would be applied to any new logic added to the repository in the future. But i'm not sure what @dbrekelmans his view is on that

(edit, fixed the mention)

Copy link
Owner

Choose a reason for hiding this comment

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

I agree that it's outside the scope of this PR, but it would be best to go back up to max level. It doesn't need to block this PR from being merged. If either of you is willing to produce a follow-up PR where we go back up to max level (and ideally fix the new warnings), that would be greatly appreciated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can propose this next PR.

@dbrekelmans
Copy link
Owner

Thanks for the contribution @ddebin! 🎉

@dbrekelmans dbrekelmans merged commit 012408b into dbrekelmans:main Nov 3, 2025
55 of 76 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants