[Console] Bind the closure (code) to the Command if possible #14431

lyrixx · 2015-04-21T19:10:02Z

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:

#!/usr/bin/env php
<?php

require __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();

stof · 2015-04-21T19:41:24Z

how does this behave in case the closure passed as argument was already bound previously ?

lyrixx · 2015-04-21T19:45:19Z

Symfony is the last one to bind, so symfony wins:

#!/usr/bin/env php
<?php

require __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();

$code = function (InputInterface $input, OutputInterface $output) use ($application) {
    $output->writeln('===================');
    $application = $this->getApplication();
    $help = $application->find('help');
    $output->writeln($help->getHelp());
    $output->writeln('===================');
};

$code = $code->bindTo($application);

$application->add((new Command('process')))
    ->setDescription('Play all other commands')
    ->setCode($code)
;
$application->run();

=>

php test.php  process
===================
The %command.name% command displays help for a given command:

  php %command.full_name% list

You can also output the help in other formats by using the --format option:

  php %command.full_name% --format=xml list

To display the list of available commands, please use the list command.
===================

(yeah, we can do crazy things with closure binding....)

jakzal · 2015-04-22T08:22:07Z

src/Symfony/Component/Console/Command/Command.php

@@ -284,6 +284,10 @@ public function setCode($code)
            throw new \InvalidArgumentException('Invalid callable provided to Command::setCode.');
        }

+        if (PHP_VERSION_ID >= 50400 && $code instanceof \Closure) {


Is the PHP version check necessary? In PHP 5.3 the instanceof check will return false, as the Closure class doesn't exist.

You are right, but what if someone define a user-land Closure 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

The Closure class exists since 5.3, but Closure::bind has been introduced in 5.4, so the check is required :)

nicolas-grekas · 2015-04-22T08:52:23Z

I'd bind only if the closure is not already bound, so that we keep BC

stof · 2015-04-22T08:59:33Z

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 · 2015-04-22T09:02:26Z

Is there a way to detect whether the closure was bound explicitly in PHP ?

yes, http://php.net/manual/en/reflectionfunctionabstract.getclosurethis.php

        if (PHP_VERSION_ID >= 50400 && $code instanceof \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 the doc says it's possible)

lyrixx · 2015-04-22T09:16:05Z

Here we go, should be ok now...

stof · 2015-04-22T09:19:35Z

src/Symfony/Component/Console/Tests/Command/CommandTest.php

+    public function getSetCodeBindToClosureTests()
+    {
+        return array(
+            array(true, 'not binded'),


typo: bound, not binded

aitboudad · 2015-04-22T09:26:55Z

src/Symfony/Component/Console/Command/Command.php

@@ -284,6 +284,13 @@ public function setCode($code)
            throw new \InvalidArgumentException('Invalid callable provided to Command::setCode.');
        }

+        if (PHP_VERSION_ID >= 50400 && $code instanceof \Closure) {
+            $r = new \ReflectionFunction($code);
+            if (null === $r->getClosureThis()) {


when it's null should we throw an exception ?

Why ? This is precisely the case where we add a new feature

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 · 2015-04-22T09:29:50Z

👍

lyrixx · 2015-04-23T09:01:50Z

@stof Is it ok for you?

stof · 2015-04-23T09:03:19Z

👍

stof · 2015-04-23T09:07:29Z

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 <?php require __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 possible

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

xabbuh added the Console label Apr 21, 2015

jakzal reviewed Apr 22, 2015
View reviewed changes

lyrixx force-pushed the command-bind-this branch from b256312 to 1400f4e Compare April 22, 2015 08:28

lyrixx force-pushed the command-bind-this branch from 1400f4e to 256de36 Compare April 22, 2015 09:15

stof reviewed Apr 22, 2015
View reviewed changes

lyrixx force-pushed the command-bind-this branch 3 times, most recently from 6050e76 to 71d7c25 Compare April 22, 2015 09:24

aitboudad reviewed Apr 22, 2015
View reviewed changes

[Console] Bind the closure (code) to the Command if possible

ff4424a

lyrixx force-pushed the command-bind-this branch from 71d7c25 to ff4424a Compare April 22, 2015 09:30

stof merged commit ff4424a into symfony:2.8 Apr 23, 2015

fabpot mentioned this pull request Nov 16, 2015

Release v2.8.0-BETA1 #16550

Merged

lyrixx deleted the command-bind-this branch December 8, 2015 15:21

This was referenced Dec 9, 2016

[Console] Cannot call setCode with static closures #20845

Closed

[Console] fixed BC issue with static closures #20847

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Bind the closure (code) to the Command if possible #14431

[Console] Bind the closure (code) to the Command if possible #14431

lyrixx commented Apr 21, 2015

stof commented Apr 21, 2015

lyrixx commented Apr 21, 2015

jakzal Apr 22, 2015

lyrixx Apr 22, 2015

nicolas-grekas Apr 22, 2015

jakzal Apr 22, 2015

nicolas-grekas commented Apr 22, 2015

stof commented Apr 22, 2015

lyrixx commented Apr 22, 2015

lyrixx commented Apr 22, 2015

stof Apr 22, 2015

aitboudad Apr 22, 2015

stof Apr 22, 2015

aitboudad Apr 22, 2015

nicolas-grekas commented Apr 22, 2015

lyrixx commented Apr 23, 2015

stof commented Apr 23, 2015

stof commented Apr 23, 2015

[Console] Bind the closure (code) to the Command if possible #14431

[Console] Bind the closure (code) to the Command if possible #14431

Conversation

lyrixx commented Apr 21, 2015

stof commented Apr 21, 2015

lyrixx commented Apr 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 22, 2015

stof commented Apr 22, 2015

lyrixx commented Apr 22, 2015

lyrixx commented Apr 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 22, 2015

lyrixx commented Apr 23, 2015

stof commented Apr 23, 2015

stof commented Apr 23, 2015