Allow queue jobs with batches

[dazza-dev]

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:

• Checked the codebase to ensure that your feature doesn't already exist.
• Take note of the contributing guidelines.
• Checked the pull requests to ensure that another person hasn't already submitted a fix.
• Added tests to ensure against regression.

[patrickbrouwers]

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

[patrickbrouwers]

By doing this, it will execute as a chain with in the batch right? It won't run parallel?

[dazza-dev]

That's right. This creates a batch and returns a PendingBatch.

[patrickbrouwers]

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

[dazza-dev]

Done

[patrickbrouwers]

See comment in trait

[dazza-dev]

I considered implementing this with a job middleware, but there are versions of Laravel that do not support this, so I discarded the idea.

[stale]

This bug report has been automatically closed because it has not had recent activity. If this is still an active bug, please comment to reopen. Thank you for your contributions.

[dazza-dev]

Hi @patrickbrouwers, please let me know if anything else is needed to finalize this.

[patrickbrouwers]

See my previous comment about the unit test.

[dazza-dev]

See my previous comment about the unit test.

Done

[keithbrink]

@dazza-dev I started using this in my project, and it works great for exports, but one issue I found was that the ExcelFake returns a PendingDispatch instead of a PendingBatch.

[dazza-dev]

Hi @keithbrink thanks for pointing that out! I haven't used ExcelFake before, so it would be really helpful if you could provide an example of your code where this issue occurs. That way, I can try to reproduce the problem on my end and work on a solution. Thanks in advance!

[keithbrink]

For sure! It would look something like this:

Excel::fake();

$pending_batch = Excel::queue(new BatchExport, 'filename');

// This will work without the fake, and fail with the fake
$pending_batch->catch(fn(Batch $batch) => \Log::info($batch->id));

Excel::assertQueued('filename');

[dazza-dev]

Hi @keithbrink, The issue has been resolved.

[keithbrink]

Brilliant, thank you.

One more comment here, there could be some tweaks to improve the usefulness of the batches. The jobs are currently executed in a chain.

The order should be:

1. QueueExport
2. ...AppendQueryToSheet jobs, in any order
3. CloseSheet

And the batch is being created something like this:

Bus::batch([
    [
        QueueExport
        ...AppendQueryToSheet (multiple)
        CloseSheet
   ]
]);

That makes it into a chain, so AppendQueryToSheet jobs cannot be run in parallel.

Using something like this would make them run in parallel:

$pending_batch = Bus::batch(...AppendQueryToSheet)->before(QueueExport)->then(CloseSheet)

However, it could be that AppendQueryToSheet cannot be run in parallel?

[patrickbrouwers]

I think depending on the user's use-case it could run in parallel. Perhaps we should indicate with a concern or method if the batch should be execute in parallel or sequence.

[patrickbrouwers]

Actually on second thought, that will give issues, as we re-open the file, it will overwrite itself when appending rows, I don't think that will work very well.
For imports it should be easier to run in parallel.

[dazza-dev]

Hi @patrickbrouwers, I hope you're doing well. I wanted to check if the PR I submitted is ready to be merged, or if there are any additional changes or improvements you'd like me to make. If there's anything else that needs to be done, please let me know so I can complete it as soon as possible.

[POWRFULCOW89]

Excellent work @dazza-dev, I was in the middle of implementing my own batched chunks workaround and this fits my use case perfectly. I would love to see this merged soon. 🙌

[henryortiz8]

@patrickbrouwers I’ve integrated this into my project, and it functions excellently for exports and imports, when its going to be merged?

[dazza-dev]

Hi @patrickbrouwers Please let me know if there’s anything else I need to adjust to get it merged. I’d be happy to make any necessary changes.

[patrickbrouwers]

I still have to dive into this (currently on holiday), but haven't gotten around to it. Will let you know. Thanks for your work on this again.

[ZRktty]

Hello @patrickbrouwers !
I was wondering if there has been any progress on this PR or if there’s anything blocking it from being merged.
Thanks, Zoltan