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

[11.x] Optimize PendingBatch@ensureJobIsBatchable #54485

Merged
merged 3 commits into from
Feb 6, 2025
Merged

[11.x] Optimize PendingBatch@ensureJobIsBatchable #54485

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39567bb
simplify PendingBatch@ensureJobIsBatchable; memoize the classes which…
cosmastech Feb 5, 2025
8851991
no need to recheck the batch jobs
cosmastech Feb 5, 2025
0dc7787
Update PendingBatch.php
taylorotwell Feb 6, 2025
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
15 changes: 12 additions & 3 deletions src/Illuminate/Bus/PendingBatch.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ class PendingBatch
*/
public $options = [];

/**
* Batches that have been checked to contain the Batchable trait.
*
* @var array<class-string, bool>
*/
protected static $batchableClasses = [];

/**
* Create a new pending batch instance.
*
Expand Down Expand Up @@ -92,14 +99,16 @@ protected function ensureJobIsBatchable(object|array $job): void
{
foreach (Arr::wrap($job) as $job) {
if ($job instanceof PendingBatch) {
$this->ensureJobIsBatchable($job->jobs->all());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@josepostiga Do you think it make sense to remove this check here? If it's a PendingBatch, that all of the jobs added to it should've had this method called for each job.

There is the case where a user creates a PendingBatch and then modifies the $jobs Collection manually ($pendingBatch->jobs->push(new SomeJobThatIsNotDispatchable)), but if we want to guard against this, we would have to ensure this check happens before we write to the repository, probably in PendingBatch@store().

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, that actually makes sense, so I agree we could drop the check here and assume jobs have been sanitized before.

That case you refer, though, worries me a little. It leaves a "known" inconsistency in the framework. Do you know how hard would it be to re-architect it so that we can also cover for that? I can look into it later, though.


return;
}

if (! in_array(Batchable::class, class_uses_recursive($job))) {
if (! (static::$batchableClasses[$job::class] ?? false) && ! in_array(Batchable::class, class_uses_recursive($job))) {
Copy link
Contributor

@shaedrich shaedrich Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

While it might not possible with class_uses_recursive, this could be taken one step further, by implementing something similar to class_uses_recursive: Caching all classes in the recursive chain 🤔

Copy link
Contributor

@shaedrich shaedrich Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Something along the lines of this: 454eb0d...0000530

Copy link
Contributor

@shaedrich shaedrich Feb 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

or 454eb0d...1990987 (1990987)

static::$batchableClasses[$job::class] = false;

throw new RuntimeException(sprintf('Attempted to batch job [%s], but it does not use the Batchable trait.', $job::class));
}

static::$batchableClasses[$job::class] = true;
}
}

Expand Down
18 changes: 18 additions & 0 deletions tests/Bus/BusPendingBatchTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,4 +231,22 @@ public function test_it_throws_exception_if_batched_job_is_not_batchable(): void

new PendingBatch(new Container, new Collection([$nonBatchableJob]));
}

public function test_it_throws_an_exception_if_batched_job_contains_batch_with_nonbatchable_job(): void
{
$this->expectException(RuntimeException::class);

$container = new Container;
new PendingBatch(
$container,
new Collection(
[new PendingBatch($container, new Collection([new BatchableJob, new class {}]))]
)
);
}
}

class BatchableJob
{
use Batchable;
}