diff --git a/CHANGELOG.md b/CHANGELOG.md index 21890d2d8d6..0427e324130 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,7 @@ - Removed `CraftCms\Cms\Plugin\Plugin::registerPlugin()`. `register()` should be used instead. ([#19263](https://github.com/craftcms/cms/pull/19263)) - Fixed a bug where the legacy `yii\web\JqueryAsset` wasn’t resolving properly. ([#19264](https://github.com/craftcms/cms/pull/19264)) - Fixed a bug where bulk entry moves could assign entries to sections that didn’t support their entry types. ([#19267](https://github.com/craftcms/cms/pull/19267)) +- Fixed a bug where queued resaves ignored offset and limit criteria or accepted non-positive batch sizes. ([#19271](https://github.com/craftcms/cms/pull/19271)) - Fixed bugs that could prevent authored content from being deleted or reassigned safely when deleting users. ([#19273](https://github.com/craftcms/cms/pull/19273)) - Fixed an issue where disabled and archived user accounts could still authenticate. ([#19265](https://github.com/craftcms/cms/pull/19265)) - Fixed issues with `craft:users:set-password` password validation, exit statuses, and session invalidation. ([#19272](https://github.com/craftcms/cms/pull/19272)) diff --git a/src/Element/Commands/Resave/ResaveCommand.php b/src/Element/Commands/Resave/ResaveCommand.php index f2ee355e7c0..19df531c071 100644 --- a/src/Element/Commands/Resave/ResaveCommand.php +++ b/src/Element/Commands/Resave/ResaveCommand.php @@ -120,6 +120,12 @@ public static function normalizeTo(?string $to): callable */ protected function validateResaveOptions(): bool { + if ((int) $this->option('batch-size') < 1) { + $this->components->error('--batch-size must be at least 1.'); + + return false; + } + // Normalize drafts/provisionalDrafts/revisions foreach (['drafts', 'provisional-drafts', 'revisions'] as $optionName) { $value = $this->optionalOption($optionName); diff --git a/src/Element/Jobs/ResaveElements.php b/src/Element/Jobs/ResaveElements.php index 07289e523c6..869b7456e47 100644 --- a/src/Element/Jobs/ResaveElements.php +++ b/src/Element/Jobs/ResaveElements.php @@ -43,10 +43,12 @@ public function __construct( public bool $ifEmpty = false, public bool $ifInvalid = false, public bool $touch = false, - public int $batchSize = 100, + int $batchSize = 100, protected ?string $description = null, ) { parent::__construct(); + + $this->batchSize = $batchSize; } protected function processElement(ElementInterface $element): void diff --git a/src/Queue/BatchedElementJob.php b/src/Queue/BatchedElementJob.php index 873ec76d3a4..b72293f35f2 100644 --- a/src/Queue/BatchedElementJob.php +++ b/src/Queue/BatchedElementJob.php @@ -45,14 +45,27 @@ protected function getQuery(): Builder { /** @var ElementQuery $query */ $query = $this->elementType::find()->orderBy('elements.id'); + $criteria = $this->criteria; + unset($criteria['offset'], $criteria['limit']); - if (! empty($this->criteria)) { - Typecast::configure($query, $this->criteria); + if (! empty($criteria)) { + Typecast::configure($query, $criteria); } return $query; } + #[\Override] + protected function queryOffset(): int + { + return (int) ($this->criteria['offset'] ?? 0); + } + + protected function queryLimit(): ?int + { + return isset($this->criteria['limit']) ? (int) $this->criteria['limit'] : null; + } + protected function processItem(mixed $item): void { $this->processElement($item); diff --git a/src/Queue/BatchedJob.php b/src/Queue/BatchedJob.php index 75bc27bbb2e..11fb49822ff 100644 --- a/src/Queue/BatchedJob.php +++ b/src/Queue/BatchedJob.php @@ -8,6 +8,7 @@ use CraftCms\Cms\Support\PHP; use Illuminate\Contracts\Database\Query\Builder; use Illuminate\Queue\Jobs\SyncJob; +use InvalidArgumentException; use Override; use function CraftCms\Cms\t; @@ -28,7 +29,15 @@ abstract class BatchedJob extends Job /** * The number of items that should be processed in a single batch. */ - public int $batchSize = 100; + public int $batchSize = 100 { + set { + if ($value < 1) { + throw new InvalidArgumentException('Batch size must be at least 1.'); + } + + $this->batchSize = $value; + } + } /** * The index of the current batch (starting with 0). @@ -59,7 +68,10 @@ public function __sleep(): array public function handle(): void { - $items = $this->getQuery()->offset($this->itemOffset)->limit($this->batchSize)->get(); + $items = $this->getQuery() + ->offset($this->queryOffset() + $this->itemOffset) + ->limit(min($this->batchSize, $this->totalItems() - $this->itemOffset)) + ->get(); $memoryLimit = PHP::sizeToBytes(ini_get('memory_limit')); $startMemory = $memoryLimit !== -1 ? memory_get_usage() : null; @@ -175,13 +187,33 @@ protected function beforeBatch(): void {} */ protected function afterBatch(): void {} + /** + * Returns the initial query offset before batch pagination is applied. + */ + protected function queryOffset(): int + { + return 0; + } + + /** + * Returns the maximum number of items to process across all batches. + */ + protected function queryLimit(): ?int + { + return null; + } + /** * Returns the total number of items across all batches. */ final protected function totalItems(): int { if (! isset($this->totalItems)) { - $this->totalItems = $this->getQuery()->count(); + $this->totalItems = max($this->getQuery()->count() - $this->queryOffset(), 0); + + if ($this->queryLimit() !== null) { + $this->totalItems = min($this->totalItems, $this->queryLimit()); + } } return $this->totalItems; diff --git a/tests/Feature/Element/Commands/ResaveCommandsTest.php b/tests/Feature/Element/Commands/ResaveCommandsTest.php index 03d1b5cba04..9c54fc270b4 100644 --- a/tests/Feature/Element/Commands/ResaveCommandsTest.php +++ b/tests/Feature/Element/Commands/ResaveCommandsTest.php @@ -160,6 +160,19 @@ && $job->toDefault === true); }); +it('rejects non-positive queue batch sizes', function (int $batchSize) { + Queue::fake(); + + $this->artisan("craft:resave:entries --queue --batch-size=$batchSize") + ->expectsOutputToContain('--batch-size must be at least 1.') + ->assertExitCode(1); + + Queue::assertNothingPushed(); +})->with([ + 'zero' => 0, + 'negative' => -1, +]); + it('filters users by group', function () { $group = UserGroup::factory()->create(['handle' => 'staff']); $groupedUser = User::factory()->createElement(['fullName' => 'Grouped User']); diff --git a/tests/Feature/Queue/BatchedElementJobTest.php b/tests/Feature/Queue/BatchedElementJobTest.php index fa12423d11b..61116ccbf75 100644 --- a/tests/Feature/Queue/BatchedElementJobTest.php +++ b/tests/Feature/Queue/BatchedElementJobTest.php @@ -68,6 +68,40 @@ ->and($job->processedElementIds)->toContain($targetEntry->id); }); +it('respects offset and limit criteria across batches', function () { + $entryIds = Entry::factory() + ->count(5) + ->create() + ->pluck('id') + ->sort() + ->values() + ->all(); + + Queue::fake(); + + $job = new TestBatchedElementJob( + EntryElement::class, + [ + 'id' => $entryIds, + 'offset' => 1, + 'limit' => 3, + ], + ); + $job->batchSize = 2; + + $job->handle(); + + expect($job->processedElementIds)->toBe(array_slice($entryIds, 1, 2)); + + Queue::assertPushed(TestBatchedElementJob::class, function (TestBatchedElementJob $nextJob) use ($entryIds) { + $nextJob->handle(); + + expect($nextJob->processedElementIds)->toBe(array_slice($entryIds, 1, 3)); + + return true; + }); +}); + it('spawns next batch for multi-batch element jobs', function () { // This test verifies that BatchedElementJob correctly inherits // the batch spawning behavior from BatchedJob. diff --git a/tests/Feature/Queue/BatchedJobTest.php b/tests/Feature/Queue/BatchedJobTest.php index fe188ad8a9c..d344f178ded 100644 --- a/tests/Feature/Queue/BatchedJobTest.php +++ b/tests/Feature/Queue/BatchedJobTest.php @@ -12,6 +12,16 @@ expect($job->batchSize)->toBe(100); }); +it('rejects non-positive batch sizes', function (int $batchSize) { + $job = new TestBatchedJob; + + expect(fn () => $job->batchSize = $batchSize) + ->toThrow(InvalidArgumentException::class, 'Batch size must be at least 1.'); +})->with([ + 'zero' => 0, + 'negative' => -1, +]); + it('starts at batch index 0', function () { $job = new TestBatchedJob; diff --git a/yii2-adapter/legacy/console/controllers/ResaveController.php b/yii2-adapter/legacy/console/controllers/ResaveController.php index 2621eae0e7f..8a2a7471f19 100644 --- a/yii2-adapter/legacy/console/controllers/ResaveController.php +++ b/yii2-adapter/legacy/console/controllers/ResaveController.php @@ -483,6 +483,12 @@ public function hasTheFields(FieldLayout $fieldLayout): bool */ public function resaveElements(string $elementType, array $criteria = []): int { + if ($this->batchSize < 1) { + $this->stderr('--batch-size must be at least 1.' . PHP_EOL, Console::FG_RED); + + return ExitCode::UNSPECIFIED_ERROR; + } + $criteria += $this->_baseCriteria(); if ($this->queue) { diff --git a/yii2-adapter/tests-laravel/Console/LegacyResaveCommandsTest.php b/yii2-adapter/tests-laravel/Console/LegacyResaveCommandsTest.php index 3c0b02c9487..b25b9f4e9bc 100644 --- a/yii2-adapter/tests-laravel/Console/LegacyResaveCommandsTest.php +++ b/yii2-adapter/tests-laravel/Console/LegacyResaveCommandsTest.php @@ -11,6 +11,7 @@ use CraftCms\Cms\Tests\TestCase; use CraftCms\Yii2Adapter\DeprecatedConcepts; use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Queue; use Illuminate\Support\Str; uses(TestCase::class); @@ -75,3 +76,20 @@ ->and(Category::find()->id($categoryElementId)->one()?->title) ->toBe('Category Title'); }); + +it('rejects non-positive queue batch sizes', function(int $batchSize) { + Queue::fake(); + + $controller = new ResaveController('resave', Craft::$app); + $controller->queue = true; + $controller->batchSize = $batchSize; + + $exitCode = $controller->resaveElements(Category::class); + + expect($exitCode)->toBe(1); + + Queue::assertNothingPushed(); +})->with([ + 'zero' => 0, + 'negative' => -1, +]);