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

[Console] Add command resolver (proof of concept) #16438

Closed
wants to merge 12 commits into
from

Conversation

Projects
None yet
@funivan

funivan commented Nov 3, 2015

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #12063
License MIT
Doc PR NA

Hello. This is simple proof of concept for command resolver.

Why?

On huge projects we should always initialize all commands, even if they are not used.
Some users want lazy-loading of commands. And there are a lot of ways how to do that. Store cache in files, db, initialize commands by unique id etc..

How?

This PR introduce new interface CommandResolverInterface. Any user can implement logic and inject it to the application. With this interface developer can create commands by request

BC ?

This interface is softly injected to this application. All tests pass. By default CommandResolver is simple holder for commands.

Current state

There are one problem: when we add Command to application, application add this command only if it is available. This logic is not covered in CommandResolverInterface. We put all responsibility to developer which Commands are available to application.

If it will be interesting to the team I can polish it (fix code style, possible change names of interface, write documentation etc.)

funivan added some commits Nov 3, 2015

@stof

View changes

Show outdated Hide outdated src/Symfony/Component/Console/Application.php
@@ -87,6 +93,8 @@ public function __construct($name = 'UNKNOWN', $version = 'UNKNOWN')
$this->helperSet = $this->getDefaultHelperSet();
$this->definition = $this->getDefaultInputDefinition();
$this->commands = !empty($commandResolver) ? $commandResolver : new CommandResolver();

This comment has been minimized.

@stof

stof Nov 3, 2015

Member

use a null comparison instead

@stof

stof Nov 3, 2015

Member

use a null comparison instead

@stof

View changes

Show outdated Hide outdated src/Symfony/Component/Console/Application.php
/**
* @var CommandResolverInterface
*/
private $commands;

This comment has been minimized.

@stof

stof Nov 3, 2015

Member

commandResolver would be a better name

@stof

stof Nov 3, 2015

Member

commandResolver would be a better name

@stof

View changes

