[Console] fixed BC issue with static closures #20847
Conversation
Static closures were unable to be used in Command::setCode since #14431. This change fixes the BC break and ensures static closures can still be used.
| @@ -283,7 +283,8 @@ public function setCode($code) | |||
| if (PHP_VERSION_ID >= 50400 && $code instanceof \Closure) { | |||
| $r = new \ReflectionFunction($code); | |||
| if (null === $r->getClosureThis()) { | |||
| $code = \Closure::bind($code, $this); | |||
| // Cannot bind static closures for PHP<7.0 so it may fail | |||
| $code = @\Closure::bind($code, $this); | |||
There was a problem hiding this comment.
IMO, we should find a way not requiring to silence errors. This is bypassing the issue
There was a problem hiding this comment.
I do completely agree, but I was really struggling to find a way around this - the only resources I could really find were from people who were doing it this way (or more convoluted ways which still required the errors being suppressed). If someone can come up with a better way, I'd much prefer it that way.
Assuming there isn't a better way, though - I could perhaps add a much more explanatory comment noting that it is only really needed for PHP5 compatibility and that the error suppress is unavoidable?
There was a problem hiding this comment.
To explain a little further: The issue is that (as far as I can tell) there is no way to tell that a closure is static (or unbindable) without actually attempting to bind it and then seeing if it bound correctly. If it didn't bind, then you end up with what you started with.
So another solution would be to create a separate method isBindable(Closure $closure) which would return true or false. Ultimately within that method it would have to do an error-suppressed bind operation in order to determine whether it was possible. So that might result in slightly clearer code - but it would be fundamentally doing the same thing.
Here is a Stack Overflow question which highlights the problem and a potential solution quite well: http://stackoverflow.com/questions/28047640/determining-if-a-closure-is-static-in-php
| $tester = new CommandTester($command); | ||
| $tester->execute(array()); | ||
|
|
||
| if (version_compare(phpversion(), '7.0.0', '<')) { |
There was a problem hiding this comment.
please use PHP_VERSION_ID for version comparisons
|
👍 |
1 similar comment
|
👍 |
|
Thank you @araines. |
This PR was squashed before being merged into the 2.8 branch (closes #20847). Discussion ---------- [Console] fixed BC issue with static closures | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20845 | License | MIT | Doc PR | n/a Static closures were unable to be used in Command::setCode since #14431. This change fixes the BC break and ensures static closures can still be used. Edit: It seems the inability to bind static closures was considered a feature in PHP5 but was considered a bug by PHP7. As such, the tests need to work around this fact - but the code can remain the same. This code change can be tidied/removed once Symfony is PHP7+ only. Discussion here: https://bugs.php.net/bug.php?id=64761 https://bugs.php.net/bug.php?id=68792 Commits ------- 3247308 [Console] fixed BC issue with static closures
Static closures were unable to be used in Command::setCode since #14431. This change fixes the BC break and ensures static closures can still be used.
Edit: It seems the inability to bind static closures was considered a feature in PHP5 but was considered a bug by PHP7. As such, the tests need to work around this fact - but the code can remain the same. This code change can be tidied/removed once Symfony is PHP7+ only.
Discussion here:
https://bugs.php.net/bug.php?id=64761
https://bugs.php.net/bug.php?id=68792