Skip to content
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

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

Merged
merged 3 commits into from
Apr 2, 2018
Merged

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

merged 3 commits into from
Apr 2, 2018

Conversation

KennedyTedesco
Copy link
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
Copy link
Contributor

m1guelpf commented Mar 31, 2018

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

@KennedyTedesco
Copy link
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
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
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
Copy link
Contributor

arjasco commented Apr 1, 2018

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
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
Copy link
Contributor

arjasco commented Apr 1, 2018

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
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
Copy link
Contributor

arjasco commented Apr 1, 2018

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
Copy link
Contributor

arjasco commented Apr 1, 2018

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

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

@KennedyTedesco
Copy link
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
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
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
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
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