laravel / ideas

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

[Proposal] Dispatch eloquent collections #1000

Open alexbowers opened 6 years ago

alexbowers commented 6 years ago

A fairly common part of my applications is to get a collection from Eloquent, and then to iterate over each of the records and dispatch a job with that record as the parameter:

Something like this:

$customers = \App\Foundation\Customer::live()->get();

$customers->each(function ($customer) {
    dispatch(new \App\Foundation\Jobs\SitemapGenerate($customer));
});

I am proposing an alternative:

$customers = \App\Foundation\Customer::live()->get();

$customers->dispatch(\App\Foundation\Jobs\SitemapGenerate::class);

Will automatically pass customer to the first parameter. If other parameters are required, i'd suggest going back to the existing method, or perhaps dispatch can accept multiple parameters to pass through?

Patryk27 commented 6 years ago

Seems like breaking SRP - suddenly a regular collection would have a dependency on whole dispatching subsystem and that's too much IMHO.

A helper method (like dispatch_for_each($customers, MyEvent::class);) could be provided though.

RemiCollin commented 6 years ago

Why not pass directly the collection to the job ?

Patryk27 commented 6 years ago

@RemiCollin: I think OP wants to have n small jobs (1 job = 1 model) instead of a single, big one.

mfn commented 6 years ago

This is a regular use-case I'm having too, but IMHO the suggested code:

$customers->dispatch(\App\Foundation\Jobs\SitemapGenerate::class);

is too much magic. It's absolutely not clear what it would do IMHO => 👎

Patryk27 commented 6 years ago

Also, to consider: what should happen when dispatching any of the event from the list is interrupted?

a) an exception should be thrown and everything should be rolled back (like no dispatching happened, if possible),

b) an exception should be thrown without any rolling-back (but what happens if someone wanted to restore the dispatching from that point when exception happened?),

c) what can be dispatched, should be dispatched and eventually all the exceptions should be recorded into a returned array,

d) (...)

mfn commented 6 years ago

everything should be rolled back (like no dispatching happened, if possible),

This isn't possible. The job might already have been picked up. There's nothing like transaction safety in a distributed environment 🤷‍♀️

arjasco commented 6 years ago

Completely understand this is a proposal and I often perform tasks like this.

I do find your first example more readable though. However you already have the power you require as Collection is macroable.

Add this to a service provider boot method:

Collection::macro('dispatch', function ($job) {
    $this->mapInto($job)->each('dispatch');
});

Your second example will now work.

Or forget the marco all together and just do:

$customers->mapInto(SitemapGenerate::class)->each('dispatch');

Which is actually very readable what is happening.

Maybe you need to retain the collection of customers for a second task??

I added the tap method to collection ages ago for this very reason, to retain your customers if you wanted to do something with the collection after.

$customers->tap(function ($customers) {
    $customers->mapInto(SitemapGenerate::class)->each('dispatch');
})->someMethodOfYourChoice()->etc()->etc()

It's all just syntax sugar.