-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Allow queue jobs with batches #4127
base: 3.1
Are you sure you want to change the base?
Conversation
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.
I like what you did here. It seems it supports older versions and no breaking changes.
Some small remarks as comments.
Could you check if you could add a unit test for ShouldBatch interface like the ShouldQueue . Would be great to have in place. Those tests can be skipped if the Illuminate\Bus\Batchable
trait doesn't exist
// Check if the import class is batchable | ||
if ($import instanceof ShouldBatch) { | ||
return Bus::batch([ | ||
$jobs->toArray(), |
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.
By doing this, it will execute as a chain with in the batch right? It won't run parallel?
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.
That's right. This creates a batch and returns a PendingBatch.
} | ||
} else { | ||
trait BatchableTrait | ||
{ |
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.
I think we can add the methods we need else where like batchCancelled here as well, so we don't have to do a method_exist check there. We can return false here
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.
Done
src/Jobs/AfterImportJob.php
Outdated
@@ -57,6 +59,13 @@ public function setDependencies(Collection $jobs) | |||
|
|||
public function handle() | |||
{ | |||
// Determine if the batch has been cancelled... | |||
if (method_exists($this, 'batchCancelled')) { |
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.
See comment in trait
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.
I considered implementing this with a job middleware, but there are versions of Laravel that do not support this, so I discarded the idea.
…checks for method existence.
1️⃣ Why should it be added? What are the benefits of this change?
Allow import and export files with batches (New laravel feature for job batching).
This also works for Laravel < 8
2️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.
No
3️⃣ Does it include tests, if possible?
No
4️⃣ Any drawbacks? Possible breaking changes?
No
5️⃣ Mark the following tasks as done: