Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Bind the closure (code) to the Command if possible#14431
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
stof commentedApr 21, 2015
how does this behave in case the closure passed as argument was already bound previously ? |
lyrixx commentedApr 21, 2015
Symfony is the last one to bind, so symfony wins: => (yeah, we can do crazy things with closure binding....) |
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.
Is the PHP version check necessary? In PHP 5.3 the instanceof check will return false, as theClosure class doesn't exist.
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.
You are right, but what if someone define a user-landClosure class ?
IHMO, it's safer to keep the version check (and will be removed in SF3.0 anyway)
edit: I did not know Closure exists since php 5.3
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.
The Closure class exists since 5.3, but Closure::bind has been introduced in 5.4, so the check is required :)
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.
Aha! :)
nicolas-grekas commentedApr 22, 2015
I'd bind only if the closure is not already bound, so that we keep BC |
stof commentedApr 22, 2015
The JS way to bind closures is better IMO, by making sure that the first one binding a closure wins, letting the creator of the closure can control how it is called without having to bother with code using the closure. Is there a way to detect whether the closure was bound explicitly in PHP ? |
lyrixx commentedApr 22, 2015
yes,http://php.net/manual/en/reflectionfunctionabstract.getclosurethis.php if (PHP_VERSION_ID >=50400 &&$codeinstanceof \Closure) {$r =new \ReflectionFunction($code);if (null ===$r->getClosureThis()) {$code = \Closure::bind($code,$this); } } but I'm not able to unbind a closure in tests ... (even if thedoc says it's possible) |
lyrixx commentedApr 22, 2015
Here we go, should be ok now... |
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.
typo:bound, notbinded
6050e76 to71d7c25CompareThere 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.
when it'snull should we throw an exception ?
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.
Why ? This is precisely the case where we add a new feature
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.
It's ok I understand now :D ,getClosureThis is not documented, so it would be nice if we add a comment that explain why we use it
nicolas-grekas commentedApr 22, 2015
👍 |
lyrixx commentedApr 23, 2015
@stof Is it ok for you? |
stof commentedApr 23, 2015
👍 |
stof commentedApr 23, 2015
Thanks@lyrixx. |
…ssible (lyrixx)This PR was merged into the 2.8 branch.Discussion----------[Console] Bind the closure (code) to the Command if possible| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | ~| License | MIT| Doc PR | ~This allow this kind of code:```php#!/usr/bin/env php<?phprequire __DIR__.'/vendor/autoload.php';use Symfony\Component\Console\Application;use Symfony\Component\Console\Command\Command;use Symfony\Component\Console\Input\InputInterface;use Symfony\Component\Console\Output\OutputInterface;$application = new Application();$application->add((new Command('process'))) ->setDescription('Play all other commands') ->setCode(function (InputInterface $input, OutputInterface $output) use ($application) { $application = $this->getApplication(); $help = $application->find('help'); $output->writeln($help->getHelp()); });$application->run();```Commits-------ff4424a [Console] Bind the closure (code) to the Command if possibleThis 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/aStatic 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=64761https://bugs.php.net/bug.php?id=68792Commits-------3247308 [Console] fixed BC issue with static closures
This allow this kind of code: