Skip to content
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

Fix PHP 8.1 incompatibilities #3186

Closed

Conversation

barasimumatik
Copy link

@barasimumatik barasimumatik commented Apr 13, 2022

While migrating to PHP 8.1 I got fatal errors in the two files changed in this commit. Both changes were suggested by the PHP interpreters diagnostic message and when I fixed them my applicaton worked as before (barring an unrelated dependency or two). It has been running under PHP 7.3 and PHP 7.4 before this.

The first change suggested simply adds the #[\ReturnTypeWillChange] attribute to the methods derived from the ArrayAccess class.

The second change relates to type safety since the preg_replace_callback no longer accepts a $subject that is NULL due to changes in PHP 8.1 (as I understood it).

I made sure the existing tests passed when using PHP 5.6 and PHP 7.4 (I didn't manage to install PHP 5.5 though, but I hope that's OK).
The tests won't run in PHP 8.1 due to incompatibilities with the PHPUnit version, and I tried to upgrade to PHPUnit 8.5 and even PHPUnit 9 with no luck.

- Slim/Collection.php: add ReturnTypeWillChange attribute to methods derived from ArrayAccess
- Slim/Http/Uri.php: use empty string instead of null when $query is null in preg_replace_callback
@l0gicgate l0gicgate modified the milestones: 4.11.0, 3.12.4 Apr 13, 2022
@l0gicgate
Copy link
Member

@barasimumatik the 3.x branch is in maintenance mode only unfortunately. If you want PHP 8.x compatibility use Slim 4.

@lcharette
Copy link

@l0gicgate Unless another issue makes Slim not compatible with PHP 8.1, this does fall in the "maintenance" category IMO.

Upgrading to Slim 4 might not be a solution for everyone depending on such a major framework as slim is. It's even more true considering the whole dependency injection change in Slim 4 that makes it not an easy upgrade for some. Compared to the simplicity of this fix...

Otherwise, composer.json should be updated to reflect PHP 8.1 incompatibility ?

@barasimumatik
Copy link
Author

@l0gicgate Unless another issue makes Slim not compatible with PHP 8.1, this does fall in the "maintenance" category IMO.

Upgrading to Slim 4 might not be a solution for everyone depending on such a major framework as slim is. It's even more true considering the whole dependency injection change in Slim 4 that makes it not an easy upgrade for some. Compared to the simplicity of this fix...

Otherwise, composer.json should be updated to reflect PHP 8.1 incompatibility ?

Yeah, I agree. This should fall under maintenance, but this is not my project so it's not up to me 🤷‍♂️.

In our case we can't just switch - it will involve a lot of work and it's hard enough to get some time for simply upgrading PHP and making sure everything works as before.
I hoped I could just upgrade PHP, but since Slim and a couple of other dependencies broke, I tried to fix any issues that stopped my application from working.

@l0gicgate
Copy link
Member

@lcharette @barasimumatik when I say maintenance, I mean security fixes only. I should have been clearer.

The risk of breaking things downstream when releasing anything from this branch now is too great and there's too many unknowns.

@barasimumatik
Copy link
Author

barasimumatik commented Apr 29, 2022

@l0gicgate I understand.

I have one question though (if you don't mind): what do you mean by breaking things downstream? Do you mean breaking things for users of Slim (like me), or do you mean something else? The only thing I had an issue with was PHPUnit not being compatible with PHP 8.1 (or perhaps even 8.0).

I guess what I'm wondering is if there is essentially no way PHP 8+ compatibility can be achieved if someone is willing to put up the work. What is blocking this, if we disregard the "maintenance mode" status, so to speak? (I'm asking mostly out of curiosity)

@dregad
Copy link

dregad commented Nov 30, 2022

@l0gicgate I'm a bit confused...

According to the 3.12.4 release notes, this issue has been fixed, yet I don't see the updated Slim/Collection.php with added #[\ReturnTypeWillChange] attribute.

Am I missing something ?

With regards to #3186 (comment)

The risk of breaking things downstream [...] is too great and there's too many unknowns.

I must admit that I fail to see what could possibly break by adding the attribute, which is basically just a comment and was designed by PHP team to prevent breaking compatibility.

@l0gicgate
Copy link
Member

@dregad it's a mistake in the release notes. This PR was never merged but it was in the milestone. For PHP 8 support, please upgrade to Slim 4.

@dregad
Copy link

dregad commented Nov 30, 2022

Sadly I can't do that at the moment due to other constraints. But thanks for taking the time to reply!

@patrickradius
Copy link

So, we are still running into these issues as well, and to be honest the mention of this PR in the 3.12.4 release notes was totally throwing us off and cost us a lot of head scratching.
We even opened a new PR with kind of the same changes because we thought this Collection class was just not done yet.

Could you please reconsider merging this PR into the 3.x branch, because it will help out a lot of people.

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.

None yet

5 participants