Skip to content

[Console] Added support for lazy-loaded commands (with Command resolver) #13989

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

[Console] Added support for lazy-loaded commands (with Command resolver) #13989

wants to merge 4 commits into from

Conversation

unkind
Copy link
Contributor

@unkind unkind commented Mar 19, 2015

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

This PR is an alternative (well, I treat it more like PoC and I didn't bother to add tests for now) to the #13946. Example of usage:

class GreetCommand extends Command
{
    public function __construct()
    {
        sleep(3);
    }

    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $output->writeln('Hello, world');
    }
}

$application = new Application();

$application->setCommandResolver(new ClosureCommandResolver(function ($name) {
    switch ($name) {
        case 'foo:bar':
            return new GreetCommand();
    }
}));

$application->addCommandConfiguration(
    CommandConfiguration::create('foo:bar')
        ->withDescription('Foo and Bar')
        ->withHelp('Help message')
        ->withDefinition(
            new InputDefinition([
                new InputOption('baz', 'b', InputOption::VALUE_REQUIRED)
            ])
        )
);

$application->run();

Note that I didn't call parent constructor in the GreetCommand on purpose. Given it's all about commands as services, I don't want to call parent constructor every time.

Thoughts?

/cc @mnapoli

@unkind unkind force-pushed the feature-lazy-loading-console-command branch from a07749f to 4b2725e Compare March 19, 2015 23:27
@hhamon
Copy link
Contributor

hhamon commented Mar 20, 2015

Hi !
Would this be submitted against master branch instead as it's a major change?

@unkind unkind force-pushed the feature-lazy-loading-console-command branch from 852f8ba to 1b884b7 Compare March 20, 2015 21:50
@unkind
Copy link
Contributor Author

unkind commented Mar 20, 2015

@hhamon hi, I submit it against 2.7 on purpose, it is still possible to add new features, see #13946.

@unkind unkind force-pushed the feature-lazy-loading-console-command branch 2 times, most recently from 46d93eb to 33b6e0e Compare March 20, 2015 22:03
@unkind
Copy link
Contributor Author

unkind commented Mar 20, 2015

I've added CommandResolverInterface and some tests. However, tests are incomplete so far.

@unkind unkind changed the title [Console] Added support for lazy-loaded commands (CommandConfiguration as VO) [Console] Added support for lazy-loaded commands (with Command resolver) Mar 20, 2015
@unkind unkind force-pushed the feature-lazy-loading-console-command branch 3 times, most recently from acf06c4 to 6c7c91a Compare March 21, 2015 09:32
@unkind unkind force-pushed the feature-lazy-loading-console-command branch from 6c7c91a to d2acc15 Compare March 25, 2015 00:18
@unkind
Copy link
Contributor Author

unkind commented Mar 25, 2015

This PR is ready for review now.

@unkind unkind force-pushed the feature-lazy-loading-console-command branch from d2acc15 to 3c82276 Compare March 25, 2015 07:05
@unkind unkind force-pushed the feature-lazy-loading-console-command branch from 3c82276 to b08fc46 Compare March 25, 2015 07:07
stof
stof reviewed Apr 18, 2015
View reviewed changes
src/Symfony/Component/Console/Command/Command.php
* @param CommandResolverInterface $resolver
* @return Command
*/
final public static function registerLazyLoaded(CommandConfiguration $configuration, CommandResolverInterface $resolver)
Copy link
Member

Choose a reason for hiding this comment

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

this is not register a command, so the name looks wrong.

@stof
Copy link
Member

stof commented Apr 18, 2015

Note that I didn't call parent constructor in the GreetCommand on purpose. Given it's all about commands as services, I don't want to call parent constructor every time.

Not calling the parent constructor is wrong: it means you don't initialize the class properly, which can leave it in a broken state (leading to fatal errors).

And this is a radical change in the way the component works (btw, there are already other console packages splitting entirely the command definition from the command execution in case you prefer such architecture).
So I'm not a fan of this refactoring. It makes the component too different from the current one. As stated in #12063, the only real info we need to make the command lazy are the command names (i.e. the main name and aliases)

@stof stof mentioned this pull request Apr 18, 2015
Closed
@unkind
Copy link
Contributor Author

unkind commented Apr 18, 2015

Not calling the parent constructor is wrong: it means you don't initialize the class properly, which can leave it in a broken state (leading to fatal errors).

Sure, it's not clear from theoretical point of view at all. It's just workaround. But the root of these problems lies in SRP violation in Command.

So I'm not a fan of this refactoring

Me either.

@unkind unkind closed this Apr 18, 2015
@unkind unkind deleted the feature-lazy-loading-console-command branch April 18, 2015 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Console
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@unkind @hhamon @stof @aitboudad