laravel / ideas

Issues board used for Laravel internals discussions.
939 stars 28 forks source link

Self scheduled commands #1876

Open freekmurze opened 5 years ago

freekmurze commented 5 years ago

Right now the way to schedule a command is by hardcoding it in the schedule property of the kernel.

It would be cool if commands would be able to schedule themselves. It could work something like this:

class MyCoolCommand extends Command implements SelfScheduled
{
   // ...

   public function schedule(Schedule $schedule): void {
      // optionally you could put some logic here

      $schedule->dailyAt('2:00');
   }
}
nunomaduro commented 5 years ago

I would not mind of implementing this, because I have this feature on Laravel Zero.

mpociot commented 5 years ago

I feel like this would do more harm than good. This would basically allow auto discovered packages to perform tasks in the background without you knowing of it.

JackEllis commented 5 years ago

IMO it's not the responsibility of the command to know when it should be run.

mrk-j commented 5 years ago

I feel like this would do more harm than good. This would basically allow auto discovered packages to perform tasks in the background without you knowing of it.

Maybe only allow self scheduling for commands that are loaded using the the load() method? But I think @freekmurze wants this for a package, is it an option to create some kind of whitelist? Although that takes the 'magic' out of this idea.

freekmurze commented 5 years ago

@mpociot I understand your concern but, even without this feature, any package can already wreck a lot of havoc.

freekmurze commented 5 years ago

@jackellis It would be optional, so you could still use the current way of doing things.

mpociot commented 5 years ago

Yeah, I agree - but a simple composer update could suddenly pull in a lot of auto-scheduled commands that you do not want (or need) - and you have no insight into this other than referring to (hopefully curated) package changelogs.

My main concern is that it allows code being pulled in, scheduled and run via CLI automatically - without me noticing it.

Lloople commented 5 years ago

As long as we have some kind of php artisan schedule:list I like this idea 👍

Miguel-Serejo commented 5 years ago

Maybe a whitelist of auto-schedulable namespaces could work? This could be done through configuration, so you don't actually have to touch kernel.php to change scheduled tasks.

andrewnclark commented 5 years ago

The levels of automation that your concerned about can already be achieved through composer due to auto registering of providers using something akin to:

$this->app->booted(function () {
    $schedule = app(Schedule::class);
    $schedule->command('artisan:command')->dailyAt('02:30');
});

As an optional feature, I don't see any harm in the proposal. I have used automated scheduling as above in various projects so I have a use case for this.

I do like the idea of a schedule:list though @Lloople!

nunomaduro commented 5 years ago

@mpociot That's already the case. You can already get the scheduler instance out of the service container and schedule tasks from the package itself. Also like the idea of having a art schedule:list command.

Lloople commented 5 years ago

Maybe also an option to disable commands auto discovery would be useful, so you can turn it off completely and write them in your kernel if you want to be sure what’s scheduled and when

crnkovic commented 5 years ago

Yeah technically every library can run shell commands or anything worse, but this would be too much magic in my opinion. You would have no idea what is running in the background. Right now you can simply visit the Kernel.php and be sure about what’s scheduled. Changing frequencies at which commands run is much simpler the way it is.

JayBizzle commented 5 years ago

schedule:list has been proposed a few times https://github.com/laravel/framework/pulls?q=is%3Apr+sort%3Aupdated-desc+schedule%3Alist+is%3Aclosed

mpociot commented 5 years ago

Yeah, @nunomaduro and @andrewnclark - I agree, but this would make the whole process even simpler and more people would use this.

@freekmurze what is your main idea behind this? Which specific command do you want to have automatically scheduled?

This would still need some kind of white-list though, don't you think? I really don't like the idea of having packages automatically scheduling commands simply by performing a composer update. And yeah, I get that this can be done already - but this is an invitation to do it.

andrewnclark commented 5 years ago

That's a valid point @mpociot . I use this stuff in select use cases with private packages where I know the consumer and the package well. It might not be best suited to promotion and use in the 'open market' so to speak.

crnkovic commented 5 years ago

This would still need some kind of white-list though, don't you think?

And this is the Console kernel class itself. You explicitly whitelist what you want to run.

I have to agree with @mpociot.

mpociot commented 5 years ago

@crnkovic yeah that was my point as well :)

ollieread commented 5 years ago

In what situation would a command need to schedule itself?

A command should perform its task, and only its task. The time at which the command should be ran isn't its concern. Scheduling would be the responsibility of the implementation, not the command.

@mpociot makes a good point, though if a package developer really wants to do this, they can do it without this implementation.

Keep it simple, keep it separate.

freekmurze commented 5 years ago

@mpociot

Yeah, I agree - but a simple composer update could suddenly pull in a lot of auto-scheduled commands that you do not want (or need)

I would consider adding an auto-scheduled command in an update a breaking change / dick move, really.

People that want to cause harm can already put unexpected code in the service provider of an update to a package. Any package update could potentially destroy your entire environment.

Self scheduling commands comes with great power, and with great power comes great responsability (another uncle once said). I kinda trust fellow devs to do the right thing.

davzie commented 5 years ago

Please don't accept this idea into Laravel. I can't have my blood pressure rising because I need to be more worried about updating my composer dependancies. I like my wife and kids a lot and this will give me a nervous breakdown and we'll end up homeless blowing homeless dudes for cash. Choose wisely, for my family's sake.

freekmurze commented 5 years ago

@mpociot about the use case, I was talking to Dries about his Secret Project™ the other day, he was talking about how it could be helpfull if commands could be scheduled in a more dynamic way, so I come up with this idea of enabling that.

