Skip to content

[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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 23, 2015
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/Symfony/Component/Console/Command/Command.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

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 the Closure class doesn't exist.

Copy link
Member Author

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-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

Copy link
Member

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha! :)

$r = new \ReflectionFunction($code);
if (null === $r->getClosureThis()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when it's null should we throw an exception ?

Copy link
Member

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

Copy link
Contributor

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

$code = \Closure::bind($code, $this);
}
}

$this->code = $code;

return $this;
Expand Down
37 changes: 37 additions & 0 deletions src/Symfony/Component/Console/Tests/Command/CommandTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,33 @@ public function testSetCode()
$this->assertEquals('interact called'.PHP_EOL.'from the code...'.PHP_EOL, $tester->getDisplay());
}

public function getSetCodeBindToClosureTests()
{
return array(
array(true, 'not bound to the command'),
array(false, 'bound to the command'),
);
}

/** @dataProvider getSetCodeBindToClosureTests */
public function testSetCodeBindToClosure($previouslyBound, $expected)
{
if (PHP_VERSION_ID < 50400) {
$this->markTestSkipped('Test skipped, for PHP 5.4+ only.');
}

$code = createClosure();
if ($previouslyBound) {
$code = $code->bindTo($this);
}

$command = new \TestCommand();
$command->setCode($code);
$tester = new CommandTester($command);
$tester->execute(array());
$this->assertEquals('interact called'.PHP_EOL.$expected.PHP_EOL, $tester->getDisplay());
}

public function testSetCodeWithNonClosureCallable()
{
$command = new \TestCommand();
Expand Down Expand Up @@ -346,3 +373,13 @@ public function testLegacyAsXml()
$this->assertXmlStringEqualsXmlFile(self::$fixturesPath.'/command_asxml.txt', $command->asXml(), '->asXml() returns an XML representation of the command');
}
}

// In order to get an unbound closure, we should create it outside a class
// scope.
function createClosure()
{
return function(InputInterface $input, OutputInterface $output)
{
$output->writeln($this instanceof Command ? 'bound to the command' : 'not bound to the command');
};
}