-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
Conversation
@KennedyTedesco Maybe you should check if the class is a command and if it's registered... |
@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. |
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? |
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(); |
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.. |
Using Changing the approach as you suggest, we would need to:
I'm using this approach: https://github.com/laravel/framework/blob/5.6/src/Illuminate/Console/Scheduling/Schedule.php#L77 |
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. |
So, in order to check this way, at the minimum we would need to: if (class_exists($command) && resolve($command) instanceof Command) {
// TODO
} |
Ahh! Apologies, I missed that 🙈this is most likely best route then. Unless |
I think is_subclass_of could be a good shout after finding this:
|
@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. |
*/ | ||
public function test_artisan_call_invalid_command_name() | ||
{ | ||
$this->artisan('foo:bars'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
I can send a pull request if it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
This implementation adds the ability to execute a command using class notation:
Using the command name: