From eea8ad811190a2a50e1d3520bcadfcae651e0f85 Mon Sep 17 00:00:00 2001 From: lukmzig Date: Thu, 30 Apr 2026 13:11:17 +0200 Subject: [PATCH 1/3] improve element delete processing --- .../Messenger/Handler/BatchDeleteHandler.php | 85 ++-- .../Handler/ElementDeleteHandler.php | 97 ++-- .../Service/ExecutionEngine/DeleteService.php | 66 +-- .../DeleteServiceInterface.php | 4 - src/ExecutionEngine/Util/StepConfig.php | 3 + .../ExecutionEngine/DeleteServiceTest.php | 426 ++++++++++++++++++ 6 files changed, 580 insertions(+), 101 deletions(-) create mode 100644 tests/Unit/Element/Service/ExecutionEngine/DeleteServiceTest.php diff --git a/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/BatchDeleteHandler.php b/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/BatchDeleteHandler.php index b411dbd54..4f2418bb1 100644 --- a/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/BatchDeleteHandler.php +++ b/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/BatchDeleteHandler.php @@ -18,15 +18,17 @@ use Pimcore\Bundle\StaticResolverBundle\Models\User\UserResolverInterface; use Pimcore\Bundle\StudioBackendBundle\Element\ExecutionEngine\AutomationAction\Messenger\Messages\BatchDeleteMessage; use Pimcore\Bundle\StudioBackendBundle\Element\Service\ElementDeleteServiceInterface; -use Pimcore\Bundle\StudioBackendBundle\Element\Service\ExecutionEngine\DeleteServiceInterface; use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\AutomationAction\AbstractHandler; use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\Config; +use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\StepConfig; use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\Trait\HandlerProgressTrait; use Pimcore\Bundle\StudioBackendBundle\Mercure\Service\PublishServiceInterface; use Pimcore\Bundle\StudioBackendBundle\Mercure\Service\UserTopicServiceInterface; use Pimcore\Bundle\StudioBackendBundle\Util\Trait\ElementProviderTrait; use Pimcore\Model\Element\ElementInterface; +use Pimcore\Model\UserInterface; use Symfony\Component\Messenger\Attribute\AsMessageHandler; +use function count; /** * @internal @@ -67,44 +69,71 @@ public function __invoke(BatchDeleteMessage $message): void )); } - $type = $this->extractConfigFieldFromJobStepConfig( + $elementType = $this->extractConfigFieldFromJobStepConfig( $message, - DeleteServiceInterface::ELEMENT_TYPE_TO_DELETE + StepConfig::ELEMENT_TYPE_TO_BATCH_DELETE->value ); - $id = $this->extractConfigFieldFromJobStepConfig($message, DeleteServiceInterface::ELEMENT_TO_DELETE); - $parentElement = $this->elementService->getElementById($this->getCoreElementType($type), $id); + $items = $this->extractConfigFieldFromJobStepConfig( + $message, + StepConfig::ITEMS_TO_BATCH_DELETE->value + ); + $totalItems = count($items); + $stepName = $this->getJobStep($message)->getName(); - if ($parentElement instanceof ElementInterface) { - try { - $this->elementDeleteService->processBatchDelete($parentElement, $user, $type); - } catch (Exception $exception) { - $this->abort($this->getAbortData( - Config::ELEMENT_BATCH_DELETE_FAILED_MESSAGE->value, - [ - 'type' => $parentElement->getType(), - 'id' => $parentElement->getId(), - 'message' => $exception->getMessage(), - ], - )); + foreach ($items as $elementId) { + $element = $this->elementService->getElementById( + $this->getCoreElementType($elementType), + $elementId + ); + + if ($element instanceof ElementInterface) { + $this->deleteBatchElement($element, $user, $elementType); } + + $this->updateProgress( + $this->publishService, + $this->userTopicService, + $jobRun, + $stepName, + $totalItems, + $totalItems, + ); } + } - $this->updateProgress( - $this->publishService, - $this->userTopicService, - $jobRun, - $this->getJobStep($message)->getName() - ); + /** + * @throws Exception + */ + private function deleteBatchElement( + ElementInterface $element, + UserInterface $user, + string $elementType + ): void { + try { + $this->elementDeleteService->processBatchDelete($element, $user, $elementType); + } catch (Exception $exception) { + $this->abort($this->getAbortData( + Config::ELEMENT_BATCH_DELETE_FAILED_MESSAGE->value, + [ + 'type' => $element->getType(), + 'id' => $element->getId(), + 'message' => $exception->getMessage(), + ], + )); + } } protected function configureStep(): void { - $this->stepConfiguration->setRequired(DeleteServiceInterface::ELEMENT_TO_DELETE); - $this->stepConfiguration->setAllowedTypes(DeleteServiceInterface::ELEMENT_TO_DELETE, 'int'); - $this->stepConfiguration->setRequired(DeleteServiceInterface::ELEMENT_TYPE_TO_DELETE); + $this->stepConfiguration->setRequired(StepConfig::ITEMS_TO_BATCH_DELETE->value); + $this->stepConfiguration->setAllowedTypes( + StepConfig::ITEMS_TO_BATCH_DELETE->value, + StepConfig::CONFIG_TYPE_ARRAY->value + ); + $this->stepConfiguration->setRequired(StepConfig::ELEMENT_TYPE_TO_BATCH_DELETE->value); $this->stepConfiguration->setAllowedTypes( - DeleteServiceInterface::ELEMENT_TYPE_TO_DELETE, - 'string' + StepConfig::ELEMENT_TYPE_TO_BATCH_DELETE->value, + StepConfig::CONFIG_TYPE_STRING->value ); } } diff --git a/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/ElementDeleteHandler.php b/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/ElementDeleteHandler.php index 81162c449..261127f76 100644 --- a/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/ElementDeleteHandler.php +++ b/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/ElementDeleteHandler.php @@ -18,15 +18,18 @@ use Pimcore\Bundle\StudioBackendBundle\Element\ExecutionEngine\AutomationAction\Messenger\Messages\ElementDeleteMessage; use Pimcore\Bundle\StudioBackendBundle\Element\Service\ElementDeleteServiceInterface; use Pimcore\Bundle\StudioBackendBundle\Element\Service\ElementServiceInterface; -use Pimcore\Bundle\StudioBackendBundle\Element\Service\ExecutionEngine\DeleteServiceInterface; use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\AutomationAction\AbstractHandler; use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Model\AbortActionData; use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\Config; +use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\StepConfig; use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\Trait\HandlerProgressTrait; use Pimcore\Bundle\StudioBackendBundle\Mercure\Service\PublishServiceInterface; use Pimcore\Bundle\StudioBackendBundle\Mercure\Service\UserTopicServiceInterface; +use Pimcore\Model\Element\ElementInterface; use Pimcore\Model\Element\ElementDescriptor; +use Pimcore\Model\UserInterface; use Symfony\Component\Messenger\Attribute\AsMessageHandler; +use function count; /** * @internal @@ -68,40 +71,63 @@ public function __invoke(ElementDeleteMessage $message): void $user = $validatedParameters->getUser(); $parentElement = $validatedParameters->getSubject(); - $element = $this->getElementById( - new ElementDescriptor( - $parentElement->getType(), - $this->extractConfigFieldFromJobStepConfig($message, DeleteServiceInterface::ELEMENT_TO_DELETE) - ), - $user, - $this->elementService + + $items = $this->extractConfigFieldFromJobStepConfig( + $message, + StepConfig::ITEMS_TO_DELETE->value ); + $totalItems = count($items); + $stepName = $this->getJobStep($message)->getName(); - if ($element->getId() === $parentElement->getId()) { - try { - $this->elementDeleteService->deleteParentElement($element, $user); - $this->updateProgress( - $this->publishService, - $this->userTopicService, - $jobRun, - $this->getJobStep($message)->getName() - ); - } catch (Exception $exception) { - $this->abort($this->getAbortData( - Config::ELEMENT_DELETE_FAILED_MESSAGE->value, - [ - 'type' => $element->getType(), - 'id' => $element->getId(), - 'message' => $exception->getMessage(), - ], - )); - } + foreach ($items as $elementId) { + $element = $this->getElementById( + new ElementDescriptor( + $parentElement->getType(), + $elementId + ), + $user, + $this->elementService + ); - return; + $this->deleteElement($element, $parentElement, $user); + + $this->updateProgress( + $this->publishService, + $this->userTopicService, + $jobRun, + $stepName, + $totalItems, + $totalItems, + ); } + } + + protected function configureStep(): void + { + $this->stepConfiguration->setRequired(StepConfig::ITEMS_TO_DELETE->value); + $this->stepConfiguration->setAllowedTypes( + StepConfig::ITEMS_TO_DELETE->value, + StepConfig::CONFIG_TYPE_ARRAY->value + ); + } + + + + /** + * @throws Exception + */ + private function deleteElement( + ElementInterface $element, + ElementInterface $parentElement, + UserInterface $user + ): void { + $isParent = $element->getId() === $parentElement->getId(); try { - $this->elementDeleteService->deleteElement($element, $user); + match ($isParent) { + true => $this->elementDeleteService->deleteParentElement($element, $user), + false => $this->elementDeleteService->deleteElement($element, $user), + }; } catch (Exception $exception) { $this->abort($this->getAbortData( Config::ELEMENT_DELETE_FAILED_MESSAGE->value, @@ -112,18 +138,5 @@ public function __invoke(ElementDeleteMessage $message): void ], )); } - - $this->updateProgress( - $this->publishService, - $this->userTopicService, - $jobRun, - $this->getJobStep($message)->getName() - ); - } - - protected function configureStep(): void - { - $this->stepConfiguration->setRequired(DeleteServiceInterface::ELEMENT_TO_DELETE); - $this->stepConfiguration->setAllowedTypes(DeleteServiceInterface::ELEMENT_TO_DELETE, 'int'); } } diff --git a/src/Element/Service/ExecutionEngine/DeleteService.php b/src/Element/Service/ExecutionEngine/DeleteService.php index c82fd8a5d..fc8d4cd74 100644 --- a/src/Element/Service/ExecutionEngine/DeleteService.php +++ b/src/Element/Service/ExecutionEngine/DeleteService.php @@ -13,6 +13,7 @@ namespace Pimcore\Bundle\StudioBackendBundle\Element\Service\ExecutionEngine; +use Generator; use Pimcore\Bundle\GenericExecutionEngineBundle\Agent\JobExecutionAgentInterface; use Pimcore\Bundle\GenericExecutionEngineBundle\Model\Job; use Pimcore\Bundle\GenericExecutionEngineBundle\Model\JobStep; @@ -23,16 +24,21 @@ use Pimcore\Bundle\StudioBackendBundle\Exception\Api\InvalidElementTypeException; use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\Config; use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\Jobs; +use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\StepConfig; use Pimcore\Bundle\StudioBackendBundle\Util\Constant\ElementTypes; use Pimcore\Model\Element\ElementDescriptor; use Pimcore\Model\Element\ElementInterface; use Pimcore\Model\UserInterface; +use function array_slice; +use function count; /** * @internal */ final readonly class DeleteService implements DeleteServiceInterface { + private const int DELETE_BATCH_SIZE = 500; + public function __construct( private JobExecutionAgentInterface $jobExecutionAgent ) { @@ -55,30 +61,22 @@ public function deleteElementsWithExecutionEngine( ); } - $jobSteps = array_merge( - $jobSteps, - array_map( - static fn (int $id) => new JobStep( - JobSteps::ELEMENT_DELETION->value, - ElementDeleteMessage::class, - '', - [self::ELEMENT_TO_DELETE => $id] - ), - $childrenIds - ) - ); + // Append parent ID to the end so it's deleted last + $allIds = array_merge($childrenIds, [$element->getId()]); - $jobSteps[] = new JobStep( - JobSteps::ELEMENT_DELETION->value, - ElementDeleteMessage::class, - '', - [self::ELEMENT_TO_DELETE => $element->getId()] - ); + foreach ($this->chunkGenerator($allIds, self::DELETE_BATCH_SIZE) as $batch) { + $jobSteps[] = new JobStep( + JobSteps::ELEMENT_DELETION->value, + ElementDeleteMessage::class, + '', + [StepConfig::ITEMS_TO_DELETE->value => $batch], + ); + } $job = new Job( name: $this->getJobName($elementType), steps: $jobSteps, - selectedElements:[ + selectedElements: [ new ElementDescriptor( $elementType, $element->getId() @@ -100,18 +98,23 @@ public function batchDeleteElements( UserInterface $user, string $elementType, ): int { - $jobSteps = array_map( - static fn (ElementInterface $element) => new JobStep( + $elementIds = array_map( + static fn (ElementInterface $element) => $element->getId(), + $elements + ); + + $jobSteps = []; + foreach ($this->chunkGenerator($elementIds, self::DELETE_BATCH_SIZE) as $batch) { + $jobSteps[] = new JobStep( JobSteps::ELEMENT_DELETION->value, BatchDeleteMessage::class, '', [ - self::ELEMENT_TO_DELETE => $element->getId(), - self::ELEMENT_TYPE_TO_DELETE => $elementType, - ] - ), - $elements - ); + StepConfig::ITEMS_TO_BATCH_DELETE->value => $batch, + StepConfig::ELEMENT_TYPE_TO_BATCH_DELETE->value => $elementType, + ], + ); + } $job = new Job( name: $this->getBatchJobName($elementType), @@ -151,4 +154,13 @@ private function getBatchJobName(string $type): string default => throw new InvalidElementTypeException($type), }; } + + private function chunkGenerator(array $items, int $size): Generator + { + $total = count($items); + + for ($i = 0; $i < $total; $i += $size) { + yield array_slice($items, $i, $size); + } + } } diff --git a/src/Element/Service/ExecutionEngine/DeleteServiceInterface.php b/src/Element/Service/ExecutionEngine/DeleteServiceInterface.php index abefcd092..f029001aa 100644 --- a/src/Element/Service/ExecutionEngine/DeleteServiceInterface.php +++ b/src/Element/Service/ExecutionEngine/DeleteServiceInterface.php @@ -21,10 +21,6 @@ */ interface DeleteServiceInterface { - public const string ELEMENT_TO_DELETE = 'element_to_delete'; - - public const string ELEMENT_TYPE_TO_DELETE = 'element_type_to_delete'; - public function deleteElementsWithExecutionEngine( ElementInterface $element, UserInterface $user, diff --git a/src/ExecutionEngine/Util/StepConfig.php b/src/ExecutionEngine/Util/StepConfig.php index 7cd37668b..9790369b7 100644 --- a/src/ExecutionEngine/Util/StepConfig.php +++ b/src/ExecutionEngine/Util/StepConfig.php @@ -42,4 +42,7 @@ enum StepConfig: string case CONFIG_TYPE_INT = 'int'; case CONFIG_TYPE_STRING = 'string'; case CONFIG_TYPE_BOOL = 'bool'; + case ITEMS_TO_BATCH_DELETE = 'items_to_batch_delete'; + case ITEMS_TO_DELETE = 'items_to_delete'; + case ELEMENT_TYPE_TO_BATCH_DELETE = 'element_type_to_batch_delete'; } diff --git a/tests/Unit/Element/Service/ExecutionEngine/DeleteServiceTest.php b/tests/Unit/Element/Service/ExecutionEngine/DeleteServiceTest.php new file mode 100644 index 000000000..32ddf3871 --- /dev/null +++ b/tests/Unit/Element/Service/ExecutionEngine/DeleteServiceTest.php @@ -0,0 +1,426 @@ +createDeleteService( + startJobExecution: function (Job $job) use (&$capturedJob) { + $capturedJob = $job; + + return $this->makeEmpty(JobRun::class, ['getId' => 42]); + } + ); + + $result = $service->deleteElementsWithExecutionEngine( + $this->makeElement($parentId), + $this->makeUser(5), + ElementTypes::TYPE_ASSET, + $childrenIds, + false, + ); + + $this->assertSame(42, $result); + + // 1500 children + 1 parent = 1501 items → 4 steps (500 + 500 + 500 + 1) + $steps = $capturedJob->getSteps(); + $this->assertCount(4, $steps); + + // First 3 steps have 500 items each + for ($i = 0; $i < 3; $i++) { + $batch = $steps[$i]->getConfig()[StepConfig::ITEMS_TO_DELETE->value]; + $this->assertCount(500, $batch); + } + + // Last step has 1 item (the parent) + $lastBatch = $steps[3]->getConfig()[StepConfig::ITEMS_TO_DELETE->value]; + $this->assertCount(1, $lastBatch); + $this->assertSame($parentId, $lastBatch[0]); + } + + /** + * @throws Exception + */ + public function testParentIdIsAppendedAfterChildren(): void + { + $childrenIds = [10, 20, 30]; + $parentId = 5; + $capturedJob = null; + + $service = $this->createDeleteService( + startJobExecution: function (Job $job) use (&$capturedJob) { + $capturedJob = $job; + + return $this->makeEmpty(JobRun::class, ['getId' => 1]); + } + ); + + $service->deleteElementsWithExecutionEngine( + $this->makeElement($parentId), + $this->makeUser(1), + ElementTypes::TYPE_OBJECT, + $childrenIds, + false, + ); + + $batch = $capturedJob->getSteps()[0]->getConfig()[StepConfig::ITEMS_TO_DELETE->value]; + $this->assertSame([10, 20, 30, 5], $batch); + } + + /** + * @throws Exception + */ + public function testRecycleBinStepPrependedWhenEnabled(): void + { + $capturedJob = null; + + $service = $this->createDeleteService( + startJobExecution: function (Job $job) use (&$capturedJob) { + $capturedJob = $job; + + return $this->makeEmpty(JobRun::class, ['getId' => 1]); + } + ); + + $service->deleteElementsWithExecutionEngine( + $this->makeElement(1), + $this->makeUser(1), + ElementTypes::TYPE_ASSET, + [2, 3], + true, + ); + + $steps = $capturedJob->getSteps(); + // First step is recycling, then deletion + $this->assertSame(RecycleBinMessage::class, $steps[0]->getMessageFQCN()); + $this->assertSame(JobSteps::ELEMENT_RECYCLING->value, $steps[0]->getName()); + $this->assertSame(ElementDeleteMessage::class, $steps[1]->getMessageFQCN()); + } + + /** + * @throws Exception + */ + public function testNoRecycleBinStepWhenDisabled(): void + { + $capturedJob = null; + + $service = $this->createDeleteService( + startJobExecution: function (Job $job) use (&$capturedJob) { + $capturedJob = $job; + + return $this->makeEmpty(JobRun::class, ['getId' => 1]); + } + ); + + $service->deleteElementsWithExecutionEngine( + $this->makeElement(1), + $this->makeUser(1), + ElementTypes::TYPE_ASSET, + [], + false, + ); + + $steps = $capturedJob->getSteps(); + $this->assertCount(1, $steps); + $this->assertSame(ElementDeleteMessage::class, $steps[0]->getMessageFQCN()); + } + + /** + * @throws Exception + */ + public function testUsesContinueOnErrorContext(): void + { + $capturedContext = null; + + $service = $this->createDeleteService( + startJobExecution: function (Job $job, ?int $ownerId, string $context) use (&$capturedContext) { + $capturedContext = $context; + + return $this->makeEmpty(JobRun::class, ['getId' => 1]); + } + ); + + $service->deleteElementsWithExecutionEngine( + $this->makeElement(1), + $this->makeUser(1), + ElementTypes::TYPE_ASSET, + [], + false, + ); + + $this->assertSame(Config::CONTEXT_CONTINUE_ON_ERROR->value, $capturedContext); + } + + /** + * @throws Exception + */ + public function testJobNameMatchesElementType(): void + { + $capturedJobs = []; + + $typeToJob = [ + ElementTypes::TYPE_ASSET => Jobs::DELETE_ASSETS->value, + ElementTypes::TYPE_DOCUMENT => Jobs::DELETE_DOCUMENTS->value, + ElementTypes::TYPE_OBJECT => Jobs::DELETE_DATA_OBJECTS->value, + ]; + + foreach ($typeToJob as $type => $expectedName) { + $service = $this->createDeleteService( + startJobExecution: function (Job $job) use (&$capturedJobs, $type) { + $capturedJobs[$type] = $job; + + return $this->makeEmpty(JobRun::class, ['getId' => 1]); + } + ); + + $service->deleteElementsWithExecutionEngine( + $this->makeElement(1), + $this->makeUser(1), + $type, + [], + false, + ); + } + + foreach ($typeToJob as $type => $expectedName) { + $this->assertSame($expectedName, $capturedJobs[$type]->getName()); + } + } + + /** + * @throws Exception + */ + public function testSelectedElementsContainsParentDescriptor(): void + { + $capturedJob = null; + + $service = $this->createDeleteService( + startJobExecution: function (Job $job) use (&$capturedJob) { + $capturedJob = $job; + + return $this->makeEmpty(JobRun::class, ['getId' => 1]); + } + ); + + $service->deleteElementsWithExecutionEngine( + $this->makeElement(99), + $this->makeUser(1), + ElementTypes::TYPE_DOCUMENT, + [200, 300], + false, + ); + + $selectedElements = $capturedJob->getSelectedElements(); + $this->assertCount(1, $selectedElements); + $this->assertSame(99, $selectedElements[0]->getId()); + $this->assertSame(ElementTypes::TYPE_DOCUMENT, $selectedElements[0]->getType()); + } + + /** + * @throws Exception + */ + public function testBatchDeleteChunksElementsIntoMultipleSteps(): void + { + $elements = []; + for ($i = 1; $i <= 1200; $i++) { + $elements[] = $this->makeElement($i); + } + $capturedJob = null; + + $service = $this->createDeleteService( + startJobExecution: function (Job $job) use (&$capturedJob) { + $capturedJob = $job; + + return $this->makeEmpty(JobRun::class, ['getId' => 55]); + } + ); + + $result = $service->batchDeleteElements($elements, $this->makeUser(1), ElementTypes::TYPE_ASSET); + + $this->assertSame(55, $result); + + $steps = $capturedJob->getSteps(); + $this->assertCount(3, $steps); + + // First batch: IDs 1–500 + $batch1 = $steps[0]->getConfig()[StepConfig::ITEMS_TO_BATCH_DELETE->value]; + $this->assertCount(500, $batch1); + $this->assertSame(1, $batch1[0]); + $this->assertSame(500, $batch1[499]); + + // Second batch: IDs 501–1000 + $batch2 = $steps[1]->getConfig()[StepConfig::ITEMS_TO_BATCH_DELETE->value]; + $this->assertCount(500, $batch2); + $this->assertSame(501, $batch2[0]); + + // Third batch: IDs 1001–1200 + $batch3 = $steps[2]->getConfig()[StepConfig::ITEMS_TO_BATCH_DELETE->value]; + $this->assertCount(200, $batch3); + $this->assertSame(1001, $batch3[0]); + $this->assertSame(1200, $batch3[199]); + + foreach ($steps as $step) { + $this->assertSame(BatchDeleteMessage::class, $step->getMessageFQCN()); + $this->assertSame( + ElementTypes::TYPE_ASSET, + $step->getConfig()[StepConfig::ELEMENT_TYPE_TO_BATCH_DELETE->value] + ); + } + } + + /** + * @throws Exception + */ + public function testBatchDeleteSingleBatch(): void + { + $elements = [$this->makeElement(10), $this->makeElement(20), $this->makeElement(30)]; + $capturedJob = null; + + $service = $this->createDeleteService( + startJobExecution: function (Job $job) use (&$capturedJob) { + $capturedJob = $job; + + return $this->makeEmpty(JobRun::class, ['getId' => 1]); + } + ); + + $service->batchDeleteElements($elements, $this->makeUser(1), ElementTypes::TYPE_DATA_OBJECT); + + $steps = $capturedJob->getSteps(); + $this->assertCount(1, $steps); + + $batch = $steps[0]->getConfig()[StepConfig::ITEMS_TO_BATCH_DELETE->value]; + $this->assertSame([10, 20, 30], $batch); + $this->assertSame( + ElementTypes::TYPE_DATA_OBJECT, + $steps[0]->getConfig()[StepConfig::ELEMENT_TYPE_TO_BATCH_DELETE->value] + ); + } + + /** + * @throws Exception + */ + public function testBatchDeleteJobNameMatchesElementType(): void + { + $capturedJobs = []; + + $typeToJob = [ + ElementTypes::TYPE_ASSET => Jobs::BATCH_DELETE_ASSETS->value, + ElementTypes::TYPE_DATA_OBJECT => Jobs::BATCH_DELETE_DATA_OBJECTS->value, + ]; + + foreach ($typeToJob as $type => $expectedName) { + $service = $this->createDeleteService( + startJobExecution: function (Job $job) use (&$capturedJobs, $type) { + $capturedJobs[$type] = $job; + + return $this->makeEmpty(JobRun::class, ['getId' => 1]); + } + ); + + $service->batchDeleteElements( + [$this->makeElement(1)], + $this->makeUser(1), + $type, + ); + } + + foreach ($typeToJob as $type => $expectedName) { + $this->assertSame($expectedName, $capturedJobs[$type]->getName()); + } + } + + /** + * @throws Exception + */ + public function testBatchDeleteHasNoSelectedElements(): void + { + $capturedJob = null; + + $service = $this->createDeleteService( + startJobExecution: function (Job $job) use (&$capturedJob) { + $capturedJob = $job; + + return $this->makeEmpty(JobRun::class, ['getId' => 1]); + } + ); + + $service->batchDeleteElements( + [$this->makeElement(1)], + $this->makeUser(1), + ElementTypes::TYPE_ASSET, + ); + + $this->assertEmpty($capturedJob->getSelectedElements()); + } + + /** + * @throws Exception + */ + private function createDeleteService(?callable $startJobExecution = null): DeleteService + { + $jobExecutionAgent = $this->makeEmpty(JobExecutionAgentInterface::class, [ + 'startJobExecution' => $startJobExecution ?? function () { + return $this->makeEmpty(JobRun::class, ['getId' => 1]); + }, + ]); + + return new DeleteService($jobExecutionAgent); + } + + /** + * @throws Exception + */ + private function makeElement(int $id): ElementInterface + { + return $this->makeEmpty(ElementInterface::class, ['getId' => $id]); + } + + /** + * @throws Exception + */ + private function makeUser(int $id): UserInterface + { + return $this->makeEmpty(UserInterface::class, ['getId' => $id]); + } +} From cd32df6d533e3e9d3727c33d2ca022e21ff968bd Mon Sep 17 00:00:00 2001 From: lukmzig Date: Thu, 30 Apr 2026 13:56:47 +0200 Subject: [PATCH 2/3] update the progress frequency --- .../Messenger/Handler/BatchDeleteHandler.php | 2 +- .../Messenger/Handler/ElementDeleteHandler.php | 15 ++++++--------- src/Element/Service/ElementDeleteService.php | 12 +++++------- .../Service/ElementDeleteServiceInterface.php | 5 +++-- 4 files changed, 15 insertions(+), 19 deletions(-) diff --git a/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/BatchDeleteHandler.php b/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/BatchDeleteHandler.php index 4f2418bb1..0360d97db 100644 --- a/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/BatchDeleteHandler.php +++ b/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/BatchDeleteHandler.php @@ -96,7 +96,7 @@ public function __invoke(BatchDeleteMessage $message): void $jobRun, $stepName, $totalItems, - $totalItems, + 100, ); } } diff --git a/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/ElementDeleteHandler.php b/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/ElementDeleteHandler.php index 261127f76..29b5f954e 100644 --- a/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/ElementDeleteHandler.php +++ b/src/Element/ExecutionEngine/AutomationAction/Messenger/Handler/ElementDeleteHandler.php @@ -81,15 +81,12 @@ public function __invoke(ElementDeleteMessage $message): void foreach ($items as $elementId) { $element = $this->getElementById( - new ElementDescriptor( - $parentElement->getType(), - $elementId - ), + new ElementDescriptor($parentElement->getType(), $elementId), $user, $this->elementService ); - $this->deleteElement($element, $parentElement, $user); + $this->deleteElement($element, $parentElement->getId(), $user); $this->updateProgress( $this->publishService, @@ -97,7 +94,7 @@ public function __invoke(ElementDeleteMessage $message): void $jobRun, $stepName, $totalItems, - $totalItems, + 100, ); } } @@ -118,15 +115,15 @@ protected function configureStep(): void */ private function deleteElement( ElementInterface $element, - ElementInterface $parentElement, + int $parentId, UserInterface $user ): void { - $isParent = $element->getId() === $parentElement->getId(); + $isParent = $element->getId() === $parentId; try { match ($isParent) { true => $this->elementDeleteService->deleteParentElement($element, $user), - false => $this->elementDeleteService->deleteElement($element, $user), + false => $this->elementDeleteService->deleteElement($element, $user, true), }; } catch (Exception $exception) { $this->abort($this->getAbortData( diff --git a/src/Element/Service/ElementDeleteService.php b/src/Element/Service/ElementDeleteService.php index ddd8710aa..87ebf0753 100644 --- a/src/Element/Service/ElementDeleteService.php +++ b/src/Element/Service/ElementDeleteService.php @@ -150,7 +150,8 @@ public function deleteParentElement( */ public function deleteElement( ElementInterface $element, - UserInterface $user + UserInterface $user, + bool $skipChildrenCheck = false ): void { /** @var User $user because of the core method */ @@ -165,14 +166,11 @@ public function deleteElement( } if ($element->isLocked()) { - throw new ForbiddenException(sprintf('Element %s is locked', $element->getId()) - ); + throw new ForbiddenException(sprintf('Element %s is locked', $element->getId())); } - if ($this->elementService->hasElementChildren($element)) { - throw new EnvironmentException( - 'Element has existing children' - ); + if (!$skipChildrenCheck && $this->elementService->hasElementChildren($element)) { + throw new EnvironmentException('Element has existing children'); } try { diff --git a/src/Element/Service/ElementDeleteServiceInterface.php b/src/Element/Service/ElementDeleteServiceInterface.php index 1aed70edc..5c587ad6f 100644 --- a/src/Element/Service/ElementDeleteServiceInterface.php +++ b/src/Element/Service/ElementDeleteServiceInterface.php @@ -57,11 +57,12 @@ public function deleteParentElement( ): void; /** - * @throws ElementDeletionFailedException|EnvironmentException + * @throws ElementDeletionFailedException|EnvironmentException|ForbiddenException */ public function deleteElement( ElementInterface $element, - UserInterface $user + UserInterface $user, + bool $skipChildrenCheck = false ): void; public function addElementToRecycleBin( From b6ac4ee37acf03fda1045ad49eb21c7dec5be471 Mon Sep 17 00:00:00 2001 From: lukmzig Date: Thu, 30 Apr 2026 14:02:34 +0200 Subject: [PATCH 3/3] remove duplicated code --- .../Service/ExecutionEngine/DeleteService.php | 15 ++------- .../Util/Trait/ChunkGeneratorTrait.php | 33 +++++++++++++++++++ .../Service/ExecutionEngine/ExportService.php | 15 ++------- src/RecycleBin/Service/JobService.php | 15 ++------- 4 files changed, 42 insertions(+), 36 deletions(-) create mode 100644 src/ExecutionEngine/Util/Trait/ChunkGeneratorTrait.php diff --git a/src/Element/Service/ExecutionEngine/DeleteService.php b/src/Element/Service/ExecutionEngine/DeleteService.php index fc8d4cd74..3a0112a18 100644 --- a/src/Element/Service/ExecutionEngine/DeleteService.php +++ b/src/Element/Service/ExecutionEngine/DeleteService.php @@ -13,7 +13,6 @@ namespace Pimcore\Bundle\StudioBackendBundle\Element\Service\ExecutionEngine; -use Generator; use Pimcore\Bundle\GenericExecutionEngineBundle\Agent\JobExecutionAgentInterface; use Pimcore\Bundle\GenericExecutionEngineBundle\Model\Job; use Pimcore\Bundle\GenericExecutionEngineBundle\Model\JobStep; @@ -25,18 +24,19 @@ use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\Config; use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\Jobs; use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\StepConfig; +use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\Trait\ChunkGeneratorTrait; use Pimcore\Bundle\StudioBackendBundle\Util\Constant\ElementTypes; use Pimcore\Model\Element\ElementDescriptor; use Pimcore\Model\Element\ElementInterface; use Pimcore\Model\UserInterface; -use function array_slice; -use function count; /** * @internal */ final readonly class DeleteService implements DeleteServiceInterface { + use ChunkGeneratorTrait; + private const int DELETE_BATCH_SIZE = 500; public function __construct( @@ -154,13 +154,4 @@ private function getBatchJobName(string $type): string default => throw new InvalidElementTypeException($type), }; } - - private function chunkGenerator(array $items, int $size): Generator - { - $total = count($items); - - for ($i = 0; $i < $total; $i += $size) { - yield array_slice($items, $i, $size); - } - } } diff --git a/src/ExecutionEngine/Util/Trait/ChunkGeneratorTrait.php b/src/ExecutionEngine/Util/Trait/ChunkGeneratorTrait.php new file mode 100644 index 000000000..060bca598 --- /dev/null +++ b/src/ExecutionEngine/Util/Trait/ChunkGeneratorTrait.php @@ -0,0 +1,33 @@ +value) { diff --git a/src/RecycleBin/Service/JobService.php b/src/RecycleBin/Service/JobService.php index 95a69359e..c79165825 100644 --- a/src/RecycleBin/Service/JobService.php +++ b/src/RecycleBin/Service/JobService.php @@ -13,26 +13,26 @@ namespace Pimcore\Bundle\StudioBackendBundle\RecycleBin\Service; -use Generator; use Pimcore\Bundle\GenericExecutionEngineBundle\Agent\JobExecutionAgentInterface; use Pimcore\Bundle\GenericExecutionEngineBundle\Model\Job; use Pimcore\Bundle\GenericExecutionEngineBundle\Model\JobStep; use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\Config; use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\Jobs; use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\StepConfig; +use Pimcore\Bundle\StudioBackendBundle\ExecutionEngine\Util\Trait\ChunkGeneratorTrait; use Pimcore\Bundle\StudioBackendBundle\RecycleBin\ExecutionEngine\Messages\RestoreItemsMessage; use Pimcore\Bundle\StudioBackendBundle\RecycleBin\ExecutionEngine\Util\JobSteps; use Pimcore\Bundle\StudioBackendBundle\Security\Service\SecurityServiceInterface; use Pimcore\Model\Element\ElementDescriptor; use function array_map; -use function array_slice; -use function count; /** * @internal */ final readonly class JobService implements JobServiceInterface { + use ChunkGeneratorTrait; + private const int RESTORE_BATCH_SIZE = 500; public function __construct( @@ -91,13 +91,4 @@ public function createRestoreJob(array $sortedItemIds): int return $jobRun->getId(); } - - private function chunkGenerator(array $items, int $size): Generator - { - $total = count($items); - - for ($i = 0; $i < $total; $i += $size) { - yield array_slice($items, $i, $size); - } - } }