spresnac commented 5 years ago

I have to agree with @mpociot ... it feels like opening pandoras box to projects. And i would like a art schedule:list command ...

freekmurze commented 5 years ago

Thanks all for the civil discussion, whatever the result of this idea / brain fart will be. 👍

driesvints commented 5 years ago

So the idea behind this is that this would be a cool way to let an end user of an app to determine the scheduled time instead of the developer. Imagine if I want to schedule a message to be sent at a specific time as a user I could say something like "next tuesday", it would be cool if that "schedule" is passed to the job and then immediately hooked into the scheduler of Laravel.

The problem here however is that there's no persisting with the scheduler. There's no way for the scheduler to "save" that job to be dispatched later. So I'm not sure if this would ever work in practice. You could combine this with queueing or a "waiting list" in the database but that raises other problems.

@freekmurze thanks for bringing this up 👍

stefanzweifel commented 5 years ago

I remember this being a thing in indatus/dispatcher back in the Laravel 4 days.

The schedule:list-command would definitely be a big plus for me. Working in bigger apps with lots of scheduled commands it sometimes can be a bit messy to get an overview of what runs when.

SelfScheduled commands: I personally prefer the current way of registering them manually … but why not both 🤷‍♂

jamesburrow23 commented 5 years ago

We recently moved our schedule commands into their own table in our database then updated the kernal to check that table for which commands should run. This allows us to enable / disable commands without touching the code in our production environment. Another advantage of doing this is that we can adjust the timing dynamically as well by storing the cron expression in a column instead of inside the kernal. It's brought better visibility and functionality to our scheduled tasks

JackEllis commented 5 years ago

@jamesburrow23 This is how I'd go about achieving what @driesvints is after.

sandervanhooft commented 5 years ago

So the idea behind this is that this would be a cool way to let an end user of an app to determine the scheduled time instead of the developer. Imagine if I want to schedule a message to be sent at a specific time as a user I could say something like "next tuesday", it would be cool if that "schedule" is passed to the job and then immediately hooked into the scheduler of Laravel.

The problem here however is that there's no persisting with the scheduler. There's no way for the scheduler to "save" that job to be dispatched later. So I'm not sure if this would ever work in practice. You could combine this with queueing or a "waiting list" in the database but that raises other problems.

Essentially you're looking for an enhanced scheduler that fits your specific use case(s). I think it could make use of Laravel's scheduler, or perhaps replace/extend it. But I disagree the best place for this logic is in the command itself. The command is about passing the instruction to execute some action. The timing for that command should be external imho.

Perhaps the auto binding of the events and listeners is inspiration here? Separate the Command and the CommandSchedule?

sandervanhooft commented 5 years ago

And let the CommandSchedule point to the Command. I think you could use some dynamic scheduling in that CommandSchedule class (seems that's what @driesvints is looking for)

sandervanhooft commented 5 years ago

We recently moved our schedule commands into their own table in our database then updated the kernal to check that table for which commands should run. This allows us to enable / disable commands without touching the code in our production environment. Another advantage of doing this is that we can adjust the timing dynamically as well by storing the cron expression in a column instead of inside the kernal. It's brought better visibility and functionality to our scheduled tasks

Heavy stuff @jamesburrow23 . Could make a Horizon-like package someday :).

jamesburrow23 commented 5 years ago

@sandervanhooft was actually a lot simpler than you might think. I agree that a package would be quite useful though as we added error tracking, logs, dashboard etc.

deleugpn commented 5 years ago

There's a massive amount of comments on the foundation of The command shouldn't know about it's own schedule, which I think miss a lot of the point of Laravel.

We can go on, but Laravel is full of things that know how to handle themselves. It's part of Laravel. I feel like it would be much more productive to let go of battling against Laravel doing this and focus on why or why not this should be implemented despite your philosophy about whether things should know how to thing themselves.

jamesburrow23 commented 5 years ago

@deleugpn I think the concern here is that historically a command didn't run on the scheduler unless you explicitly registered it in the kernal. While this can be a pain it's also reassuring that you know what's firing when your cron job runs. The argument could also be made that sometimes Laravel's convenience can also add ambiguity to your code if you don't know where something happened, i,e I just created a model and there are 50 events listening to that and then they all do things unrelated to what I'm doing that I might not have taken into account. While this can be avoided with good coding practices I've worked with many developers who have abused the convenience that Laravel provided them.

JayBizzle commented 5 years ago

Heavy stuff @jamesburrow23 . Could make a Horizon-like package someday :).

@sandervanhooft Already exists https://github.com/codestudiohq/laravel-totem 👍

sandervanhooft commented 5 years ago

😍

drbyte commented 5 years ago

I'm neither for nor against, but as I read through this discussion I note there have been some good suggestions that can mitigate concerns:

I like the combination of:

Maybe whitelisting isn't the right final name, but Laravel already auto-registers things based on Convention over Configuration, such as auto-registering of Commands do now. So, just like auto-registering of Commands, Laravel could only auto-schedule CommandSchedule classes that sit inside that whitelisted namespace. The point here is it forces visibility in VCS, before moving to production: This way if a package wants to publish a specific schedule for a command it has to create the CommandSchedule class for it somewhere in the app's whitelisted namespace. Way more visible that way.

The scheduler:list thing would definitely be beneficial.

Again, I'm not arguing for or against. Just pointing out that the idea isn't that much of a stretch from what Laravel already does.

sandervanhooft commented 5 years ago

@drbyte Agree, scheduler:list is a no-regret move.