Skip to content
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.

Sign up for GitHub

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 #3974

Closed
wants to merge 5 commits into from
Closed

Allow queue jobs with batches #3974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
68b0193
ShouldBatch Concern
winkelco Aug 3, 2023
b6c2028
Import with batches
winkelco Aug 3, 2023
5383e48
Export with batches
winkelco Aug 3, 2023
0595d0a
[FIX] Temporary file creation on multi-server setups
winkelco Oct 29, 2023
3a1aec7
Merge pull request #1 from winkelco/winkelco-patch-1
winkelco Oct 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/ChunkReader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@

namespace Maatwebsite\Excel;

use Maatwebsite\Excel\Concerns\ShouldBatch;
use Illuminate\Bus\Queueable;
use Illuminate\Container\Container;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\PendingDispatch;
use Illuminate\Queue\InteractsWithQueue;
use Illuminate\Queue\Jobs\SyncJob;
use Illuminate\Support\Collection;
use Illuminate\Support\Facades\Bus;
use Maatwebsite\Excel\Concerns\ShouldQueueWithoutChain;
use Maatwebsite\Excel\Concerns\WithChunkReading;
use Maatwebsite\Excel\Concerns\WithEvents;
Expand Down Expand Up @@ -38,7 +40,7 @@ public function __construct(Container $container)
* @param WithChunkReading $import
* @param Reader $reader
* @param TemporaryFile $temporaryFile
* @return \Illuminate\Foundation\Bus\PendingDispatch|null
* @return \Illuminate\Foundation\Bus\PendingDispatch|\Illuminate\Bus\PendingBatch|null
*/
public function read(WithChunkReading $import, Reader $reader, TemporaryFile $temporaryFile)
{
Expand Down Expand Up @@ -93,6 +95,13 @@ public function read(WithChunkReading $import, Reader $reader, TemporaryFile $te

$jobs->push($afterImportJob);

// Check if the import class is batchable
if ($import instanceof ShouldBatch) {
return Bus::batch([
$jobs->toArray(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would create a batch with a chain, is that your intention? Would it make sense to add a concern to allow in parallel?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the concern is added so that the response is a PendingBatch and then you can add things like batch name, jobs at the end of the batch etc.

]);
}

if ($import instanceof ShouldQueue) {
return new PendingDispatch(
(new QueueImport($import))->chain($jobs->toArray())
Expand Down
7 changes: 7 additions & 0 deletions src/Concerns/ShouldBatch.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

namespace Maatwebsite\Excel\Concerns;

interface ShouldBatch
{
}
3 changes: 2 additions & 1 deletion src/Excel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Maatwebsite\Excel;

use Illuminate\Bus\PendingBatch;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\PendingDispatch;
use Illuminate\Support\Collection;
Expand Down Expand Up @@ -145,7 +146,7 @@ public function import($import, $filePath, string $disk = null, string $readerTy
$readerType = FileTypeDetector::detect($filePath, $readerType);
$response = $this->reader->read($import, $filePath, $readerType, $disk);

if ($response instanceof PendingDispatch) {
if ($response instanceof PendingDispatch || $response instanceof PendingBatch) {
return $response;
}

Expand Down
5 changes: 4 additions & 1 deletion src/Files/RemoteTemporaryFile.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

namespace Maatwebsite\Excel\Files;

use Illuminate\Support\Arr;

class RemoteTemporaryFile extends TemporaryFile
{
/**
Expand Down Expand Up @@ -94,7 +96,8 @@ public function delete(): bool
public function sync(): TemporaryFile
{
if (!$this->localTemporaryFile->exists()) {
touch($this->localTemporaryFile->getLocalPath());
$this->localTemporaryFile = resolve(TemporaryFileFactory::class)
->makeLocal(Arr::last(explode('/', $this->filename)));
}

$this->disk()->copy(
Expand Down
10 changes: 9 additions & 1 deletion src/Jobs/AfterImportJob.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@

namespace Maatwebsite\Excel\Jobs;

use Illuminate\Bus\Batchable;
use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
use Illuminate\Queue\InteractsWithQueue;
use Maatwebsite\Excel\Concerns\WithEvents;
use Maatwebsite\Excel\Events\ImportFailed;
use Maatwebsite\Excel\HasEventBus;
Expand All @@ -12,7 +15,7 @@

class AfterImportJob implements ShouldQueue
{
use Queueable, HasEventBus;
use Batchable, Queueable, HasEventBus, Dispatchable, ProxyFailures, InteractsWithQueue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Batchable trait doesn't exists < Laravel 8

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right. this would break compatibility with versions prior to 8

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no security patches for laravel 8 anymore and 9 neither in 3 weeks. you shouldn't encourage people to stay on insecure versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't endorse nor encourage it. But the package is currently in a situation that dropping support for older versions has an impact on users. Unfortunately the harsh truth looking at packagist installs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I hinted dropping support means nothing for those user, they are already outdated versions of everything, I doubt they ever update anything only running composer install, I'm sure they don't even upgrade security patches, so why would they update your library either?

By dropping support in a minor they can still install the older package without the new features if they want with no issues, you do not need to create a major to drop support of a dependency with composer, and even if you do want to bump the version, then what's the issue? Those old users still can't upgrade (Composer will only install the last compatible version) and they likely won't be missing on anything, they'll still have the features they've been using for 6 years or they already would have upgraded their laravel which is quite easy to do in most cases.

Upgrading a major just has more friction because all the rest of the users need to bump the required major version manually as well as opposed to just composer upgrade

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the majority of issues opened here are of people using older Laravel versions with the latest version of this package.
Features like queue batches are great, but I want to think it through carefully before releasing it to a lot of people. Having a package with a lot of users comes with a responsibility and less freedom. The last time I dropped older stuff I got huge backlash and way too many complaints. Not fun I can tell you. Therefore I decided to do a major release in the future which will - from that time on - only support the actively supported Laravel and PHP versions. I currently don't have time to finish the last bits of the planned 4.0 release, so any version dropping will also have to wait until then. I'm sorry if that's inconvenient to some, but that's also open source 😅


/**
* @var WithEvents
Expand All @@ -36,6 +39,11 @@ public function __construct($import, Reader $reader)

public function handle()
{
// Determine if the batch has been cancelled...
if ($this->batch()->cancelled()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would break if ShouldBatch isn't implemented and user would use the old queueing

return;
}

if ($this->import instanceof ShouldQueue && $this->import instanceof WithEvents) {
$this->reader->clearListeners();
$this->reader->registerListeners($this->import->registerEvents());
Expand Down
8 changes: 7 additions & 1 deletion src/Jobs/AppendDataToSheet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Maatwebsite\Excel\Jobs;

use Illuminate\Bus\Batchable;
use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
Expand All @@ -12,7 +13,7 @@

class AppendDataToSheet implements ShouldQueue
{
use Queueable, Dispatchable, ProxyFailures, InteractsWithQueue;
use Batchable, Queueable, Dispatchable, ProxyFailures, InteractsWithQueue;

/**
* @var array
Expand Down Expand Up @@ -73,6 +74,11 @@ public function middleware()
*/
public function handle(Writer $writer)
{
// Determine if the batch has been cancelled...
if ($this->batch()->cancelled()) {
return;
}

(new LocalizeJob($this->sheetExport))->handle($this, function () use ($writer) {
$writer = $writer->reopen($this->temporaryFile, $this->writerType);

Expand Down
8 changes: 7 additions & 1 deletion src/Jobs/AppendQueryToSheet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Maatwebsite\Excel\Jobs;

use Illuminate\Bus\Batchable;
use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
Expand All @@ -13,7 +14,7 @@

class AppendQueryToSheet implements ShouldQueue
{
use Queueable, Dispatchable, ProxyFailures, InteractsWithQueue;
use Batchable, Queueable, Dispatchable, ProxyFailures, InteractsWithQueue;

/**
* @var TemporaryFile
Expand Down Expand Up @@ -87,6 +88,11 @@ public function middleware()
*/
public function handle(Writer $writer)
{
// Determine if the batch has been cancelled...
if ($this->batch()->cancelled()) {
return;
}

(new LocalizeJob($this->sheetExport))->handle($this, function () use ($writer) {
$writer = $writer->reopen($this->temporaryFile, $this->writerType);

Expand Down
8 changes: 7 additions & 1 deletion src/Jobs/AppendViewToSheet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Maatwebsite\Excel\Jobs;

use Illuminate\Bus\Batchable;
use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
Expand All @@ -13,7 +14,7 @@

class AppendViewToSheet implements ShouldQueue
{
use Queueable, Dispatchable, InteractsWithQueue;
use Batchable, Queueable, Dispatchable, InteractsWithQueue;

/**
* @var TemporaryFile
Expand Down Expand Up @@ -68,6 +69,11 @@ public function middleware()
*/
public function handle(Writer $writer)
{
// Determine if the batch has been cancelled...
if ($this->batch()->cancelled()) {
return;
}

(new LocalizeJob($this->sheetExport))->handle($this, function () use ($writer) {
$writer = $writer->reopen($this->temporaryFile, $this->writerType);

Expand Down
10 changes: 9 additions & 1 deletion src/Jobs/CloseSheet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,18 @@

namespace Maatwebsite\Excel\Jobs;

use Illuminate\Bus\Batchable;
use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
use Illuminate\Queue\InteractsWithQueue;
use Maatwebsite\Excel\Concerns\WithEvents;
use Maatwebsite\Excel\Files\TemporaryFile;
use Maatwebsite\Excel\Writer;

class CloseSheet implements ShouldQueue
{
use Queueable, ProxyFailures;
use Batchable, Queueable, Dispatchable, ProxyFailures, InteractsWithQueue;

/**
* @var object
Expand Down Expand Up @@ -54,6 +57,11 @@ public function __construct($sheetExport, TemporaryFile $temporaryFile, string $
*/
public function handle(Writer $writer)
{
// Determine if the batch has been cancelled...
if ($this->batch()->cancelled()) {
return;
}

$writer = $writer->reopen(
$this->temporaryFile,
$this->writerType
Expand Down
9 changes: 8 additions & 1 deletion src/Jobs/QueueExport.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

namespace Maatwebsite\Excel\Jobs;

use Illuminate\Bus\Batchable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
use Illuminate\Queue\InteractsWithQueue;
use Maatwebsite\Excel\Concerns\WithMultipleSheets;
use Maatwebsite\Excel\Files\TemporaryFile;
use Maatwebsite\Excel\Jobs\Middleware\LocalizeJob;
Expand All @@ -12,7 +14,7 @@

class QueueExport implements ShouldQueue
{
use ExtendedQueueable, Dispatchable;
use Batchable, ExtendedQueueable, Dispatchable, InteractsWithQueue;

/**
* @var object
Expand Down Expand Up @@ -58,6 +60,11 @@ public function middleware()
*/
public function handle(Writer $writer)
{
// Determine if the batch has been cancelled...
if ($this->batch()->cancelled()) {
return;
}

(new LocalizeJob($this->export))->handle($this, function () use ($writer) {
$writer->open($this->export);

Expand Down
8 changes: 7 additions & 1 deletion src/Jobs/ReadChunk.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Maatwebsite\Excel\Jobs;

use Illuminate\Bus\Batchable;
use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Queue\InteractsWithQueue;
Expand All @@ -22,7 +23,7 @@

class ReadChunk implements ShouldQueue
{
use Queueable, HasEventBus, InteractsWithQueue;
use Batchable, Queueable, HasEventBus, InteractsWithQueue;

/**
* @var int
Expand Down Expand Up @@ -131,6 +132,11 @@ public function retryUntil()
*/
public function handle(TransactionHandler $transaction)
{
// Determine if the batch has been cancelled...
if ($this->batch()->cancelled()) {
return;
}

if (method_exists($this->import, 'setChunkOffset')) {
$this->import->setChunkOffset($this->startRow);
}
Expand Down
11 changes: 10 additions & 1 deletion src/Jobs/StoreQueuedExport.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,17 @@

namespace Maatwebsite\Excel\Jobs;

use Illuminate\Bus\Batchable;
use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
use Illuminate\Queue\InteractsWithQueue;
use Maatwebsite\Excel\Files\Filesystem;
use Maatwebsite\Excel\Files\TemporaryFile;

class StoreQueuedExport implements ShouldQueue
{
use Queueable;
use Batchable, Queueable, Dispatchable, InteractsWithQueue;

/**
* @var string
Expand All @@ -25,6 +28,7 @@ class StoreQueuedExport implements ShouldQueue
* @var TemporaryFile
*/
private $temporaryFile;

/**
* @var array|string
*/
Expand All @@ -49,6 +53,11 @@ public function __construct(TemporaryFile $temporaryFile, string $filePath, stri
*/
public function handle(Filesystem $filesystem)
{
// Determine if the batch has been cancelled...
if ($this->batch()->cancelled()) {
return;
}

$filesystem->disk($this->disk, $this->diskOptions)->copy(
$this->temporaryFile,
$this->filePath
Expand Down
16 changes: 14 additions & 2 deletions src/QueuedWriter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

namespace Maatwebsite\Excel;

use Maatwebsite\Excel\Concerns\ShouldBatch;
use Illuminate\Foundation\Bus\PendingDispatch;
use Illuminate\Support\Collection;
use Illuminate\Support\Facades\Bus;
use Maatwebsite\Excel\Concerns\FromCollection;
use Maatwebsite\Excel\Concerns\FromQuery;
use Maatwebsite\Excel\Concerns\FromView;
Expand Down Expand Up @@ -54,7 +56,7 @@ public function __construct(Writer $writer, TemporaryFileFactory $temporaryFileF
* @param string $disk
* @param string|null $writerType
* @param array|string $diskOptions
* @return \Illuminate\Foundation\Bus\PendingDispatch
* @return \Illuminate\Foundation\Bus\PendingDispatch|\Illuminate\Bus\PendingBatch
*/
public function store($export, string $filePath, string $disk = null, string $writerType = null, $diskOptions = [])
{
Expand All @@ -63,15 +65,25 @@ public function store($export, string $filePath, string $disk = null, string $wr

$jobs = $this->buildExportJobs($export, $temporaryFile, $writerType);

$queueExportJob = new QueueExport($export, $temporaryFile, $writerType);

$jobs->push(new StoreQueuedExport(
$temporaryFile,
$filePath,
$disk,
$diskOptions
));

// Check if the export class is batchable
if ($export instanceof ShouldBatch) {
return Bus::batch([
$jobs->prepend($queueExportJob)
->toArray(),
]);
}

return new PendingDispatch(
(new QueueExport($export, $temporaryFile, $writerType))->chain($jobs->toArray())
$queueExportJob->chain($jobs->toArray())
);
}

Expand Down