Show outdated Hide outdated src/Symfony/Component/Console/Application.php
if (!isset($this->commands[$name])) {
$command = $this->commands->get($name);
if (!isset($command)) {

This comment has been minimized.

@stof

stof Nov 3, 2015

Member

don't use isset to check for null

@stof

stof Nov 3, 2015

Member

don't use isset to check for null

@stof

View changes

Show outdated Hide outdated src/Symfony/Component/Console/CommandsResolver/CommandResolver.php
/**
* {@inheritdoc}
*/
public function add(Command $command)

This comment has been minimized.

@stof

stof Nov 3, 2015

Member

wrong indentation

@stof

stof Nov 3, 2015

Member

wrong indentation

@stof

View changes

Show outdated Hide outdated src/Symfony/Component/Console/CommandsResolver/CommandResolver.php
$this->commands[$alias] = $command;
}
return $this;

This comment has been minimized.

@stof

stof Nov 3, 2015

Member

-1 for making it fluent

@stof

stof Nov 3, 2015

Member

-1 for making it fluent

This comment has been minimized.

@funivan

funivan Nov 3, 2015

@stof can you explain why?
Should I return Command object instead ?

@funivan

funivan Nov 3, 2015

@stof can you explain why?
Should I return Command object instead ?

This comment has been minimized.

@stof

stof Nov 3, 2015

Member

IMO, you should not return anything. Returning the command object does not make sense: if the user need it, they can save it first as they are passing it as argument.

Btw, the code using this method ignores the return value entirely, which is a clear indication that there is no meaningful one.

and regarding the fluent interface, I suggest you to read http://ocramius.github.io/blog/fluent-interfaces-are-evil/ as there are good arguments in it

@stof

stof Nov 3, 2015

Member

IMO, you should not return anything. Returning the command object does not make sense: if the user need it, they can save it first as they are passing it as argument.

Btw, the code using this method ignores the return value entirely, which is a clear indication that there is no meaningful one.

and regarding the fluent interface, I suggest you to read http://ocramius.github.io/blog/fluent-interfaces-are-evil/ as there are good arguments in it

This comment has been minimized.

@funivan

funivan Nov 3, 2015

Ok thanks. I will fix it in a minute.

@funivan

funivan Nov 3, 2015

Ok thanks. I will fix it in a minute.

@stof

View changes

Show outdated Hide outdated src/Symfony/Component/Console/CommandsResolver/CommandResolverInterface.php
public function add(Command $command);
/**
* Check if command exist.

This comment has been minimized.

@stof

stof Nov 3, 2015

Member

checks whether the command exists

@stof

stof Nov 3, 2015

Member

checks whether the command exists

This comment has been minimized.

@funivan

funivan Nov 3, 2015

add method should check if command exist?

@funivan

funivan Nov 3, 2015

add method should check if command exist?

funivan added some commits Nov 3, 2015

@aitboudad

View changes

Show outdated Hide outdated src/Symfony/Component/Console/Application.php
@@ -87,6 +93,8 @@ public function __construct($name = 'UNKNOWN', $version = 'UNKNOWN')
$this->helperSet = $this->getDefaultHelperSet();
$this->definition = $this->getDefaultInputDefinition();
$this->commandResolver = ($commandResolver !== null) ? $commandResolver : new CommandResolver();

This comment has been minimized.

@aitboudad

aitboudad Nov 4, 2015

Contributor

it should be null !== $commandResolver ? and parentheses is useless here

@aitboudad

aitboudad Nov 4, 2015

Contributor

it should be null !== $commandResolver ? and parentheses is useless here

@aitboudad

View changes

Show outdated Hide outdated src/Symfony/Component/Console/CommandsResolver/CommandResolver.php
*/
public function has($name)
{
return !empty($this->commands[$name]);

This comment has been minimized.

@aitboudad

aitboudad Nov 4, 2015

Contributor

isset is enough

@aitboudad

aitboudad Nov 4, 2015

Contributor

isset is enough

This comment has been minimized.

@aitboudad

View changes

Show outdated Hide outdated src/Symfony/Component/Console/CommandsResolver/CommandResolver.php
*/
public function get($name)
{
return !empty($this->commands[$name]) ? $this->commands[$name] : null;

This comment has been minimized.

@aitboudad

aitboudad Nov 4, 2015

Contributor

same here

@aitboudad

aitboudad Nov 4, 2015

Contributor

same here

@stloyd

View changes

Show outdated Hide outdated src/Symfony/Component/Console/Tests/CommandResolver/CommandResolverTest.php
*/
class CommandResolverTest extends \PHPUnit_Framework_TestCase
{
public function testLazyCommandResolver()

This comment has been minimized.

@stloyd

stloyd Nov 4, 2015

Contributor

Wrong indent in whole file. Use 4 spaces not 2.

@stloyd

stloyd Nov 4, 2015

Contributor

Wrong indent in whole file. Use 4 spaces not 2.

This comment has been minimized.

@funivan

funivan Nov 4, 2015

Sorry for that. I will fix it.

@funivan

funivan Nov 4, 2015

Sorry for that. I will fix it.

@stloyd

View changes

Show outdated Hide outdated src/Symfony/Component/Console/Tests/Fixtures/CustomCommandResolver.php
/**
* @author Ivan Shcherbak <dev@funivan.com>
*/
class CustomCommandResolver implements CommandResolverInterface

This comment has been minimized.

@stloyd

stloyd Nov 4, 2015

Contributor

Why not extend existing one and just add default fixture code in constructor?

@stloyd

stloyd Nov 4, 2015

Contributor

Why not extend existing one and just add default fixture code in constructor?

This comment has been minimized.

@funivan

funivan Nov 4, 2015

I want to show how to implement CommandResolverInterface from zero.
What do you mean: add default fixture code in constructor? Can you show me an example, I don`t understand.

@funivan

funivan Nov 4, 2015

I want to show how to implement CommandResolverInterface from zero.
What do you mean: add default fixture code in constructor? Can you show me an example, I don`t understand.

funivan added some commits Nov 4, 2015

@funivan

This comment has been minimized.

Show comment
Hide comment
@funivan

funivan Nov 12, 2015

@stof Hello. Can you help me? I can`t pass all tests of this PR at CI services (In local environment all tests are green).
Some times it failure in AppVeyor sometimes in travis, but always in different places. And even more, sometimes failures are not relevant to my commits. Thank you.
What should I do to make this PR green?

funivan commented Nov 12, 2015

@stof Hello. Can you help me? I can`t pass all tests of this PR at CI services (In local environment all tests are green).
Some times it failure in AppVeyor sometimes in travis, but always in different places. And even more, sometimes failures are not relevant to my commits. Thank you.
What should I do to make this PR green?

@xabbuh xabbuh added the Console label Nov 12, 2015

@TomasVotruba

This comment has been minimized.

Show comment
Hide comment
@TomasVotruba

TomasVotruba Mar 3, 2016

Contributor

Very nice PR!

Tests for Console are passing well. It fails on Symfony/Bridge/ProxyManager package, not sure why.
Maybe rerun might help.

Contributor

TomasVotruba commented Mar 3, 2016

Very nice PR!

Tests for Console are passing well. It fails on Symfony/Bridge/ProxyManager package, not sure why.
Maybe rerun might help.

@funivan

This comment has been minimized.

Show comment
Hide comment
@funivan

funivan Mar 3, 2016

@TomasVotruba yep and I don`t know how to resolve this. Is there any way to rerun this tests?

funivan commented Mar 3, 2016

@TomasVotruba yep and I don`t know how to resolve this. Is there any way to rerun this tests?

@DavidBadura

This comment has been minimized.

Show comment
Hide comment
@DavidBadura

DavidBadura Mar 3, 2016

Contributor

with a rebase 😉

Contributor

DavidBadura commented Mar 3, 2016

with a rebase 😉

@javiereguiluz javiereguiluz changed the title from [CONSOLE] Add command resolver (proof of concept) to [Console] Add command resolver (proof of concept) Mar 3, 2016

@funivan

This comment has been minimized.

Show comment
Hide comment
@funivan

funivan Jul 22, 2016

Hi 2 all. It is easier to create new pull request and implement the same logic.
If this feature is interesting to the community and to the core team I can implement the same algorithm of the command resolver.

funivan commented Jul 22, 2016

Hi 2 all. It is easier to create new pull request and implement the same logic.
If this feature is interesting to the community and to the core team I can implement the same algorithm of the command resolver.

{
$this->name = $name;
$this->version = $version;
$this->defaultCommand = 'list';
$this->helperSet = $this->getDefaultHelperSet();
$this->definition = $this->getDefaultInputDefinition();
$this->commandResolver = $commandResolver !== null ? $commandResolver : new CommandResolver();

This comment has been minimized.

@chalasr

chalasr Oct 13, 2016

Member

I think we use $this->commandResolver = $commandResolver ?: new CommandResolver()

@chalasr

chalasr Oct 13, 2016

Member

I think we use $this->commandResolver = $commandResolver ?: new CommandResolver()

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 11, 2017

Member

Closing in favor of #22734 which is going to be accepted and provide most of what this tries to achieve.
Thanks for working on this @funivan.

Member

nicolas-grekas commented Jul 11, 2017

Closing in favor of #22734 which is going to be accepted and provide most of what this tries to achieve.
Thanks for working on this @funivan.

@funivan funivan deleted the funivan:console-command-resolv branch Jul 11, 2017

nicolas-grekas added a commit that referenced this pull request Jul 12, 2017

feature #22734 [Console] Add support for command lazy-loading (chalasr)
This PR was merged into the 3.4 branch.

Discussion
----------

[Console] Add support for command lazy-loading

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #12063 #16438 #13946 #21781
| License       | MIT
| Doc PR        | todo

This PR adds command lazy-loading support to the console, based on PSR-11 and DI tags.
(#12063 (comment))

Commands registered as services which set the `command` attribute on their `console.command` tag are now instantiated when calling `Application::get()` instead of all instantiated at `run()`.

__Usage__

```yaml
app.command.heavy:
    tags:
        - { name: console.command, command: app:heavy }
```

This way private command services can be inlined (no public aliases, remain really private).

With console+PSR11 implem only:

```php
$application = new Application();
$container = new ServiceLocator(['heavy' => function () { return new Heavy(); }]);
$application->setCommandLoader(new ContainerCommandLoader($container, ['app:heavy' => 'heavy']);
```

Implementation is widely inspired from Twig runtime loaders (without the definition/runtime separation which is not needed here).

Commits
-------

7f97519 Add support for command lazy-loading

symfony-splitter pushed a commit to symfony/security-bundle that referenced this pull request Jul 12, 2017

feature #22734 [Console] Add support for command lazy-loading (chalasr)
This PR was merged into the 3.4 branch.

Discussion
----------

[Console] Add support for command lazy-loading

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/symfony#12063 symfony/symfony#16438 symfony/symfony#13946 #21781
| License       | MIT
| Doc PR        | todo

This PR adds command lazy-loading support to the console, based on PSR-11 and DI tags.
(symfony/symfony#12063 (comment))

Commands registered as services which set the `command` attribute on their `console.command` tag are now instantiated when calling `Application::get()` instead of all instantiated at `run()`.

__Usage__

```yaml
app.command.heavy:
    tags:
        - { name: console.command, command: app:heavy }
```

This way private command services can be inlined (no public aliases, remain really private).

With console+PSR11 implem only:

```php
$application = new Application();
$container = new ServiceLocator(['heavy' => function () { return new Heavy(); }]);
$application->setCommandLoader(new ContainerCommandLoader($container, ['app:heavy' => 'heavy']);
```

Implementation is widely inspired from Twig runtime loaders (without the definition/runtime separation which is not needed here).

Commits
-------

7f97519 Add support for command lazy-loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment