Skip to content

Conversation

@jrfnl
Copy link
Collaborator

@jrfnl jrfnl commented Sep 15, 2025

These are only some "low hanging fruit" fixes. There is a deeper problem in the underlying wiring causing a lot more of these deprecation notices, which I haven't been able to track down yet.

At least the fixes in this PR should allow to focus on the deeper problem by getting rid of a sizable amount of deprecation notices (reduces the test deprecation notice output from > 2200 lines to less than 500 lines).

Ref: https://wiki.php.net/rfc/deprecations_php_8_5#deprecate_using_values_null_as_an_array_offset_and_when_calling_array_key_exists

…tices

These are only some "low hanging fruit" fixes. There is a deeper problem in the underlying wiring causing a lot more of these deprecation notices, which I haven't been able to track down yet.

At least the fixes in this PR should allow to focus on the deeper problem by getting rid of a sizable amount of deprecation notices.

Ref: https://wiki.php.net/rfc/deprecations_php_8_5#deprecate_using_values_null_as_an_array_offset_and_when_calling_array_key_exists
@swissspidy
Copy link

@jrfnl The two big remaining issues are here:

$__pwClass = (__CLASS__ && __FUNCTION__ !== $__pwClosureName) ? __CLASS__ : null;
if (!empty(\Patchwork\CallRerouting\State::$routes[$__pwClass][__FUNCTION__])) {

$__pwClass = (__CLASS__ && __FUNCTION__ !== $__pwClosureName) ? __CLASS__ : null;
if (!empty(\Patchwork\CallRerouting\State::$routes[$__pwClass][__FUNCTION__])) {

$__pwClass can be null. Casting to (string) on those lines removes all but 2 deprecation warnings for me when running the tests. The remaining ones coming from tests/never-typed.phpt.

@swissspidy
Copy link

Actually, that last one was just a few lines down:

$__pwClass = (__CLASS__ && __FUNCTION__ !== $__pwClosureName) ? __CLASS__ : null;
if (!empty(\Patchwork\CallRerouting\State::$routes[$__pwClass][__FUNCTION__])) {

Casting to a string fixes all the deprecation issues in the project when running tests 🎉

@antecedent
Copy link
Owner

@swissspidy, that was a really great catch! Thanks to the both of you :)

@jrfnl, do I remember correctly: this should go to the upcoming patch release of 2.2.x, and then we should wait for more PHP 8.5-related issues, right?

@jrfnl
Copy link
Collaborator Author

jrfnl commented Sep 16, 2025

I love how this PR became a group collaboration!!!

Nice find @swissspidy !

And @antecedent I agree with the fix you added (nitpick: I would use single quotes, but it's not as if it would make any difference in reality).

Do I remember correctly: this should go to the upcoming patch release of 2.2.x, and then we should wait for more PHP 8.5-related issues, right?

I would recommend merging this & releasing a patch release soon after. Anything that helps unblock other projects from testing against PHP 8.5 is a good thing TM and if more fixes would still be needed later, they can go into the next patch release.

Having said that, chances of more fixes being needed are getting slimmer by the day as the hard feature freeze for PHP 8.5 is only one week away.

@antecedent antecedent merged commit 8b6b235 into master Sep 17, 2025
20 checks passed
@antecedent
Copy link
Owner

Yay! Also, @jrfnl, re: single/double quotes: is there a reason to prefer one over another in this case, besides coding style? I mean, I would be all for standardizing the style choices in Patchwork in the future; my question is coming from a place of curiosity :) Anything performance-related? I would reason that string interning would take care of that (we only have constants in the snippet), but maybe I am missing out on something?

@jrfnl
Copy link
Collaborator Author

jrfnl commented Sep 17, 2025

Yay! Also, @jrfnl, re: single/double quotes: is there a reason to prefer one over another in this case, besides coding style? I mean, I would be all for standardizing the style choices in Patchwork in the future; my question is coming from a place of curiosity :) Anything performance-related? I would reason that string interning would take care of that (we only have constants in the snippet), but maybe I am missing out on something?

For older PHP versions there was a small performance benefit as PHP wouldn't try to find and handle interpolation. For modern PHP versions that's no longer the case as (IIRC) the compiler will optimize it nowadays.

I would be all for standardizing the style choices in Patchwork in the future

Happy to help with that and propose a PHP_CodeSniffer ruleset if you like. Would you want to standardize on something like PSR12 (or in the future PERCS) ? Or to standardize to something which is closest to what is currently used ?

@jrfnl jrfnl deleted the feature/179-callrerouting-fix-php-8.5-deprecation-notice branch September 17, 2025 13:00
@jrfnl jrfnl modified the milestones: 2.2 Next, 2.2.3 Sep 17, 2025
@antecedent
Copy link
Owner

Would you want to standardize on something like PSR12 (or in the future PERCS) ? Or to standardize to something which is closest to what is currently used ?

I suppose that PSR-12 (or PERCS) is the typical "default" choice for new PHP projects nowadays. If it is indeed so, then it is what I would go with. Thanks for offering to help!

@jrfnl
Copy link
Collaborator Author

jrfnl commented Sep 18, 2025

@antecedent Will put it on my low-priority list, so may still be a while before I get to it. Then again, this repo is low traffic, so I suppose that's not an issue anyway.

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