From 6668c5d4c1a81c2f8fab364e1626043a82162ff3 Mon Sep 17 00:00:00 2001 From: Alexander Batenburg Date: Tue, 18 Aug 2026 19:06:47 +0200 Subject: [PATCH 1/5] Add interpretation-stage events: PreInterpretFileEvent and PreQueueRowEvent Listeners can now normalize the source file, and modify, skip, or fan out extracted rows before they are queued - covering row filtering, cross-row state, synthetic resolver columns, and 1-to-N imports without writing a custom interpreter. Both events are mirrored into the Studio preview so the mapping UI shows the columns an actual import would produce. Co-Authored-By: Claude Fable 5 --- doc/06_Extending/02_Events.md | 78 ++++++ .../Interpreter/AbstractInterpreter.php | 47 ++++ .../InterpreterConfigurationFactoryPass.php | 10 + src/Event/PreInterpretFileEvent.php | 62 +++++ src/Event/PreQueueRowEvent.php | 125 +++++++++ src/Hydrator/PreviewHydrator.php | 14 +- src/Preview/PreviewEventApplier.php | 107 ++++++++ src/Resources/config/services.yml | 2 + src/Service/Studio/PreviewDataService.php | 16 +- src/Service/Studio/TransformationService.php | 14 +- tests/unit/InterpreterRowEventTest.php | 239 ++++++++++++++++++ tests/unit/PreQueueRowEventTest.php | 110 ++++++++ tests/unit/PreviewEventApplierTest.php | 129 ++++++++++ 13 files changed, 950 insertions(+), 3 deletions(-) create mode 100644 src/Event/PreInterpretFileEvent.php create mode 100644 src/Event/PreQueueRowEvent.php create mode 100644 src/Preview/PreviewEventApplier.php create mode 100644 tests/unit/InterpreterRowEventTest.php create mode 100644 tests/unit/PreQueueRowEventTest.php create mode 100644 tests/unit/PreviewEventApplierTest.php diff --git a/doc/06_Extending/02_Events.md b/doc/06_Extending/02_Events.md index 6804dec76..4672dcb19 100644 --- a/doc/06_Extending/02_Events.md +++ b/doc/06_Extending/02_Events.md @@ -12,6 +12,8 @@ Listening for events customizes import behaviour without replacing any component | Event | Fired | |---|---| +| `PreInterpretFileEvent` | Before the interpreter starts reading the source file. | +| `PreQueueRowEvent` | For every extracted row, before it is added to the processing queue. | | `DataObject\PreSaveEvent` | Before an imported data object is saved. | | `DataObject\PostSaveEvent` | After an imported data object is saved. | | `DataObject\ProcessElementExceptionEvent` | When processing a record throws an exception. | @@ -23,6 +25,82 @@ that failed, when the failure can be attributed to one. `PostPreparationEvent` exposes the configuration name, the execution type, and whether the source file was interpreted. +## Interpretation-Stage Events + +The two interpretation-stage events customize how the source file turns into import rows without writing a custom +interpreter. Both expose the configuration name and the execution type, and both are also dispatched (with +`isPreview()` returning `true`) when Pimcore Studio renders the source preview and the available mapping columns, so +the configuration UI shows exactly the data an actual import would produce. + +### PreInterpretFileEvent + +Dispatched before the interpreter validates and reads the source file. `setPath()` replaces the file that gets +interpreted - use it to normalize a file (transcode it, strip a report preamble, rewrite delimiters) while keeping the +standard interpreter. It also marks the start of an interpretation run, which stateful `PreQueueRowEvent` listeners can +use as a reset signal. + +### PreQueueRowEvent + +Dispatched for every row the interpreter extracted, right before the row is added to the processing queue. The listener +receives the row exactly as the interpreter produced it - before the delta check, the resolver's identifier extraction, +and the mapping pipeline - so changed values (including the ID column) affect which element a row resolves to. + +| Method | Purpose | +|---|---| +| `getOriginalRow()` | The row as extracted, unaffected by other listeners. | +| `getRows()` / `setRows(array $rows)` | The rows that will be queued. Set one row to modify it, an empty array to skip it, or multiple rows to fan the source row out into multiple elements. | +| `skipRow(bool $keepInCleanupIdentifierCache = false)` | Skip the row. See the cleanup warning below. | + +```php +namespace App\EventListener; + +use Pimcore\Bundle\DataImporterBundle\Event\PreQueueRowEvent; +use Symfony\Component\EventDispatcher\Attribute\AsEventListener; + +#[AsEventListener] +final class ProductRowListener +{ + public function __invoke(PreQueueRowEvent $event): void + { + if ($event->getConfigName() !== 'my-product-import') { + return; + } + + $row = $event->getOriginalRow(); + + // skip discontinued products, but keep their existing objects + if (($row['status'] ?? '') === 'discontinued') { + $event->skipRow(keepInCleanupIdentifierCache: true); + + return; + } + + // add a computed column that mapping, resolver and location strategies can use + $row['path'] = '/products/' . $row['category'] . '/' . $row['sku']; + + // fan out: one source row per configured sales channel becomes one element each + $rows = []; + foreach (explode(',', $row['channels']) as $channel) { + $rows[] = ['channel' => $channel] + $row; + } + + $event->setRows($rows); + } +} +``` + +:::warning + +When the import uses an active cleanup strategy, every element whose identifier is not seen during interpretation is +deleted or unpublished. A skipped row's element counts as "not seen". Skip rows with +`skipRow(keepInCleanupIdentifierCache: true)` when their existing elements must survive the cleanup. + +::: + +Custom interpreters get both events automatically as long as they extend `AbstractInterpreter` (or provide a +`setEventDispatcher()` method) - the interpreter compiler pass wires the dispatcher into every tagged interpreter +service. + ## Example Adjust a data object right before it is saved: diff --git a/src/DataSource/Interpreter/AbstractInterpreter.php b/src/DataSource/Interpreter/AbstractInterpreter.php index 13855f6e1..fb75e2e2a 100644 --- a/src/DataSource/Interpreter/AbstractInterpreter.php +++ b/src/DataSource/Interpreter/AbstractInterpreter.php @@ -15,6 +15,8 @@ use Pimcore\Bundle\ApplicationLoggerBundle\ApplicationLogger; use Pimcore\Bundle\ApplicationLoggerBundle\FileObject; use Pimcore\Bundle\DataImporterBundle\DataSource\Interpreter\DeltaChecker\DeltaChecker; +use Pimcore\Bundle\DataImporterBundle\Event\PreInterpretFileEvent; +use Pimcore\Bundle\DataImporterBundle\Event\PreQueueRowEvent; use Pimcore\Bundle\DataImporterBundle\Exception\InvalidInputException; use Pimcore\Bundle\DataImporterBundle\PimcoreDataImporterBundle; use Pimcore\Bundle\DataImporterBundle\Processing\ImportProcessingService; @@ -23,6 +25,7 @@ use Pimcore\Model\Tool\TmpStore; use Pimcore\Tool\Admin; use Psr\Log\LoggerAwareTrait; +use Symfony\Contracts\EventDispatcher\EventDispatcherInterface; /** * @internal @@ -45,6 +48,8 @@ abstract class AbstractInterpreter implements InterpreterInterface protected Resolver $resolver; + protected ?EventDispatcherInterface $eventDispatcher = null; + /** * @var string[] */ @@ -128,11 +133,22 @@ public function setResolver(Resolver $resolver): void $this->resolver = $resolver; } + public function setEventDispatcher(?EventDispatcherInterface $eventDispatcher): void + { + $this->eventDispatcher = $eventDispatcher; + } + public function interpretFile(string $path): bool { $success = false; $this->resetIdentifierCache(); + if ($this->eventDispatcher !== null) { + $event = new PreInterpretFileEvent($this->configName, $this->executionType, $path); + $this->eventDispatcher->dispatch($event); + $path = $event->getPath(); + } + if ($this->fileValid($path)) { $archiveLogMessage = 'Interpreted source file and created queue items.'; $this->doInterpretFileAndCallProcessRow($path); @@ -161,6 +177,37 @@ public function interpretFile(string $path): bool abstract protected function doInterpretFileAndCallProcessRow(string $path): void; protected function processImportRow(array $data) + { + foreach ($this->applyPreQueueRowEvent($data) as $row) { + $this->addRowToQueue($row); + } + } + + /** + * Lets PreQueueRowEvent listeners modify, skip, or fan out the row the interpreter + * extracted. Without a dispatcher or listeners the row passes through unchanged. + * + * @return array the rows to queue + */ + private function applyPreQueueRowEvent(array $data): array + { + if ($this->eventDispatcher === null) { + return [$data]; + } + + $event = new PreQueueRowEvent($this->configName, $this->executionType, $data); + $this->eventDispatcher->dispatch($event); + + if ($event->isRowSkipped() && $event->shouldKeepSkippedRowInIdentifierCache()) { + // Register the skipped row's identifier so an active cleanup strategy does not + // treat its existing element as removed from the source. + $this->addToIdentifierCache($event->getOriginalRow()); + } + + return $event->getRows(); + } + + private function addRowToQueue(array $data): void { $this->assertValidRowEncoding($data); diff --git a/src/DependencyInjection/CompilerPass/InterpreterConfigurationFactoryPass.php b/src/DependencyInjection/CompilerPass/InterpreterConfigurationFactoryPass.php index 74cedf9f1..7ed2c6957 100644 --- a/src/DependencyInjection/CompilerPass/InterpreterConfigurationFactoryPass.php +++ b/src/DependencyInjection/CompilerPass/InterpreterConfigurationFactoryPass.php @@ -33,6 +33,16 @@ public function process(ContainerBuilder $container): void foreach ($tags as $attributes) { $interpreters[$attributes['type']] = new Reference($id); } + + // Wire the event dispatcher into every interpreter that supports it (built-in + // and custom alike), so PreInterpretFileEvent/PreQueueRowEvent are dispatched. + $definition = $container->getDefinition($id); + $class = $definition->getClass() ?? $id; + if (method_exists($class, 'setEventDispatcher') + && !$definition->hasMethodCall('setEventDispatcher') + ) { + $definition->addMethodCall('setEventDispatcher', [new Reference('event_dispatcher')]); + } } } diff --git a/src/Event/PreInterpretFileEvent.php b/src/Event/PreInterpretFileEvent.php new file mode 100644 index 000000000..1b3fccdd0 --- /dev/null +++ b/src/Event/PreInterpretFileEvent.php @@ -0,0 +1,62 @@ +configName; + } + + public function getExecutionType(): string + { + return $this->executionType; + } + + public function getPath(): string + { + return $this->path; + } + + public function setPath(string $path): self + { + $this->path = $path; + + return $this; + } + + /** + * True when the file is being read for the Studio preview instead of an actual import. + */ + public function isPreview(): bool + { + return $this->preview; + } +} diff --git a/src/Event/PreQueueRowEvent.php b/src/Event/PreQueueRowEvent.php new file mode 100644 index 000000000..22620622b --- /dev/null +++ b/src/Event/PreQueueRowEvent.php @@ -0,0 +1,125 @@ +setRows([$changedRow]); + * - skip: $event->skipRow(); or $event->setRows([]); + * - fan-out: $event->setRows([$rowA, $rowB, $rowC]); + * + * When an import uses an active cleanup strategy, every element whose identifier is not seen + * during interpretation gets deleted or unpublished. Skipping a row therefore makes the + * cleanup treat the row's existing element as removed from the source. Use + * $event->skipRow(keepInCleanupIdentifierCache: true) to skip the row but still register its + * identifier, so the existing element is left untouched. + * + * The same event is dispatched (with isPreview() returning true) when the Studio preview + * renders the source columns, so columns added by listeners are visible and mappable in the + * configuration UI. Rows skipped in preview mode are still displayed unmodified. + */ +final class PreQueueRowEvent extends Event +{ + /** + * @var array + */ + private array $rows; + + private bool $keepSkippedRowInIdentifierCache = false; + + public function __construct( + private readonly string $configName, + private readonly string $executionType, + private readonly array $originalRow, + private readonly bool $preview = false, + ) { + $this->rows = [$originalRow]; + } + + public function getConfigName(): string + { + return $this->configName; + } + + public function getExecutionType(): string + { + return $this->executionType; + } + + /** + * The row as the interpreter extracted it, unaffected by any listener. + */ + public function getOriginalRow(): array + { + return $this->originalRow; + } + + /** + * The rows that will be queued. Initially exactly the original row. + * + * @return array + */ + public function getRows(): array + { + return $this->rows; + } + + /** + * Replace the rows to queue: one row to modify, an empty array to skip, + * multiple rows to fan the source row out into multiple elements. + * + * @param array $rows + */ + public function setRows(array $rows): self + { + $this->rows = array_values($rows); + + return $this; + } + + /** + * Skip this row entirely. With $keepInCleanupIdentifierCache set to true the original + * row's identifier is still registered, so an active cleanup strategy does not treat the + * row's existing element as removed from the source. + */ + public function skipRow(bool $keepInCleanupIdentifierCache = false): self + { + $this->rows = []; + $this->keepSkippedRowInIdentifierCache = $keepInCleanupIdentifierCache; + + return $this; + } + + public function isRowSkipped(): bool + { + return $this->rows === []; + } + + public function shouldKeepSkippedRowInIdentifierCache(): bool + { + return $this->keepSkippedRowInIdentifierCache; + } + + /** + * True when the row is being read for the Studio preview instead of an actual import. + */ + public function isPreview(): bool + { + return $this->preview; + } +} diff --git a/src/Hydrator/PreviewHydrator.php b/src/Hydrator/PreviewHydrator.php index fe1b725b2..2372f000b 100644 --- a/src/Hydrator/PreviewHydrator.php +++ b/src/Hydrator/PreviewHydrator.php @@ -15,6 +15,7 @@ use Exception; use Pimcore\Bundle\DataImporterBundle\DataSource\Interpreter\InterpreterFactory; +use Pimcore\Bundle\DataImporterBundle\Preview\PreviewEventApplier; use Pimcore\Bundle\DataImporterBundle\Preview\PreviewService; use Pimcore\Bundle\DataImporterBundle\Schema\ColumnHeadersResponse; use Pimcore\Bundle\DataImporterBundle\Schema\DataPreviewResponse; @@ -33,7 +34,8 @@ public function __construct( private SecurityServiceInterface $securityService, private PreviewService $previewService, - private InterpreterFactory $interpreterFactory + private InterpreterFactory $interpreterFactory, + private PreviewEventApplier $previewEventApplier ) { } @@ -66,7 +68,17 @@ public function loadAvailableColumnHeaders(string $name, array $config): array $config['interpreterConfig'], $config['processingConfig'] ); + $previewFilePath = $this->previewEventApplier->applyToPath( + $name, + $config['processingConfig'], + $previewFilePath + ); $dataPreview = $interpreter->previewData($previewFilePath); + $dataPreview = $this->previewEventApplier->applyToPreviewData( + $name, + $config['processingConfig'], + $dataPreview + ); $columnHeaders = $dataPreview->getDataColumnHeaders(); if (!$this->isValidJson($columnHeaders)) { diff --git a/src/Preview/PreviewEventApplier.php b/src/Preview/PreviewEventApplier.php new file mode 100644 index 000000000..c957706ce --- /dev/null +++ b/src/Preview/PreviewEventApplier.php @@ -0,0 +1,107 @@ +resolveExecutionType($processingConfig), + $path, + preview: true + ); + $this->eventDispatcher->dispatch($event); + + return $event->getPath(); + } + + /** + * Lets PreQueueRowEvent listeners rewrite the preview record, mirroring what happens to + * every row of an actual import. A skipped row stays visible unmodified (the preview is + * an inspection aid); a fan-out shows the first resulting row and exposes the columns of + * all resulting rows, so every column a listener adds is mappable. + */ + public function applyToPreviewData( + string $configName, + array $processingConfig, + PreviewData $previewData, + array $mappedColumns = [] + ): PreviewData { + $originalRow = $previewData->getRawData(); + + $event = new PreQueueRowEvent( + $configName, + $this->resolveExecutionType($processingConfig), + $originalRow, + preview: true + ); + $this->eventDispatcher->dispatch($event); + + $rows = $event->getRows(); + if ($rows === [$originalRow]) { + return $previewData; + } + + $labels = []; + foreach ($previewData->getDataColumnHeaders() as $columnHeader) { + $labels[$columnHeader['dataIndex']] = $columnHeader['label']; + } + + $row = $rows[0] ?? $originalRow; + foreach ($rows as $additionalRow) { + foreach ($additionalRow as $index => $value) { + if (!array_key_exists($index, $row)) { + $row[$index] = $value; + } + } + } + + foreach (array_keys($row) as $index) { + if (!array_key_exists($index, $labels) && !array_key_exists((string) $index, $labels)) { + $labels[$index] = is_int($index) ? "[$index]" : (string) $index; + } + } + + return new PreviewData($labels, $row, $previewData->getRecordNumber(), $mappedColumns); + } + + private function resolveExecutionType(array $processingConfig): string + { + return $processingConfig['executionType'] ?? ImportProcessingService::EXECUTION_TYPE_SEQUENTIAL; + } +} diff --git a/src/Resources/config/services.yml b/src/Resources/config/services.yml index dadef2196..c545d0c99 100644 --- a/src/Resources/config/services.yml +++ b/src/Resources/config/services.yml @@ -23,6 +23,8 @@ services: Pimcore\Bundle\DataImporterBundle\Processing\ExecutionService: ~ Pimcore\Bundle\DataImporterBundle\Settings\ConfigurationPreparationService: ~ Pimcore\Bundle\DataImporterBundle\Preview\PreviewService: ~ + + Pimcore\Bundle\DataImporterBundle\Preview\PreviewEventApplier: ~ Pimcore\Bundle\DataImporterBundle\EventListener\ConfigurationEventSubscriber: ~ Pimcore\Bundle\DataImporterBundle\EventListener\DataImporterListener: tags: diff --git a/src/Service/Studio/PreviewDataService.php b/src/Service/Studio/PreviewDataService.php index df3bdd645..5f9b4354c 100644 --- a/src/Service/Studio/PreviewDataService.php +++ b/src/Service/Studio/PreviewDataService.php @@ -19,6 +19,7 @@ use Pimcore\Bundle\DataImporterBundle\Event\Studio\PreResponse\ColumnHeadersEvent; use Pimcore\Bundle\DataImporterBundle\Event\Studio\PreResponse\DataPreviewEvent; use Pimcore\Bundle\DataImporterBundle\Hydrator\PreviewHydratorInterface; +use Pimcore\Bundle\DataImporterBundle\Preview\PreviewEventApplier; use Pimcore\Bundle\DataImporterBundle\Preview\PreviewService; use Pimcore\Bundle\DataImporterBundle\Schema\ColumnHeadersResponse; use Pimcore\Bundle\DataImporterBundle\Schema\DataPreviewResponse; @@ -56,7 +57,8 @@ public function __construct( private ConfigurationPreparationService $configurationPreparationService, private DataLoaderFactory $dataLoaderFactory, private InterpreterFactory $interpreterFactory, - private EventDispatcherInterface $eventDispatcher + private EventDispatcherInterface $eventDispatcher, + private PreviewEventApplier $previewEventApplier ) { } @@ -184,6 +186,12 @@ public function loadPreviewData( $preparedConfig['processingConfig'] ); + $previewFilePath = $this->previewEventApplier->applyToPath( + $name, + $preparedConfig['processingConfig'], + $previewFilePath + ); + if (!$interpreter->fileValid($previewFilePath)) { throw new EnvironmentException( 'Preview file is not valid for the configured interpreter. ' @@ -192,6 +200,12 @@ public function loadPreviewData( } $dataPreview = $interpreter->previewData($previewFilePath, $recordNumber, $mappedColumns); + $dataPreview = $this->previewEventApplier->applyToPreviewData( + $name, + $preparedConfig['processingConfig'], + $dataPreview, + $mappedColumns + ); $preview = $dataPreview->getDataPreview(); if (!$this->previewHydrator->isValidJson($preview)) { diff --git a/src/Service/Studio/TransformationService.php b/src/Service/Studio/TransformationService.php index 4c0ac6345..dd7cc0781 100644 --- a/src/Service/Studio/TransformationService.php +++ b/src/Service/Studio/TransformationService.php @@ -18,6 +18,7 @@ use Pimcore\Bundle\DataImporterBundle\Event\Studio\PreResponse\TransformationResultTypeEvent; use Pimcore\Bundle\DataImporterBundle\Hydrator\TransformationHydratorInterface; use Pimcore\Bundle\DataImporterBundle\Mapping\MappingConfigurationFactory; +use Pimcore\Bundle\DataImporterBundle\Preview\PreviewEventApplier; use Pimcore\Bundle\DataImporterBundle\Preview\PreviewService; use Pimcore\Bundle\DataImporterBundle\Processing\ImportProcessingService; use Pimcore\Bundle\DataImporterBundle\Schema\TransformationResultPreviewsResponse; @@ -45,7 +46,8 @@ public function __construct( private InterpreterFactory $interpreterFactory, private MappingConfigurationFactory $mappingConfigurationFactory, private ImportProcessingService $importProcessingService, - private EventDispatcherInterface $eventDispatcher + private EventDispatcherInterface $eventDispatcher, + private PreviewEventApplier $previewEventApplier ) { } @@ -76,7 +78,17 @@ public function loadTransformationResultPreviews( $preparedConfig['processingConfig'] ); + $previewFilePath = $this->previewEventApplier->applyToPath( + $name, + $preparedConfig['processingConfig'], + $previewFilePath + ); $dataPreview = $interpreter->previewData($previewFilePath, $recordNumber); + $dataPreview = $this->previewEventApplier->applyToPreviewData( + $name, + $preparedConfig['processingConfig'], + $dataPreview + ); $importDataRow = $dataPreview->getRawData(); } diff --git a/tests/unit/InterpreterRowEventTest.php b/tests/unit/InterpreterRowEventTest.php new file mode 100644 index 000000000..e04ad6fdf --- /dev/null +++ b/tests/unit/InterpreterRowEventTest.php @@ -0,0 +1,239 @@ +configName = uniqid('test_row_event_'); + } + + protected function _after(): void + { + $this->drainQueue(); + + foreach ($this->tempFiles as $file) { + @unlink($file); + } + $this->tempFiles = []; + } + + private function queueService(): QueueService + { + return new QueueService(); + } + + private function createInterpreter(?EventDispatcherInterface $eventDispatcher): CsvFileInterpreter + { + $interpreter = new CsvFileInterpreter( + new DeltaChecker(\Pimcore\Db::get()), + $this->queueService(), + ApplicationLogger::getInstance() + ); + $interpreter->setLogger(new NullLogger()); + $interpreter->setConfigName($this->configName); + $interpreter->setExecutionType(ImportProcessingService::EXECUTION_TYPE_SEQUENTIAL); + $interpreter->setIdDataIndex('sku'); + $interpreter->setDoDeltaCheck(false); + $interpreter->setDoCleanup(false); + $interpreter->setDoArchiveImportFile(false); + $interpreter->setSettings([ + 'skipFirstRow' => true, + 'saveHeaderName' => true, + 'delimiter' => ',', + 'enclosure' => '"', + 'escape' => '\\', + ]); + $interpreter->setEventDispatcher($eventDispatcher); + + return $interpreter; + } + + private function writeCsv(string $content): string + { + $path = tempnam(sys_get_temp_dir(), 'di_csv_') . '.csv'; + file_put_contents($path, $content); + $this->tempFiles[] = $path; + + return $path; + } + + /** + * @return array + */ + private function loadQueuedRows(): array + { + $queueService = $this->queueService(); + $rows = []; + foreach ($queueService->getAllQueueEntryIds(ImportProcessingService::EXECUTION_TYPE_SEQUENTIAL) as $id) { + $entry = $queueService->getQueueEntryById($id); + if (($entry['configName'] ?? null) === $this->configName) { + $rows[] = json_decode($entry['data'], true); + } + } + + return $rows; + } + + private function drainQueue(): void + { + $queueService = $this->queueService(); + foreach ($queueService->getAllQueueEntryIds(ImportProcessingService::EXECUTION_TYPE_SEQUENTIAL) as $id) { + $entry = $queueService->getQueueEntryById($id); + if (($entry['configName'] ?? null) === $this->configName) { + $queueService->markQueueEntryAsProcessed($id); + } + } + } + + public function testCompilerPassWiresEventDispatcherIntoTaggedInterpreters(): void + { + try { + $interpreter = $this->tester->grabService(CsvFileInterpreter::class); + } catch (\Throwable $e) { + // The service is private; retrieving it by id needs a test container + // (framework.test: true), which not every local project setup provides. + $this->markTestSkipped('Service container does not expose private services: ' . $e->getMessage()); + } + + $property = new \ReflectionProperty($interpreter, 'eventDispatcher'); + $this->assertInstanceOf(EventDispatcherInterface::class, $property->getValue($interpreter)); + } + + public function testRowsAreQueuedUnchangedWithoutListeners(): void + { + $interpreter = $this->createInterpreter(new EventDispatcher()); + + $this->assertTrue($interpreter->interpretFile($this->writeCsv(self::CSV))); + + $rows = $this->loadQueuedRows(); + $this->assertCount(2, $rows); + $this->assertSame(['sku' => 'A-1', 'name' => 'First'], $rows[0]); + $this->assertSame(['sku' => 'B-2', 'name' => 'Second'], $rows[1]); + } + + public function testRowsAreQueuedUnchangedWithoutDispatcher(): void + { + $interpreter = $this->createInterpreter(null); + + $this->assertTrue($interpreter->interpretFile($this->writeCsv(self::CSV))); + + $this->assertCount(2, $this->loadQueuedRows()); + } + + public function testListenerCanModifyRows(): void + { + $dispatcher = new EventDispatcher(); + $dispatcher->addListener(PreQueueRowEvent::class, function (PreQueueRowEvent $event): void { + $row = $event->getOriginalRow(); + $row['path'] = '/products/' . $row['sku']; + $event->setRows([$row]); + }); + + $this->createInterpreter($dispatcher)->interpretFile($this->writeCsv(self::CSV)); + + $rows = $this->loadQueuedRows(); + $this->assertCount(2, $rows); + $this->assertSame('/products/A-1', $rows[0]['path']); + $this->assertSame('/products/B-2', $rows[1]['path']); + } + + public function testListenerCanSkipRows(): void + { + $dispatcher = new EventDispatcher(); + $dispatcher->addListener(PreQueueRowEvent::class, function (PreQueueRowEvent $event): void { + if ($event->getOriginalRow()['sku'] === 'B-2') { + $event->skipRow(); + } + }); + + $this->createInterpreter($dispatcher)->interpretFile($this->writeCsv(self::CSV)); + + $rows = $this->loadQueuedRows(); + $this->assertCount(1, $rows); + $this->assertSame('A-1', $rows[0]['sku']); + } + + public function testListenerCanFanOutOneRowIntoMultipleQueueItems(): void + { + $dispatcher = new EventDispatcher(); + $dispatcher->addListener(PreQueueRowEvent::class, function (PreQueueRowEvent $event): void { + $row = $event->getOriginalRow(); + if ($row['sku'] !== 'A-1') { + return; + } + + $rows = []; + foreach ([2023, 2024, 2025] as $year) { + $rows[] = $row + ['year' => $year, 'key' => $row['sku'] . '-' . $year]; + } + $event->setRows($rows); + }); + + $this->createInterpreter($dispatcher)->interpretFile($this->writeCsv(self::CSV)); + + $rows = $this->loadQueuedRows(); + $this->assertCount(4, $rows, 'A-1 fans out into three rows, B-2 stays one row'); + $this->assertSame(['A-1-2023', 'A-1-2024', 'A-1-2025'], array_column(array_slice($rows, 0, 3), 'key')); + $this->assertSame('B-2', $rows[3]['sku']); + } + + public function testPreInterpretFileEventCanReplaceTheSourceFile(): void + { + $originalPath = $this->writeCsv(self::CSV); + $replacementPath = $this->writeCsv("sku,name\nC-3,Replaced\n"); + + $dispatcher = new EventDispatcher(); + $dispatcher->addListener( + PreInterpretFileEvent::class, + function (PreInterpretFileEvent $event) use ($replacementPath): void { + $event->setPath($replacementPath); + } + ); + + $this->createInterpreter($dispatcher)->interpretFile($originalPath); + + $rows = $this->loadQueuedRows(); + $this->assertCount(1, $rows); + $this->assertSame('C-3', $rows[0]['sku']); + } +} diff --git a/tests/unit/PreQueueRowEventTest.php b/tests/unit/PreQueueRowEventTest.php new file mode 100644 index 000000000..a6f32ac05 --- /dev/null +++ b/tests/unit/PreQueueRowEventTest.php @@ -0,0 +1,110 @@ + 'A-1', 'name' => 'First']; + + private function createEvent(): PreQueueRowEvent + { + return new PreQueueRowEvent( + 'test_config', + ImportProcessingService::EXECUTION_TYPE_SEQUENTIAL, + self::ROW + ); + } + + public function testInitialStatePassesRowThroughUnchanged(): void + { + $event = $this->createEvent(); + + $this->assertSame([self::ROW], $event->getRows()); + $this->assertSame(self::ROW, $event->getOriginalRow()); + $this->assertFalse($event->isRowSkipped()); + $this->assertFalse($event->shouldKeepSkippedRowInIdentifierCache()); + $this->assertFalse($event->isPreview()); + } + + public function testSetRowsModifiesTheRow(): void + { + $event = $this->createEvent(); + $modified = self::ROW + ['path' => '/products/A-1']; + + $event->setRows([$modified]); + + $this->assertSame([$modified], $event->getRows()); + $this->assertSame(self::ROW, $event->getOriginalRow(), 'original row must stay untouched'); + $this->assertFalse($event->isRowSkipped()); + } + + public function testSetRowsFansOutAndReindexes(): void + { + $event = $this->createEvent(); + $rowA = self::ROW + ['year' => 2024]; + $rowB = self::ROW + ['year' => 2025]; + + $event->setRows([3 => $rowA, 7 => $rowB]); + + $this->assertSame([$rowA, $rowB], $event->getRows()); + } + + public function testSetRowsWithEmptyArraySkipsTheRow(): void + { + $event = $this->createEvent(); + + $event->setRows([]); + + $this->assertTrue($event->isRowSkipped()); + $this->assertFalse($event->shouldKeepSkippedRowInIdentifierCache()); + } + + public function testSkipRowDefaultsToNotKeepingTheIdentifier(): void + { + $event = $this->createEvent(); + + $event->skipRow(); + + $this->assertSame([], $event->getRows()); + $this->assertTrue($event->isRowSkipped()); + $this->assertFalse($event->shouldKeepSkippedRowInIdentifierCache()); + } + + public function testSkipRowCanKeepTheIdentifierForCleanup(): void + { + $event = $this->createEvent(); + + $event->skipRow(keepInCleanupIdentifierCache: true); + + $this->assertTrue($event->isRowSkipped()); + $this->assertTrue($event->shouldKeepSkippedRowInIdentifierCache()); + } + + public function testPreviewFlag(): void + { + $event = new PreQueueRowEvent( + 'test_config', + ImportProcessingService::EXECUTION_TYPE_SEQUENTIAL, + self::ROW, + preview: true + ); + + $this->assertTrue($event->isPreview()); + } +} diff --git a/tests/unit/PreviewEventApplierTest.php b/tests/unit/PreviewEventApplierTest.php new file mode 100644 index 000000000..be4829ae3 --- /dev/null +++ b/tests/unit/PreviewEventApplierTest.php @@ -0,0 +1,129 @@ + 'A-1', 'name' => 'First']; + + private const LABELS = ['sku' => 'sku', 'name' => 'name']; + + private function createApplier(?callable $rowListener = null, ?callable $fileListener = null): PreviewEventApplier + { + $dispatcher = new EventDispatcher(); + if ($rowListener !== null) { + $dispatcher->addListener(PreQueueRowEvent::class, $rowListener); + } + if ($fileListener !== null) { + $dispatcher->addListener(PreInterpretFileEvent::class, $fileListener); + } + + return new PreviewEventApplier($dispatcher); + } + + private function createPreviewData(): PreviewData + { + return new PreviewData(self::LABELS, self::ROW, 0); + } + + public function testPreviewDataIsReturnedUnchangedWithoutListeners(): void + { + $previewData = $this->createPreviewData(); + + $result = $this->createApplier()->applyToPreviewData('test_config', [], $previewData); + + $this->assertSame($previewData, $result); + } + + public function testListenerAddedColumnsBecomeVisibleAndMappable(): void + { + $applier = $this->createApplier(function (PreQueueRowEvent $event): void { + $this->assertTrue($event->isPreview()); + $row = $event->getOriginalRow(); + $row['path'] = '/products/' . $row['sku']; + $event->setRows([$row]); + }); + + $result = $applier->applyToPreviewData('test_config', [], $this->createPreviewData()); + + $this->assertSame('/products/A-1', $result->getRawData()['path']); + $this->assertContains( + ['id' => 'path', 'dataIndex' => 'path', 'label' => 'path'], + $result->getDataColumnHeaders() + ); + } + + public function testIntegerIndexedColumnsGetTheCsvStyleLabel(): void + { + $applier = $this->createApplier(function (PreQueueRowEvent $event): void { + $event->setRows([[0 => 'A-1', 1 => 'First', 2 => 'synthetic']]); + }); + + $previewData = new PreviewData([0 => 'sku [0]', 1 => 'name [1]'], [0 => 'A-1', 1 => 'First'], 0); + $result = $applier->applyToPreviewData('test_config', [], $previewData); + + $this->assertContains( + ['id' => '2', 'dataIndex' => '2', 'label' => '[2]'], + $result->getDataColumnHeaders() + ); + } + + public function testSkippedRowStaysVisibleInPreview(): void + { + $applier = $this->createApplier(function (PreQueueRowEvent $event): void { + $event->skipRow(); + }); + + $result = $applier->applyToPreviewData('test_config', [], $this->createPreviewData()); + + $this->assertSame(self::ROW, $result->getRawData()); + } + + public function testFanOutShowsFirstRowAndExposesColumnsOfAllRows(): void + { + $applier = $this->createApplier(function (PreQueueRowEvent $event): void { + $row = $event->getOriginalRow(); + $event->setRows([ + $row + ['year' => 2024], + $row + ['year' => 2025, 'extra' => 'x'], + ]); + }); + + $result = $applier->applyToPreviewData('test_config', [], $this->createPreviewData()); + + $this->assertSame(2024, $result->getRawData()['year'], 'first fan-out row is displayed'); + $this->assertSame('x', $result->getRawData()['extra'], 'columns unique to later rows are exposed'); + } + + public function testFileListenerCanReplaceThePreviewPath(): void + { + $applier = $this->createApplier(null, function (PreInterpretFileEvent $event): void { + $this->assertTrue($event->isPreview()); + $event->setPath('/tmp/replacement.csv'); + }); + + $this->assertSame( + '/tmp/replacement.csv', + $applier->applyToPath('test_config', [], '/tmp/original.csv') + ); + } +} From 174bb120917681ebadcf4bd4b21f7e8ceb2d5e70 Mon Sep 17 00:00:00 2001 From: Alexander Batenburg Date: Tue, 18 Aug 2026 19:26:45 +0200 Subject: [PATCH 2/5] Address review feedback on interpretation-stage events - Add declare(strict_types=1) to both new event classes. - PreviewEventApplier: skip dispatch for empty preview records, display the first fan-out row exactly as queued (no value merging from later rows), and drop headers for columns no resulting row contains. - Document that the preview dispatches only the displayed record (stateful listeners should branch on isPreview()) and that interpreters not extending AbstractInterpreter must dispatch the events themselves. - Replace the reflection-based dispatcher-wiring test with a container-level compiler-pass test (fixes Sonar php:S3011, removes the broad Throwable catch). - Add end-to-end regression tests for skipRow() cleanup interaction using a stub load strategy: kept identifier survives cleanup, dropped one does not. Co-Authored-By: Claude Fable 5 --- doc/06_Extending/02_Events.md | 13 ++- src/Event/PreInterpretFileEvent.php | 1 + src/Event/PreQueueRowEvent.php | 1 + src/Preview/PreviewEventApplier.php | 38 +++++-- tests/unit/InterpreterCompilerPassTest.php | 80 ++++++++++++++ tests/unit/InterpreterRowEventTest.php | 122 ++++++++++++++++++--- tests/unit/PreviewEventApplierTest.php | 41 ++++++- 7 files changed, 262 insertions(+), 34 deletions(-) create mode 100644 tests/unit/InterpreterCompilerPassTest.php diff --git a/doc/06_Extending/02_Events.md b/doc/06_Extending/02_Events.md index 4672dcb19..0abe97e62 100644 --- a/doc/06_Extending/02_Events.md +++ b/doc/06_Extending/02_Events.md @@ -30,7 +30,10 @@ that failed, when the failure can be attributed to one. The two interpretation-stage events customize how the source file turns into import rows without writing a custom interpreter. Both expose the configuration name and the execution type, and both are also dispatched (with `isPreview()` returning `true`) when Pimcore Studio renders the source preview and the available mapping columns, so -the configuration UI shows exactly the data an actual import would produce. +the configuration UI shows exactly the data an actual import would produce. The preview dispatches `PreQueueRowEvent` +only for the record being displayed - preceding records are not replayed. Listeners that carry state across rows (for +example a group marker taken from an earlier row) should check `isPreview()` and fall back to stateless behavior, as +the carried state is not available in preview mode. ### PreInterpretFileEvent @@ -97,9 +100,11 @@ deleted or unpublished. A skipped row's element counts as "not seen". Skip rows ::: -Custom interpreters get both events automatically as long as they extend `AbstractInterpreter` (or provide a -`setEventDispatcher()` method) - the interpreter compiler pass wires the dispatcher into every tagged interpreter -service. +Custom interpreters that extend `AbstractInterpreter` get both events automatically: the interpreter compiler pass +wires the event dispatcher into every tagged interpreter service that provides a `setEventDispatcher()` method, and +the base class dispatches the events in `interpretFile()` and `processImportRow()`. An interpreter that implements +`InterpreterInterface` directly still receives the dispatcher through that setter, but must dispatch +`PreInterpretFileEvent` and `PreQueueRowEvent` itself from its own reading loop. ## Example diff --git a/src/Event/PreInterpretFileEvent.php b/src/Event/PreInterpretFileEvent.php index 1b3fccdd0..5573b9921 100644 --- a/src/Event/PreInterpretFileEvent.php +++ b/src/Event/PreInterpretFileEvent.php @@ -1,4 +1,5 @@ getRawData(); + // an empty preview record produces no row event during a real import either + if ($originalRow === []) { + return $previewData; + } + $event = new PreQueueRowEvent( $configName, $this->resolveExecutionType($processingConfig), @@ -73,7 +83,7 @@ public function applyToPreviewData( $this->eventDispatcher->dispatch($event); $rows = $event->getRows(); - if ($rows === [$originalRow]) { + if ($rows === [$originalRow] || $rows === []) { return $previewData; } @@ -82,22 +92,28 @@ public function applyToPreviewData( $labels[$columnHeader['dataIndex']] = $columnHeader['label']; } - $row = $rows[0] ?? $originalRow; - foreach ($rows as $additionalRow) { - foreach ($additionalRow as $index => $value) { - if (!array_key_exists($index, $row)) { - $row[$index] = $value; - } + $resultColumns = []; + foreach ($rows as $resultRow) { + foreach (array_keys($resultRow) as $index) { + $resultColumns[$index] = true; + } + } + + // drop columns no resulting row contains, keeping the original header order + foreach (array_keys($labels) as $index) { + if (!isset($resultColumns[$index]) && !isset($resultColumns[(string) $index])) { + unset($labels[$index]); } } - foreach (array_keys($row) as $index) { + foreach (array_keys($resultColumns) as $index) { if (!array_key_exists($index, $labels) && !array_key_exists((string) $index, $labels)) { $labels[$index] = is_int($index) ? "[$index]" : (string) $index; } } - return new PreviewData($labels, $row, $previewData->getRecordNumber(), $mappedColumns); + // display the first resulting row exactly as it would be queued - no value merging + return new PreviewData($labels, $rows[0], $previewData->getRecordNumber(), $mappedColumns); } private function resolveExecutionType(array $processingConfig): string diff --git a/tests/unit/InterpreterCompilerPassTest.php b/tests/unit/InterpreterCompilerPassTest.php new file mode 100644 index 000000000..0574f57ed --- /dev/null +++ b/tests/unit/InterpreterCompilerPassTest.php @@ -0,0 +1,80 @@ +setDefinition(InterpreterFactory::class, new Definition(InterpreterFactory::class)); + + $csv = new Definition(CsvFileInterpreter::class); + $csv->addTag(self::INTERPRETER_TAG, ['type' => 'csv']); + $container->setDefinition(CsvFileInterpreter::class, $csv); + + // a tagged interpreter without a setEventDispatcher() method + $plain = new Definition(\stdClass::class); + $plain->addTag(self::INTERPRETER_TAG, ['type' => 'plain']); + $container->setDefinition('test.plain_interpreter', $plain); + + (new InterpreterConfigurationFactoryPass())->process($container); + + return $container; + } + + public function testEventDispatcherIsWiredIntoSupportingInterpreters(): void + { + $container = $this->processContainer(); + + $calls = $container->getDefinition(CsvFileInterpreter::class)->getMethodCalls(); + $dispatcherCalls = array_values(array_filter( + $calls, + static fn (array $call): bool => $call[0] === 'setEventDispatcher' + )); + + $this->assertCount(1, $dispatcherCalls); + $this->assertEquals([new Reference('event_dispatcher')], $dispatcherCalls[0][1]); + } + + public function testInterpretersWithoutTheSetterAreLeftAlone(): void + { + $container = $this->processContainer(); + + $this->assertSame([], $container->getDefinition('test.plain_interpreter')->getMethodCalls()); + } + + public function testAllTaggedInterpretersEndUpInTheFactoryBlueprints(): void + { + $container = $this->processContainer(); + + $bluePrints = $container->getDefinition(InterpreterFactory::class)->getArgument('$interpreterBluePrints'); + + $this->assertEquals( + ['csv' => new Reference(CsvFileInterpreter::class), 'plain' => new Reference('test.plain_interpreter')], + $bluePrints + ); + } +} diff --git a/tests/unit/InterpreterRowEventTest.php b/tests/unit/InterpreterRowEventTest.php index e04ad6fdf..64d3b3cb2 100644 --- a/tests/unit/InterpreterRowEventTest.php +++ b/tests/unit/InterpreterRowEventTest.php @@ -20,6 +20,9 @@ use Pimcore\Bundle\DataImporterBundle\Event\PreQueueRowEvent; use Pimcore\Bundle\DataImporterBundle\Processing\ImportProcessingService; use Pimcore\Bundle\DataImporterBundle\Queue\QueueService; +use Pimcore\Bundle\DataImporterBundle\Resolver\Load\LoadStrategyInterface; +use Pimcore\Bundle\DataImporterBundle\Resolver\Resolver; +use Pimcore\Model\Element\ElementInterface; use Psr\Log\NullLogger; use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Contracts\EventDispatcher\EventDispatcherInterface; @@ -62,8 +65,10 @@ private function queueService(): QueueService return new QueueService(); } - private function createInterpreter(?EventDispatcherInterface $eventDispatcher): CsvFileInterpreter - { + private function createInterpreter( + ?EventDispatcherInterface $eventDispatcher, + bool $doCleanup = false + ): CsvFileInterpreter { $interpreter = new CsvFileInterpreter( new DeltaChecker(\Pimcore\Db::get()), $this->queueService(), @@ -74,7 +79,10 @@ private function createInterpreter(?EventDispatcherInterface $eventDispatcher): $interpreter->setExecutionType(ImportProcessingService::EXECUTION_TYPE_SEQUENTIAL); $interpreter->setIdDataIndex('sku'); $interpreter->setDoDeltaCheck(false); - $interpreter->setDoCleanup(false); + $interpreter->setDoCleanup($doCleanup); + if ($doCleanup) { + $interpreter->setResolver($this->createResolver()); + } $interpreter->setDoArchiveImportFile(false); $interpreter->setSettings([ 'skipFirstRow' => true, @@ -97,21 +105,72 @@ private function writeCsv(string $content): string return $path; } + /** + * A resolver whose loading strategy pretends the elements A-1 and B-2 already exist, + * so cleanup behavior can be observed through the cleanup queue items alone. + */ + private function createResolver(): Resolver + { + $resolver = new Resolver(); + $resolver->setLoadingStrategy(new class () implements LoadStrategyInterface { + public function loadElement(array $inputData): ?ElementInterface + { + return null; + } + + public function loadElementByIdentifier($identifier): ?ElementInterface + { + return null; + } + + public function extractIdentifierFromData(array $inputData) + { + return $inputData['sku'] ?? null; + } + + public function loadFullIdentifierList(): array + { + return ['A-1', 'B-2']; + } + + public function setDataObjectClassId($dataObjectClassId): void + { + } + + public function setSettings(array $settings): void + { + } + }); + + return $resolver; + } + /** * @return array */ - private function loadQueuedRows(): array + private function loadQueueEntries(string $jobType): array { $queueService = $this->queueService(); - $rows = []; + $entries = []; foreach ($queueService->getAllQueueEntryIds(ImportProcessingService::EXECUTION_TYPE_SEQUENTIAL) as $id) { $entry = $queueService->getQueueEntryById($id); - if (($entry['configName'] ?? null) === $this->configName) { - $rows[] = json_decode($entry['data'], true); + if (($entry['configName'] ?? null) === $this->configName && ($entry['jobType'] ?? null) === $jobType) { + $entries[] = $entry; } } - return $rows; + return $entries; + } + + /** + * @return array + */ + private function loadQueuedRows(): array + { + return array_map( + static fn (array $entry): array => json_decode($entry['data'], true), + $this->loadQueueEntries(ImportProcessingService::JOB_TYPE_PROCESS) + ); } private function drainQueue(): void @@ -125,18 +184,45 @@ private function drainQueue(): void } } - public function testCompilerPassWiresEventDispatcherIntoTaggedInterpreters(): void + public function testSkippedRowWithKeptIdentifierSurvivesCleanup(): void { - try { - $interpreter = $this->tester->grabService(CsvFileInterpreter::class); - } catch (\Throwable $e) { - // The service is private; retrieving it by id needs a test container - // (framework.test: true), which not every local project setup provides. - $this->markTestSkipped('Service container does not expose private services: ' . $e->getMessage()); - } + $dispatcher = new EventDispatcher(); + $dispatcher->addListener(PreQueueRowEvent::class, function (PreQueueRowEvent $event): void { + if ($event->getOriginalRow()['sku'] === 'B-2') { + $event->skipRow(keepInCleanupIdentifierCache: true); + } + }); + + $this->createInterpreter($dispatcher, doCleanup: true)->interpretFile($this->writeCsv(self::CSV)); + + $rows = $this->loadQueuedRows(); + $this->assertCount(1, $rows); + $this->assertSame('A-1', $rows[0]['sku']); + $this->assertSame( + [], + $this->loadQueueEntries(ImportProcessingService::JOB_TYPE_CLEANUP), + 'the skipped row kept its identifier, so its existing element must not be cleaned up' + ); + } - $property = new \ReflectionProperty($interpreter, 'eventDispatcher'); - $this->assertInstanceOf(EventDispatcherInterface::class, $property->getValue($interpreter)); + public function testSkippedRowWithoutKeptIdentifierGetsCleanedUp(): void + { + $dispatcher = new EventDispatcher(); + $dispatcher->addListener(PreQueueRowEvent::class, function (PreQueueRowEvent $event): void { + if ($event->getOriginalRow()['sku'] === 'B-2') { + $event->skipRow(); + } + }); + + $this->createInterpreter($dispatcher, doCleanup: true)->interpretFile($this->writeCsv(self::CSV)); + + $cleanupEntries = $this->loadQueueEntries(ImportProcessingService::JOB_TYPE_CLEANUP); + $this->assertCount(1, $cleanupEntries); + $this->assertSame( + 'B-2', + $cleanupEntries[0]['data'], + 'a skipped row without a kept identifier counts as removed from the source' + ); } public function testRowsAreQueuedUnchangedWithoutListeners(): void diff --git a/tests/unit/PreviewEventApplierTest.php b/tests/unit/PreviewEventApplierTest.php index be4829ae3..916fc5818 100644 --- a/tests/unit/PreviewEventApplierTest.php +++ b/tests/unit/PreviewEventApplierTest.php @@ -111,7 +111,46 @@ public function testFanOutShowsFirstRowAndExposesColumnsOfAllRows(): void $result = $applier->applyToPreviewData('test_config', [], $this->createPreviewData()); $this->assertSame(2024, $result->getRawData()['year'], 'first fan-out row is displayed'); - $this->assertSame('x', $result->getRawData()['extra'], 'columns unique to later rows are exposed'); + $this->assertArrayNotHasKey( + 'extra', + $result->getRawData(), + 'the displayed row must be exactly the first queued row - no values merged from later rows' + ); + $this->assertContains( + ['id' => 'extra', 'dataIndex' => 'extra', 'label' => 'extra'], + $result->getDataColumnHeaders(), + 'columns unique to later fan-out rows are still mappable' + ); + } + + public function testColumnsRemovedByTheListenerDisappearFromTheHeaders(): void + { + $applier = $this->createApplier(function (PreQueueRowEvent $event): void { + $row = $event->getOriginalRow(); + unset($row['name']); + $event->setRows([$row]); + }); + + $result = $applier->applyToPreviewData('test_config', [], $this->createPreviewData()); + + $this->assertSame(['sku' => 'A-1'], $result->getRawData()); + $this->assertNotContains( + ['id' => 'name', 'dataIndex' => 'name', 'label' => 'name'], + $result->getDataColumnHeaders(), + 'a column no queued row contains must not stay visible or mappable' + ); + } + + public function testEmptyPreviewRecordDispatchesNoEvent(): void + { + $applier = $this->createApplier(function (PreQueueRowEvent $event): void { + $this->fail('a real import emits no row event for an empty source, so preview must not either'); + }); + + $previewData = new PreviewData([], [], -1); + $result = $applier->applyToPreviewData('test_config', [], $previewData); + + $this->assertSame($previewData, $result); } public function testFileListenerCanReplaceThePreviewPath(): void From 2fa46fc2e0749dea8b586cf5795017c1ca269cca Mon Sep 17 00:00:00 2001 From: Alexander Batenburg Date: Tue, 18 Aug 2026 19:31:00 +0200 Subject: [PATCH 3/5] Resolve SonarCloud findings from the review-feedback commit Extract label building out of applyToPreviewData (cognitive complexity), comment the intentionally empty stub methods, and drop the unused closure parameter in the empty-record preview test. Co-Authored-By: Claude Fable 5 --- src/Preview/PreviewEventApplier.php | 28 ++++++++++++++++++++++---- tests/unit/InterpreterRowEventTest.php | 2 ++ tests/unit/PreviewEventApplierTest.php | 2 +- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/Preview/PreviewEventApplier.php b/src/Preview/PreviewEventApplier.php index 25b83bdb4..ac6ff128a 100644 --- a/src/Preview/PreviewEventApplier.php +++ b/src/Preview/PreviewEventApplier.php @@ -87,6 +87,23 @@ public function applyToPreviewData( return $previewData; } + // display the first resulting row exactly as it would be queued - no value merging + return new PreviewData( + $this->buildResultLabels($previewData, $rows), + $rows[0], + $previewData->getRecordNumber(), + $mappedColumns + ); + } + + /** + * Restricts the original column headers to the columns the resulting rows actually + * contain (keeping the original order) and adds labels for listener-added columns. + * + * @param array $rows + */ + private function buildResultLabels(PreviewData $previewData, array $rows): array + { $labels = []; foreach ($previewData->getDataColumnHeaders() as $columnHeader) { $labels[$columnHeader['dataIndex']] = $columnHeader['label']; @@ -99,7 +116,6 @@ public function applyToPreviewData( } } - // drop columns no resulting row contains, keeping the original header order foreach (array_keys($labels) as $index) { if (!isset($resultColumns[$index]) && !isset($resultColumns[(string) $index])) { unset($labels[$index]); @@ -107,13 +123,17 @@ public function applyToPreviewData( } foreach (array_keys($resultColumns) as $index) { - if (!array_key_exists($index, $labels) && !array_key_exists((string) $index, $labels)) { + if (!$this->hasLabel($labels, $index)) { $labels[$index] = is_int($index) ? "[$index]" : (string) $index; } } - // display the first resulting row exactly as it would be queued - no value merging - return new PreviewData($labels, $rows[0], $previewData->getRecordNumber(), $mappedColumns); + return $labels; + } + + private function hasLabel(array $labels, int|string $index): bool + { + return array_key_exists($index, $labels) || array_key_exists((string) $index, $labels); } private function resolveExecutionType(array $processingConfig): string diff --git a/tests/unit/InterpreterRowEventTest.php b/tests/unit/InterpreterRowEventTest.php index 64d3b3cb2..a774b8ff8 100644 --- a/tests/unit/InterpreterRowEventTest.php +++ b/tests/unit/InterpreterRowEventTest.php @@ -135,10 +135,12 @@ public function loadFullIdentifierList(): array public function setDataObjectClassId($dataObjectClassId): void { + // the stub resolves identifiers from the row data alone } public function setSettings(array $settings): void { + // the stub needs no configuration } }); diff --git a/tests/unit/PreviewEventApplierTest.php b/tests/unit/PreviewEventApplierTest.php index 916fc5818..e51e22b10 100644 --- a/tests/unit/PreviewEventApplierTest.php +++ b/tests/unit/PreviewEventApplierTest.php @@ -143,7 +143,7 @@ public function testColumnsRemovedByTheListenerDisappearFromTheHeaders(): void public function testEmptyPreviewRecordDispatchesNoEvent(): void { - $applier = $this->createApplier(function (PreQueueRowEvent $event): void { + $applier = $this->createApplier(function (): void { $this->fail('a real import emits no row event for an empty source, so preview must not either'); }); From ca63409bb92595cedf2541a10469a9cc97e85f04 Mon Sep 17 00:00:00 2001 From: Alexander Batenburg Date: Mon, 19 Oct 2026 08:00:05 +0200 Subject: [PATCH 4/5] Address second review round on interpretation-stage events - PreviewDataService: keep a listener's replacement path separate from the local preview copy, so the JSON-validation cleanup can never delete a file owned by a PreInterpretFileEvent listener. - InterpreterConfigurationFactoryPass: resolve the class through parent definitions and container parameters before checking for setEventDispatcher(), and skip services that already inherit the call. Parents are processed first so a child never gets the call twice. Co-Authored-By: Claude Opus 5.5 --- .../InterpreterConfigurationFactoryPass.php | 73 +++++++++++++++---- src/Service/Studio/PreviewDataService.php | 8 +- tests/unit/InterpreterCompilerPassTest.php | 69 ++++++++++++++++-- 3 files changed, 129 insertions(+), 21 deletions(-) diff --git a/src/DependencyInjection/CompilerPass/InterpreterConfigurationFactoryPass.php b/src/DependencyInjection/CompilerPass/InterpreterConfigurationFactoryPass.php index 7ed2c6957..6cd842feb 100644 --- a/src/DependencyInjection/CompilerPass/InterpreterConfigurationFactoryPass.php +++ b/src/DependencyInjection/CompilerPass/InterpreterConfigurationFactoryPass.php @@ -13,8 +13,10 @@ namespace Pimcore\Bundle\DataImporterBundle\DependencyInjection\CompilerPass; use Pimcore\Bundle\DataImporterBundle\DataSource\Interpreter\InterpreterFactory; +use Symfony\Component\DependencyInjection\ChildDefinition; use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface; use Symfony\Component\DependencyInjection\ContainerBuilder; +use Symfony\Component\DependencyInjection\Definition; use Symfony\Component\DependencyInjection\Reference; /** @@ -28,25 +30,70 @@ public function process(ContainerBuilder $container): void { $taggedServices = $container->findTaggedServiceIds(self::INTERPRETER_TAG); $interpreters = []; - if (sizeof($taggedServices)) { - foreach ($taggedServices as $id => $tags) { - foreach ($tags as $attributes) { - $interpreters[$attributes['type']] = new Reference($id); - } + foreach ($taggedServices as $id => $tags) { + foreach ($tags as $attributes) { + $interpreters[$attributes['type']] = new Reference($id); + } + } - // Wire the event dispatcher into every interpreter that supports it (built-in - // and custom alike), so PreInterpretFileEvent/PreQueueRowEvent are dispatched. - $definition = $container->getDefinition($id); - $class = $definition->getClass() ?? $id; - if (method_exists($class, 'setEventDispatcher') - && !$definition->hasMethodCall('setEventDispatcher') - ) { - $definition->addMethodCall('setEventDispatcher', [new Reference('event_dispatcher')]); + // Wire the event dispatcher into every interpreter that supports it (built-in + // and custom alike), so PreInterpretFileEvent/PreQueueRowEvent are dispatched. + // This pass runs before child definitions and class parameters are resolved, so + // both are resolved here. Parents go first: a child inherits their method calls. + $chains = []; + foreach (array_keys($taggedServices) as $id) { + $chains[$id] = $this->getDefinitionChain($container, $id); + } + uasort($chains, static fn (array $a, array $b): int => count($a) <=> count($b)); + + foreach ($chains as $id => $chain) { + $class = $this->resolveClass($container, $chain, $id); + if ($class === null || !method_exists($class, 'setEventDispatcher')) { + continue; + } + + foreach ($chain as $definition) { + if ($definition->hasMethodCall('setEventDispatcher')) { + continue 2; } } + + $chain[0]->addMethodCall('setEventDispatcher', [new Reference('event_dispatcher')]); } $serviceLocator = $container->getDefinition(InterpreterFactory::class); $serviceLocator->setArgument('$interpreterBluePrints', $interpreters); } + + /** + * @return list the service definition followed by its parent definitions + */ + private function getDefinitionChain(ContainerBuilder $container, string $id): array + { + $chain = [$container->getDefinition($id)]; + while (end($chain) instanceof ChildDefinition && $container->hasDefinition(end($chain)->getParent())) { + $chain[] = $container->getDefinition(end($chain)->getParent()); + } + + return $chain; + } + + /** + * @param list $chain + */ + private function resolveClass(ContainerBuilder $container, array $chain, string $id): ?string + { + $class = $id; + foreach ($chain as $definition) { + if ($definition->getClass() !== null) { + $class = $definition->getClass(); + + break; + } + } + + $class = $container->getParameterBag()->resolveValue($class); + + return is_string($class) ? $class : null; + } } diff --git a/src/Service/Studio/PreviewDataService.php b/src/Service/Studio/PreviewDataService.php index 5f9b4354c..123929be3 100644 --- a/src/Service/Studio/PreviewDataService.php +++ b/src/Service/Studio/PreviewDataService.php @@ -186,20 +186,22 @@ public function loadPreviewData( $preparedConfig['processingConfig'] ); - $previewFilePath = $this->previewEventApplier->applyToPath( + // a listener may replace the path with a file it owns - only the local preview + // copy ($previewFilePath) is ever deleted below + $interpretedFilePath = $this->previewEventApplier->applyToPath( $name, $preparedConfig['processingConfig'], $previewFilePath ); - if (!$interpreter->fileValid($previewFilePath)) { + if (!$interpreter->fileValid($interpretedFilePath)) { throw new EnvironmentException( 'Preview file is not valid for the configured interpreter. ' . 'Please re-upload or re-copy the preview data.' ); } - $dataPreview = $interpreter->previewData($previewFilePath, $recordNumber, $mappedColumns); + $dataPreview = $interpreter->previewData($interpretedFilePath, $recordNumber, $mappedColumns); $dataPreview = $this->previewEventApplier->applyToPreviewData( $name, $preparedConfig['processingConfig'], diff --git a/tests/unit/InterpreterCompilerPassTest.php b/tests/unit/InterpreterCompilerPassTest.php index 0574f57ed..a8adac93b 100644 --- a/tests/unit/InterpreterCompilerPassTest.php +++ b/tests/unit/InterpreterCompilerPassTest.php @@ -16,6 +16,7 @@ use Pimcore\Bundle\DataImporterBundle\DataSource\Interpreter\CsvFileInterpreter; use Pimcore\Bundle\DataImporterBundle\DataSource\Interpreter\InterpreterFactory; use Pimcore\Bundle\DataImporterBundle\DependencyInjection\CompilerPass\InterpreterConfigurationFactoryPass; +use Symfony\Component\DependencyInjection\ChildDefinition; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Definition; use Symfony\Component\DependencyInjection\Reference; @@ -45,20 +46,78 @@ private function processContainer(): ContainerBuilder return $container; } - public function testEventDispatcherIsWiredIntoSupportingInterpreters(): void + private function emptyContainer(): ContainerBuilder { - $container = $this->processContainer(); + $container = new ContainerBuilder(); + $container->setDefinition(InterpreterFactory::class, new Definition(InterpreterFactory::class)); - $calls = $container->getDefinition(CsvFileInterpreter::class)->getMethodCalls(); - $dispatcherCalls = array_values(array_filter( - $calls, + return $container; + } + + /** + * @return list> the setEventDispatcher calls of the definition + */ + private function dispatcherCalls(Definition $definition): array + { + return array_values(array_filter( + $definition->getMethodCalls(), static fn (array $call): bool => $call[0] === 'setEventDispatcher' )); + } + + public function testEventDispatcherIsWiredIntoSupportingInterpreters(): void + { + $container = $this->processContainer(); + + $dispatcherCalls = $this->dispatcherCalls($container->getDefinition(CsvFileInterpreter::class)); $this->assertCount(1, $dispatcherCalls); $this->assertEquals([new Reference('event_dispatcher')], $dispatcherCalls[0][1]); } + public function testClassInheritedFromAnUntaggedParentIsResolved(): void + { + $container = $this->emptyContainer(); + $container->setDefinition('test.interpreter_parent', (new Definition(CsvFileInterpreter::class))->setAbstract(true)); + $child = new ChildDefinition('test.interpreter_parent'); + $child->addTag(self::INTERPRETER_TAG, ['type' => 'child']); + $container->setDefinition('test.child_interpreter', $child); + + (new InterpreterConfigurationFactoryPass())->process($container); + + $this->assertCount(1, $this->dispatcherCalls($container->getDefinition('test.child_interpreter'))); + } + + public function testClassParameterIsResolved(): void + { + $container = $this->emptyContainer(); + $container->setParameter('test.interpreter_class', CsvFileInterpreter::class); + $definition = new Definition('%test.interpreter_class%'); + $definition->addTag(self::INTERPRETER_TAG, ['type' => 'parameterized']); + $container->setDefinition('test.parameterized_interpreter', $definition); + + (new InterpreterConfigurationFactoryPass())->process($container); + + $this->assertCount(1, $this->dispatcherCalls($container->getDefinition('test.parameterized_interpreter'))); + } + + public function testDispatcherCallInheritedFromAParentIsNotDuplicated(): void + { + // the child is registered first, so the parent's call must be added before the child is checked + $container = $this->emptyContainer(); + $child = new ChildDefinition('test.parent_interpreter'); + $child->addTag(self::INTERPRETER_TAG, ['type' => 'child']); + $container->setDefinition('test.child_interpreter', $child); + $parent = new Definition(CsvFileInterpreter::class); + $parent->addTag(self::INTERPRETER_TAG, ['type' => 'parent']); + $container->setDefinition('test.parent_interpreter', $parent); + + (new InterpreterConfigurationFactoryPass())->process($container); + + $this->assertCount(1, $this->dispatcherCalls($container->getDefinition('test.parent_interpreter'))); + $this->assertSame([], $this->dispatcherCalls($container->getDefinition('test.child_interpreter'))); + } + public function testInterpretersWithoutTheSetterAreLeftAlone(): void { $container = $this->processContainer(); From 9513c5e934975473c6e02d1391c75d5b4392e7ee Mon Sep 17 00:00:00 2001 From: Alexander Batenburg Date: Mon, 19 Oct 2026 08:24:20 +0200 Subject: [PATCH 5/5] Split an over-long line in InterpreterCompilerPassTest Co-Authored-By: Claude Opus 5.5 --- tests/unit/InterpreterCompilerPassTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/InterpreterCompilerPassTest.php b/tests/unit/InterpreterCompilerPassTest.php index a8adac93b..b3b385b7d 100644 --- a/tests/unit/InterpreterCompilerPassTest.php +++ b/tests/unit/InterpreterCompilerPassTest.php @@ -78,7 +78,8 @@ public function testEventDispatcherIsWiredIntoSupportingInterpreters(): void public function testClassInheritedFromAnUntaggedParentIsResolved(): void { $container = $this->emptyContainer(); - $container->setDefinition('test.interpreter_parent', (new Definition(CsvFileInterpreter::class))->setAbstract(true)); + $parent = (new Definition(CsvFileInterpreter::class))->setAbstract(true); + $container->setDefinition('test.interpreter_parent', $parent); $child = new ChildDefinition('test.interpreter_parent'); $child->addTag(self::INTERPRETER_TAG, ['type' => 'child']); $container->setDefinition('test.child_interpreter', $child);