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 errors #1407
Fix PHP 8.1 errors #1407
Conversation
@@ -130,6 +130,7 @@ public function offsetGet($key): Subject | |||
* | |||
* @param int|string $key | |||
*/ | |||
#[\ReturnTypeWillChange] |
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.
As mentioned in #1397 (comment), this comment was added instead of the void
return type in order to preserve backwards compatibility.
if ($token == '|' || $token == '&') { | ||
// PHP 8.1 returns the intersection token `&` as an array, | ||
// while previous versions return it as a string. | ||
if ($token == '|' || $token == '&' || (is_array($token) && $token[1] == '&')) { |
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.
This will break if PHP returns a token-array with only one item. Can I count on the array to always have a 1
-index or should I check for it explicitly? 🤔
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.
I'd say that the explicit check isn't hard to add but might make the code somewhat harder to understand:
if ($token == '|' || $token == '&' || (is_array($token) && ($token[1] ?? '') == '&')) {
//...
}
It could be more readable by separating out the expression into independent booleans:
$isFoo = is_string($token) && $token == '|' || $token == '&';
$isBar = is_array($token) && ($token[1] ?? '') == '&';
if ($isFoo || $isBar) {
return false;
}
Best of luck with naming $isFoo
and $isBar
correctly ...
@ciaranmcnulty If I can help you to review this PR in any way, please let me know. |
FYI I've forked and applied all the patches in the suggested order (see #1402 (comment)) and I can confirm that by using this fork we've been able to finish our Symfony 6 migration and to upgrade from php 8 to 8.1. Everything works as expected since few days 👌 https://github.com/gogaille/phpspec/tree/php-81-and-symfony-6 |
We can also confirm that it works - https://github.com/loic425/Sylius/runs/5037334478 (thanks @loic425 for branch). |
Hello, it's a friendly 2-weeks reminder about this PR 😄 👋 @ciaranmcnulty do you think it's possible to be merged in the predictable future? Maybe someone else from PHPSpec Organization has some info? @MarcelloDuarte @stof? 🚀 |
Fixes PHP 8.1 errors in CI pipeline.
Closes #1397.
There are deprecation warnings, but the pipeline still succeeds.
I think this is related to phpspec/prophecy#414.