Skip to content

[5.6] Execute an Artisan command using either its name or class#23764

Merged
taylorotwell merged 3 commits into
laravel:5.6from
KennedyTedesco:artisan-5.6
Apr 2, 2018
Merged

[5.6] Execute an Artisan command using either its name or class#23764
taylorotwell merged 3 commits into
laravel:5.6from
KennedyTedesco:artisan-5.6

Conversation

@KennedyTedesco

Copy link
Copy Markdown
Contributor

This implementation adds the ability to execute a command using class notation:

Artisan::call(FooCommand::class, [
    'user' => 1, '--queue' => 'default'
]);

Using the command name:

Artisan::call('foo:bar', [
    'user' => 1, '--queue' => 'default'
]);

@m1guelpf

m1guelpf commented Mar 31, 2018

Copy link
Copy Markdown
Contributor

@KennedyTedesco Maybe you should check if the class is a command and if it's registered...

@KennedyTedesco

Copy link
Copy Markdown
Contributor Author

@m1guelpf Hi, there! Thanks for your comment. :) I don't think it's necessary. It's a developer responsibility pass a valid command class or name, otherwise an error will be throw, anyway.

@laurencei

Copy link
Copy Markdown
Contributor

So this removes the requirement to register commands with the console? You could run anything from there?

What is the benefit? We already have auto-loading command logic that @themsaid did in 5.5 - why would we ever want to call the class directly?

@KennedyTedesco

Copy link
Copy Markdown
Contributor Author

Actually, it's not about auto-loading or anything. All those stuff remains the same. This implementation's just an easy-way to call a command using its class. It just instantiates the class to extract the command name. 😊

We already have this feature when scheduling a task:

https://laravel.com/docs/5.6/scheduling#scheduling-artisan-commands

$schedule->command(EmailsCommand::class, ['--force'])->daily();

@arjasco

arjasco commented Apr 1, 2018

Copy link
Copy Markdown
Contributor

Yeah, I’m for this. I personally would change it to.

if ($command instanceof Command) {
    $command = $this->laravel->make($command)->getName();
}

That way it’s definitely a Command subclass no need to check it it exists because it would if it’s an instance of it anyway..

@KennedyTedesco

Copy link
Copy Markdown
Contributor Author

Using class_exists() is the simplest way because if the developer inform a class, it need to be a command, otherwise, an error will be throw (checking it or not).

Changing the approach as you suggest, we would need to:

  • Check if $command is a class;
  • Check if $command is an instance of Command;
  • Finally, get its name.

I'm using this approach:

https://github.com/laravel/framework/blob/5.6/src/Illuminate/Console/Scheduling/Schedule.php#L77

@arjasco

arjasco commented Apr 1, 2018

Copy link
Copy Markdown
Contributor

Well if it’s an instance of Command it’s a class.. so the first check doesn’t need to happen.

I agree both being kept consistent if scheduling already functions that way. Just feels weird being able to give both of them a random class that has the method getName and it will work if the command name in question exists..

I would think making sure it’s actually a Command instance just seems sensible otherwise as said above some completely different instance could provide a valid command name which does work.. but syntactically doesn’t make sense.

@KennedyTedesco

Copy link
Copy Markdown
Contributor Author

$command instanceof Command it's not a valid statement because EmailsCommand::class is not a instance (yet).

So, in order to check this way, at the minimum we would need to:

if (class_exists($command) && resolve($command) instanceof Command) {
   // TODO
}

@arjasco

arjasco commented Apr 1, 2018

Copy link
Copy Markdown
Contributor

Ahh! Apologies, I missed that 🙈this is most likely best route then.

Unless is_subclass_of works for this case?

http://php.net/manual/en/function.is-subclass-of.php

@arjasco

arjasco commented Apr 1, 2018

Copy link
Copy Markdown
Contributor

I think is_subclass_of could be a good shout after finding this:

if (is_subclass_of($command, Command::class) &&

@KennedyTedesco

Copy link
Copy Markdown
Contributor Author

@arjasco This definitely do the job elegantly. Thank you so much.

If this PR gets merged i'll change that part on the Schedule too.

@KennedyTedesco KennedyTedesco changed the title [5.6] Execute an Artisan command using either it's name or class [5.6] Execute an Artisan command using either its name or class Apr 1, 2018
@taylorotwell taylorotwell merged commit f58c004 into laravel:5.6 Apr 2, 2018
@KennedyTedesco KennedyTedesco deleted the artisan-5.6 branch April 2, 2018 20:30
*/
public function test_artisan_call_invalid_command_name()
{
$this->artisan('foo:bars');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@KennedyTedesco this test freezes entirely on master branch. Can you fix it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@taylorotwell We would need to look a bit more carefully, because wasn't caused by my implementation. Actually, looks like a problem of symfony. On the dependencies of the 5.6 branch when we execute a command that doesn't exist it throws a CommandNotFoundException, but on master branch it just gets stuck.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@taylorotwell We can fix on our side by evaluating if the command really exists. What you think about it?

KennedyTedesco@bc20aaa

I can send a pull request if it's fine.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants