[With or Without IDs] Export and Import should be possible without IDs#20
[With or Without IDs] Export and Import should be possible without IDs#20lukadschaak merged 25 commits intomainfrom
Conversation
WalkthroughThis update introduces a modular approach for exporting and importing Pimcore elements, documents, assets, and data objects. It adds context-sensitive handling for including IDs, overhauls document property management, and implements a strategy pattern for merging elements during import. The UI for export options is improved with modal dialogs, and extensive event-driven logging and statistics are added. Numerous new classes, configuration changes, and tests support these enhancements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (JS Export Dialog)
participant Controller
participant Exporter
participant Converter
participant Populator
participant Serializer
User->>UI (JS Export Dialog): Initiate export (choose options)
UI (JS Export Dialog)->>Controller: Send export request (filename, include_ids)
Controller->>Exporter: Call export(elements, format, {includeIds})
Exporter->>Converter: convert(element, context)
Converter->>Populator: populate(target, source, context)
Populator-->>Converter: (conditionally sets id/parentId if includeIds)
Converter-->>Exporter: Return converted element
Exporter->>Serializer: serialize(converted elements, format)
Serializer-->>Exporter: Return serialized data
Exporter-->>Controller: Return exported file
Controller-->>UI (JS Export Dialog): Respond with file download
UI (JS Export Dialog)-->>User: Download completed
sequenceDiagram
participant User
participant Controller (Import)
participant Importer
participant Repository
participant MergeStrategy
participant EventDispatcher
participant Logger
User->>Controller (Import): Upload import file (with overwrite option)
Controller (Import)->>Importer: import(yaml, format, forcedSave, overwrite)
Importer->>Repository: findByPath(path)
alt No existing element
Importer->>Repository: save(newElement)
Importer->>EventDispatcher: Dispatch CREATED event
else Existing element found
alt overwrite && (no valid id or same id)
Importer->>MergeStrategy: mergeAndSave(old, new)
Importer->>EventDispatcher: Dispatch UPDATED event
else id mismatch
Importer->>EventDispatcher: Dispatch INCONSISTENCY event
EventDispatcher->>Logger: Log inconsistency error
else overwrite == false
Importer->>EventDispatcher: Dispatch SKIPPED event
end
end
Controller (Import)-->>User: Return import result (with statistics)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/pimcore/export/documents/converters_populators.yaml (2)
15-15: Unify service-ID notation for populatorsOther populators in this list use the explicit service aliases (e.g.
neusta_pimcore_import_export.page.property.language.populator), whereas the new entry relies on the FQCN.
Symfony treats the FQCN as a valid service ID, so this works, but mixing the two styles in the same array is noisy and invites future copy-paste mistakes.Either register an alias (e.g.
neusta_pimcore_import_export.ids.populator) and reference that here, or switch all entries to FQCNs for consistency.
77-78: Service definition may be redundantBecause
_defaults.autowire/autoconfigureis enabled and the IdsPopulator lives undersrc/, the standardservices.yamlresource import already registers the class as a service.
Keeping an explicit, empty definition here is harmless but clutters the file and can become a trap if constructor arguments are later added and this stub is forgotten.If no bundle-specific visibility tweak is required, consider removing the block:
- Neusta\Pimcore\ImportExportBundle\Populator\IdsPopulator: ~
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
config/pimcore/export/assets/converters_populators.yaml(2 hunks)config/pimcore/export/documents/converters_populators.yaml(2 hunks)config/pimcore/export/elements/converters_populators.yaml(1 hunks)config/pimcore/export/objects/converters_populators.yaml(2 hunks)phpunit.xml.dist(1 hunks)public/js/exportAsset.js(1 hunks)public/js/exportDataObjects.js(1 hunks)public/js/exportDocument.js(1 hunks)src/Command/Base/AbstractExportBaseCommand.php(1 hunks)src/Command/ExportAssetsCommand.php(1 hunks)src/Command/ExportDataObjectsCommand.php(1 hunks)src/Command/ExportDocumentsCommand.php(1 hunks)src/Controller/Admin/ExportAssetsController.php(2 hunks)src/Controller/Admin/ExportDataObjectsController.php(2 hunks)src/Controller/Admin/ExportDocumentsController.php(2 hunks)src/Export/Exporter.php(1 hunks)src/Model/Element.php(1 hunks)src/Populator/IdsPopulator.php(1 hunks)tests/Integration/Export/ExporterTest.php(3 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_simple_saved_pages_export__1.yaml(0 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_simple_unsaved_pages_export__1.yaml(0 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_single_image_export__1.yaml(0 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_single_page_export__1.yaml(0 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_single_page_export_json__1.json(0 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_tree_pages_export__1.yaml(0 hunks)translations/admin.de.yaml(1 hunks)translations/admin.en.yaml(1 hunks)
💤 Files with no reviewable changes (6)
- tests/Integration/Export/snapshots/ExporterTest__test_simple_saved_pages_export__1.yaml
- tests/Integration/Export/snapshots/ExporterTest__test_simple_unsaved_pages_export__1.yaml
- tests/Integration/Export/snapshots/ExporterTest__test_single_page_export__1.yaml
- tests/Integration/Export/snapshots/ExporterTest__test_tree_pages_export__1.yaml
- tests/Integration/Export/snapshots/ExporterTest__test_single_image_export__1.yaml
- tests/Integration/Export/snapshots/ExporterTest__test_single_page_export_json__1.json
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
config/pimcore/export/assets/converters_populators.yaml (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
config/pimcore/export/objects/converters_populators.yaml (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Populator/IdsPopulator.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
config/pimcore/export/documents/converters_populators.yaml (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
config/pimcore/export/elements/converters_populators.yaml (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
🧬 Code Graph Analysis (8)
public/js/exportAsset.js (2)
public/js/exportDataObjects.js (3)
includeIds(24-24)filename(26-26)defaultFilename(23-23)public/js/exportDocument.js (3)
includeIds(24-24)filename(26-26)defaultFilename(23-23)
src/Command/ExportDataObjectsCommand.php (1)
src/Export/Exporter.php (1)
export(34-52)
src/Controller/Admin/ExportAssetsController.php (2)
public/js/exportAsset.js (2)
filename(26-26)includeIds(24-24)src/Export/Exporter.php (1)
export(34-52)
public/js/exportDataObjects.js (2)
public/js/exportAsset.js (3)
includeIds(24-24)filename(26-26)defaultFilename(23-23)public/js/exportDocument.js (3)
includeIds(24-24)filename(26-26)defaultFilename(23-23)
src/Command/ExportDocumentsCommand.php (1)
src/Export/Exporter.php (1)
export(34-52)
public/js/exportDocument.js (2)
public/js/exportDataObjects.js (3)
includeIds(24-24)filename(26-26)defaultFilename(23-23)public/js/exportAsset.js (3)
includeIds(24-24)filename(26-26)defaultFilename(23-23)
src/Command/ExportAssetsCommand.php (1)
src/Export/Exporter.php (1)
export(34-52)
src/Populator/IdsPopulator.php (1)
src/Model/Element.php (1)
Element(5-15)
🔇 Additional comments (39)
phpunit.xml.dist (1)
18-18: LGTM: Standard test configuration updateThis environment variable addition correctly configures the test kernel for the expanded integration test coverage related to the new export functionality with ID inclusion options.
translations/admin.de.yaml (1)
1-1: LGTM: Clear German translation for ID exclusion promptThe translation accurately explains the behavior when IDs are excluded during export, informing users that mapping will be done via key and path instead.
translations/admin.en.yaml (1)
1-1: LGTM: Clear English translation for ID exclusion promptThe translation clearly communicates the export behavior when IDs are excluded, maintaining consistency with the German translation and providing clear user guidance.
src/Command/ExportDocumentsCommand.php (1)
88-88: LGTM: Correctly passes includeIds option to exporterThe change properly propagates the
includeIdscommand option to the exporter service through the context parameters array, enabling conditional ID inclusion in document exports.public/js/exportAsset.js (2)
24-24: LGTM: Correct implementation of user ID exclusion promptThe confirm dialog correctly uses the translation key and properly inverts the boolean logic (using
!) since confirm returnstruefor "Yes" but we wantincludeIdsto befalsewhen the user confirms they want to exclude IDs.
28-28: LGTM: Properly passes includeIds parameter to backendThe
ids_includedparameter is correctly added to the download URL, enabling the backend to receive the user's choice about including IDs in the export. The parameter name matches the expected backend interface.src/Command/ExportAssetsCommand.php (1)
82-82: LGTM! Clean integration of the includeIds option.The change correctly passes the
includeIdscommand line option as a context parameter to the exporter, enabling conditional ID inclusion during asset export.src/Command/Base/AbstractExportBaseCommand.php (1)
40-45: Good implementation with helpful warning message.The
includeIdsoption is properly implemented as a boolean flag with an appropriate warning about re-importing risks. The option will be inherited by all export commands, ensuring consistency across the export functionality.src/Model/Element.php (1)
9-10: Excellent approach to prevent unwanted null value exports.Removing the default values for
$idand$parentIdis a smart solution that prevents automatic serialization of null/zero values when IDs should be excluded. The explanatory comments make the intention clear for future maintainers.public/js/exportDocument.js (2)
24-25: Consistent user experience across export functions.The confirmation prompt correctly implements the user choice for ID inclusion, with clear documentation of the negation logic. This matches the implementation in other export JavaScript files.
28-28: Proper parameter passing to backend.The
ids_includedparameter is correctly passed to the backend route, enabling the server-side export logic to respect the user's choice about ID inclusion.src/Command/ExportDataObjectsCommand.php (1)
84-84: Consistent implementation across export commands.The change correctly mirrors the pattern used in other export commands, passing the
includeIdsoption as a context parameter to enable conditional ID inclusion during data object export.public/js/exportDataObjects.js (2)
24-25: Good implementation with clear documentation.The logic correctly inverts the confirm dialog response since the question asks about excluding IDs but the variable represents including IDs. The inline comment effectively clarifies this potentially confusing inversion.
28-28: Parameter passing implemented correctly.The
ids_includedparameter is properly passed to the backend route, enabling the conditional ID inclusion functionality.config/pimcore/export/assets/converters_populators.yaml (2)
11-12: Good architectural improvement with conditional ID handling.Adding the IdsPopulator to the asset folder converter enables conditional inclusion of ID fields based on export context, which aligns with the PR's objective of making ID export configurable.
90-91: Service registration follows Symfony best practices.The IdsPopulator service is properly registered with appropriate defaults for autowiring and autoconfiguration.
src/Export/Exporter.php (3)
30-30: Well-designed method signature extension.The addition of the optional
$ctxParamsparameter maintains backward compatibility while enabling flexible context passing to converters and populators. The type annotation is clear and appropriate.Also applies to: 34-34
36-40: Context creation implemented correctly.The GenericContext is properly instantiated and populated with the provided parameters using the standard setValue method from the converter bundle.
45-45: Context properly passed to converters.The context parameter is correctly passed to the converter's convert method, enabling populators to access export configuration like the includeIds flag.
src/Populator/IdsPopulator.php (3)
1-8: Standard file structure with correct imports.The file header and imports follow PHP and project conventions correctly.
9-13: Proper implementation of Populator interface.The class correctly implements the Populator interface with appropriate generic type annotations. The implementation aligns with the established pattern where populators are used exclusively within converters for type safety.
14-21: Conditional ID population implemented correctly.The populate method efficiently checks the context for the 'includeIds' flag and only populates ID fields when explicitly requested. The null-safe operator usage and boolean check ensure robust operation.
config/pimcore/export/objects/converters_populators.yaml (3)
12-12: IdsPopulator correctly added to main object converter.The IdsPopulator is appropriately added to the main export converter, enabling conditional ID inclusion for data object exports. The placement as the first populator ensures IDs are populated before other field processing.
22-34: Verify the intentional inconsistency in ID handling.The
export_object.without.relationsconverter still includes explicitidandparentIdproperties (lines 28, 33) while the main converter now uses IdsPopulator for conditional ID inclusion. This creates inconsistent behavior where nested objects might always include IDs regardless of theincludeIdscontext flag.Please verify if this inconsistency is intentional. If the "without relations" converter should also respect the conditional ID inclusion, consider adding IdsPopulator to its populators list and removing the explicit ID properties.
49-50: Service registration follows established pattern.The IdsPopulator service is properly registered with appropriate defaults, consistent with the same service registration in other export configuration files.
src/Controller/Admin/ExportAssetsController.php (3)
40-45: LGTM! Consistent implementation of conditional ID inclusion.The changes properly extract the
ids_includedquery parameter with a sensible default offalseand pass it through to the export method.
66-71: LGTM! Consistent parameter handling in exportWithChildren.The implementation mirrors the
exportmethod, maintaining consistency across both endpoints.
77-84: LGTM! Proper integration with the exporter service.The method signature update and context parameter passing correctly integrate with the enhanced exporter that supports conditional ID inclusion.
src/Controller/Admin/ExportDataObjectsController.php (3)
37-42: LGTM! Consistent implementation across export controllers.The parameter extraction and passing follows the same pattern established in other export controllers, maintaining consistency.
62-67: LGTM! Proper handling of the includeIds parameter.The
exportWithChildrenmethod correctly implements the same pattern as the single export method.
73-82: LGTM! Well-structured context parameter passing.The array formatting for the context parameters is clean and properly integrates with the exporter service.
src/Controller/Admin/ExportDocumentsController.php (3)
37-42: LGTM! Maintains consistency with other export controllers.The implementation follows the established pattern across all export controllers, ensuring a uniform API.
62-67: LGTM! Consistent parameter handling.The
exportWithChildrenmethod properly implements the same logic as the single document export.
73-82: LGTM! Proper exporter service integration.The context parameter structure is consistent and correctly formatted for the exporter service.
config/pimcore/export/elements/converters_populators.yaml (2)
11-12: LGTM! Proper populator registration.The
IdsPopulatoris correctly added to the populators list, following the bundle's architecture where populators are used exclusively within converters.
18-23: LGTM! Proper service configuration.The service registration follows Symfony best practices with autowiring and autoconfiguration enabled, ensuring the
IdsPopulatorservice is properly available for dependency injection.tests/Integration/Export/ExporterTest.php (3)
38-49: LGTM! Comprehensive test coverage for ID inclusion.The new test properly verifies that IDs are included when the
includeIdscontext parameter is set totrue. Using snapshot testing ensures the output format is exactly as expected.
59-65: LGTM! Consistent test pattern for page exports.The test follows the same pattern as the image test, ensuring consistent coverage across different entity types.
111-124: LGTM! Tree structure export testing with IDs.This test is particularly valuable as it verifies that parent-child relationships are properly exported when IDs are included, which is crucial for maintaining data integrity during import.
mike4git
force-pushed
the
feature/include-ids-optionally
branch
from
9343e43 to
a57db50
Compare
…ionally IDs included
mike4git
force-pushed
the
feature/include-ids-optionally
branch
from
a57db50 to
42ce4ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Controller/Admin/Base/AbstractImportBaseController.php (2)
23-23: Consider using PHP enums for better type safety.The new
FAILURE_INCONSISTENCYconstant and corresponding message are correctly implemented and align with the PR objectives. However, based on previous feedback, consider migrating from integer constants to PHP enums for better type safety, code completion, and self-documentation.Example enum implementation:
enum ImportResult: int { case ERR_NO_FILE_UPLOADED = 1; case SUCCESS_ELEMENT_REPLACEMENT = 2; case SUCCESS_WITHOUT_REPLACEMENT = 3; case SUCCESS_NEW_ELEMENT = 4; case FAILURE_INCONSISTENCY = 5; public function getMessage(): string { return match($this) { self::ERR_NO_FILE_UPLOADED => 'No file uploaded', self::SUCCESS_ELEMENT_REPLACEMENT => 'replaced successfully', self::SUCCESS_WITHOUT_REPLACEMENT => 'not replaced', self::SUCCESS_NEW_ELEMENT => 'imported successfully', self::FAILURE_INCONSISTENCY => 'failed due to inconsistency in the data', }; } }Also applies to: 33-33
125-125: Improve HTML table structure for better accessibility.The change from plain text to HTML table format significantly improves result presentation. Consider enhancing the table structure for better accessibility and styling.
- $resultMessage = '<table><tr><th>' . $this->elementType . '</th><th>Count</th></tr>'; + $resultMessage = '<table class="import-results"><thead><tr><th>' . $this->elementType . '</th><th>Count</th></tr></thead><tbody>';- $resultMessage .= '<tr><td>'; - $resultMessage .= $this->messagesMap[$resultCode] . '</td><td>' . $result . '</td></tr>'; + $resultMessage .= '<tr><td>' . $this->messagesMap[$resultCode] . '</td><td>' . $result . '</td></tr>';- return $resultMessage . '</table>'; + return $resultMessage . '</tbody></table>';This adds proper HTML structure with
theadandtbodyelements and a CSS class for potential styling.Also applies to: 129-131, 134-134
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Controller/Admin/Base/AbstractImportBaseController.php(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/Base/AbstractImportBaseController.php (1)
Learnt from: jan888adams
PR: #10
File: src/Controller/Admin/PageImportController.php:23-23
Timestamp: 2025-04-01T09:57:30.185Z
Learning: We should use PHP enums instead of integer constants with message mapping arrays where possible, as they provide better type safety, code completion, and self-documentation.
🔇 Additional comments (1)
src/Controller/Admin/Base/AbstractImportBaseController.php (1)
53-53: LGTM! Proper statistics initialization.The addition of
FAILURE_INCONSISTENCYto the result statistics initialization is correct and maintains consistency with the existing pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
translations/admin.de.yaml (1)
1-6: German translations look good with minor suggestion.The German translations are accurate and consistent. One minor suggestion for line 6: consider using "deaktiviere" instead of "aktiviere" to match the logic (since the checkbox excludes IDs when checked).
-neusta_pimcore_import_export_exclude_ids_info: 'Es werden keine IDs (id/parentId) exportiert. Die Zuordnung geschieht über Key und Pfad.<br/>Um IDs zu exportieren, aktiviere bitte diese Option.' +neusta_pimcore_import_export_exclude_ids_info: 'Es werden keine IDs (id/parentId) exportiert. Die Zuordnung geschieht über Key und Pfad.<br/>Um IDs zu exportieren, deaktiviere bitte diese Option.'src/Serializer/Normalizer/PrioritizedAttributesNormalizer.php (1)
31-49: Improve return type specificity and error handling.The normalize method should be more explicit about its return types and handle edge cases better.
- /** - * @param object $object - * @param string|null $format - * @param array<string, mixed> $context - * - * @return array<string, mixed> - */ - public function normalize($object, ?string $format = null, array $context = []) + /** + * @param object $object + * @param string|null $format + * @param array<string, mixed> $context + * + * @return array<string, mixed>|string|int|float|bool|null + */ + public function normalize($object, ?string $format = null, array $context = []): mixed { $data = $this->normalizer->normalize($object, $format, $context); - if (!\is_array($data)) { + if (!is_array($data)) { return $data; } $sorted = []; foreach ($this->priorities as $key) { - if (\array_key_exists($key, $data)) { + if (array_key_exists($key, $data)) { $sorted[$key] = $data[$key]; unset($data[$key]); } } - // Hänge alle restlichen Properties hinten dran + // Append all remaining properties at the end return array_merge($sorted, $data); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
config/services.yaml(1 hunks)public/js/exportDocument.js(1 hunks)src/Controller/Admin/Base/AbstractImportBaseController.php(5 hunks)src/Serializer/Normalizer/PrioritizedAttributesNormalizer.php(1 hunks)src/Serializer/YamlSerializer.php(1 hunks)translations/admin.de.yaml(1 hunks)translations/admin.en.yaml(1 hunks)
🧠 Learnings (3)
📓 Common learnings
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Serializer/Normalizer/PrioritizedAttributesNormalizer.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/Base/AbstractImportBaseController.php (1)
Learnt from: jan888adams
PR: #10
File: src/Controller/Admin/PageImportController.php:23-23
Timestamp: 2025-04-01T09:57:30.185Z
Learning: We should use PHP enums instead of integer constants with message mapping arrays where possible, as they provide better type safety, code completion, and self-documentation.
🪛 GitHub Actions: Quality Assurance
src/Serializer/Normalizer/PrioritizedAttributesNormalizer.php
[error] 9-30: PHP CS Fixer formatting issues detected: incorrect PHPDoc tags and alignment. Run 'php-cs-fixer fix' to automatically fix these style issues.
🚧 Files skipped from review as they are similar to previous changes (2)
- public/js/exportDocument.js
- translations/admin.en.yaml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Serializer/Normalizer/PrioritizedAttributesNormalizer.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/Base/AbstractImportBaseController.php (1)
Learnt from: jan888adams
PR: #10
File: src/Controller/Admin/PageImportController.php:23-23
Timestamp: 2025-04-01T09:57:30.185Z
Learning: We should use PHP enums instead of integer constants with message mapping arrays where possible, as they provide better type safety, code completion, and self-documentation.
🪛 GitHub Actions: Quality Assurance
src/Serializer/Normalizer/PrioritizedAttributesNormalizer.php
[error] 9-30: PHP CS Fixer formatting issues detected: incorrect PHPDoc tags and alignment. Run 'php-cs-fixer fix' to automatically fix these style issues.
🔇 Additional comments (6)
src/Serializer/YamlSerializer.php (1)
26-28: LGTM! Attributes prioritization properly integrated.The addition of the
attributescontext key correctly implements prioritized attribute ordering during YAML serialization. The attribute list matches the configuration inconfig/services.yamland integrates well with the newPrioritizedAttributesNormalizer.config/services.yaml (1)
124-139: Service configuration is well-structured.The
PrioritizedAttributesNormalizerservice is properly configured with comprehensive attribute priorities and correctly tagged as a serializer normalizer. The priority list covers all essential attributes and aligns with the serialization context inYamlSerializer.src/Serializer/Normalizer/PrioritizedAttributesNormalizer.php (1)
9-30: Ensure formatting, documentation, and return-type consistency in PrioritizedAttributesNormalizer
- Translate the German inline comment in the constructor (around line 16):
- $this->priorities = $priorities; // z.B.: ['type', 'id', 'parentId', 'path', ...] + $this->priorities = $priorities; // e.g.: ['type', 'id', 'parentId', 'path', ...]- Add an explicit return type to
normalize()(in signature and PHPDoc), e.g.:/** * @return array<string, mixed> */ public function normalize(object $object, ?string $format, array $context): array- Run the project’s coding-standard tool (PHP CS Fixer or equivalent) to verify there are no remaining PHPDoc or alignment issues.
src/Controller/Admin/Base/AbstractImportBaseController.php (3)
23-23: Well-implemented failure handling for data inconsistencies.The addition of
FAILURE_INCONSISTENCYconstant and corresponding message mapping provides clear handling of import conflicts when IDs don't match. This aligns well with the PR objectives of supporting imports both with and without IDs.Also applies to: 33-33, 53-53
98-112: Excellent ID consistency validation logic.The defensive programming approach here is excellent:
- Proper null check for
$element->getId()handles imports without IDs- ID comparison prevents data corruption when IDs mismatch
- Detailed error logging helps with troubleshooting
- Generic error message using
$this->elementTypeworks for all element typesThis implementation correctly addresses the core challenge of the PR: supporting imports with and without IDs while maintaining data integrity.
124-135: HTML table output improves result presentation.The change from plain text to HTML table format provides better structured output for import results, making it easier to understand the import statistics.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/js/exportAsset.js(1 hunks)public/js/exportDataObjects.js(1 hunks)public/js/exportDocument.js(1 hunks)
🧠 Learnings (1)
📓 Common learnings
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
🧬 Code Graph Analysis (1)
public/js/exportAsset.js (2)
public/js/exportDataObjects.js (5)
includeIds(24-24)win(26-79)defaultFilename(23-23)form(59-59)values(61-61)public/js/exportDocument.js (4)
win(25-78)defaultFilename(23-23)form(58-58)values(60-60)
🚧 Files skipped from review as they are similar to previous changes (2)
- public/js/exportDocument.js
- public/js/exportDataObjects.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
🧬 Code Graph Analysis (1)
public/js/exportAsset.js (2)
public/js/exportDataObjects.js (5)
includeIds(24-24)win(26-79)defaultFilename(23-23)form(59-59)values(61-61)public/js/exportDocument.js (4)
win(25-78)defaultFilename(23-23)form(58-58)values(60-60)
🔇 Additional comments (1)
public/js/exportAsset.js (1)
26-81: LGTM! Consistent implementation with other export modules.The modal dialog implementation follows the same pattern established in
exportDocument.jsandexportDataObjects.js. The form includes proper validation, clear UI elements, and correctly passes theids_includedparameter to the backend route.
mike4git
force-pushed
the
feature/include-ids-optionally
branch
from
b1d7037 to
18a0ace
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/Controller/Admin/ImportDocumentsController.php (1)
43-52: Consider adding explicit forced save and error logging for consistency.Unlike
ImportDataObjectsControllerandImportAssetsController, this controller doesn't:
- Pass
trueas the third parameter to$this->importer->import()- Log exceptions with
$this->applicationLogger->error()This creates inconsistency across the import controllers.
Apply this diff to align with other controllers:
- return $this->importer->import($content, $format); + return $this->importer->import($content, $format, true); } catch (\Exception $e) { + $this->applicationLogger->error($e->getMessage()); throw new \Exception('Error reading uploaded file: ' . $e->getMessage(), 0, $e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Controller/Admin/Base/AbstractImportBaseController.php(7 hunks)src/Controller/Admin/ImportAssetsController.php(4 hunks)src/Controller/Admin/ImportDataObjectsController.php(3 hunks)src/Controller/Admin/ImportDocumentsController.php(2 hunks)src/Import/Importer.php(1 hunks)
🧠 Learnings (5)
📓 Common learnings
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/ImportAssetsController.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/ImportDataObjectsController.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/ImportDocumentsController.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/Base/AbstractImportBaseController.php (2)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
Learnt from: jan888adams
PR: #10
File: src/Controller/Admin/PageImportController.php:23-23
Timestamp: 2025-04-01T09:57:30.185Z
Learning: We should use PHP enums instead of integer constants with message mapping arrays where possible, as they provide better type safety, code completion, and self-documentation.
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/ImportAssetsController.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/ImportDataObjectsController.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/ImportDocumentsController.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/Base/AbstractImportBaseController.php (2)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
Learnt from: jan888adams
PR: #10
File: src/Controller/Admin/PageImportController.php:23-23
Timestamp: 2025-04-01T09:57:30.185Z
Learning: We should use PHP enums instead of integer constants with message mapping arrays where possible, as they provide better type safety, code completion, and self-documentation.
🔇 Additional comments (17)
src/Import/Importer.php (1)
37-37: LGTM! Good refactoring to make forced save behavior explicit.Changing the default from
truetofalsemakes the import behavior more explicit and controllable. The import controllers are properly updated to passtrueexplicitly when forced save is needed, maintaining the expected behavior while improving code clarity.src/Controller/Admin/ImportDocumentsController.php (2)
7-7: LGTM! Consistent dependency injection updates.The imports correctly reflect the new dependencies being used across import controllers.
Also applies to: 9-9
24-31: LGTM! Constructor properly updated with new dependencies.The constructor correctly switches to
ApplicationLoggerand addsParentRelationResolver, maintaining consistency with other import controllers.src/Controller/Admin/ImportDataObjectsController.php (3)
7-7: LGTM! Consistent dependency injection updates.The imports correctly reflect the new dependencies being used across import controllers.
Also applies to: 9-9
24-31: LGTM! Constructor properly updated with new dependencies.The constructor correctly switches to
ApplicationLoggerand addsParentRelationResolver, maintaining consistency with other import controllers.
48-52: LGTM! Proper implementation of forced save and error logging.This correctly implements the updated pattern:
- Explicit forced save with
trueparameter to maintain expected behavior- Proper error logging before rethrowing exceptions
This serves as a good reference for consistency across import controllers.
src/Controller/Admin/ImportAssetsController.php (4)
7-7: LGTM! Consistent dependency injection updates.The imports correctly reflect the new dependencies being used across import controllers.
Also applies to: 10-10
25-33: LGTM! Constructor properly updated with new dependencies.The constructor correctly switches to
ApplicationLoggerand addsParentRelationResolver, maintaining consistency with other import controllers.
52-52: LGTM! Proper forced save for ZIP imports.The ZIP import path correctly uses forced save to maintain expected behavior.
72-73: LGTM! Proper error logging before rethrowing.The error logging follows the consistent pattern used across import controllers.
src/Controller/Admin/Base/AbstractImportBaseController.php (7)
24-24: LGTM! Well-designed failure handling for data inconsistencies.The new
FAILURE_INCONSISTENCYconstant and its integration into the statistics tracking properly addresses the PR objective of handling ID conflicts during import operations.Also applies to: 34-34, 55-55
45-49: LGTM! Constructor properly updated with enhanced dependencies.The switch to
ApplicationLoggerand addition ofParentRelationResolversupports the improved logging and parent relation management throughout the import process.
97-97: LGTM! Simplified path lookup.Removing the slash prepending simplifies the path lookup logic appropriately.
100-117: Excellent consistency check implementation!The ID consistency logic correctly addresses the PR objectives:
- Null/zero ID handling: Properly allows imports without IDs (line 100)
- Matching ID validation: Prevents conflicts when IDs match (line 100)
- Inconsistency detection: Flags data inconsistencies when same key/path have different IDs (lines 109-115)
- Proper error messaging: Generic element-type message (addresses past review feedback)
The parent relation resolution and structured logging provide excellent traceability.
120-120: LGTM! Consistent logging for all import outcomes.The structured logging with
writeApplicationLogprovides excellent visibility into import actions for both skipped and new elements, supporting better debugging and audit trails.Also applies to: 125-129
135-144: LGTM! Improved result formatting with HTML table.The HTML table format provides much better readability compared to plain text, making it easier for users to understand import results.
160-175: LGTM! Well-structured application logging.The
writeApplicationLogmethod provides excellent structured logging with:
- Clear action prefixes ([REPLACE], [SKIP], [NEW])
- Comprehensive element details (type, key, path, ID)
- Related object context for replacements
- Proper component identification
This supports excellent debugging and audit capabilities.
… PimcoreApplicationLoggerBundle
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
public/js/exportAsset.js (1)
25-80: Well-implemented modal dialog with minor suggestions for improvement.The modal dialog implementation is well-structured and follows ExtJS best practices. The form validation, event handling, and internationalization are properly implemented.
A few minor suggestions:
- Consider using template strings for the checkbox label HTML construction to improve readability
- The double negation
!!values.includeIdsis correct but could be more explicit withBoolean(values.includeIds)Consider this minor refactor for improved readability:
- boxLabel: t('neusta_pimcore_import_export_exclude_ids_label') + - ' <span class="pimcore_object_label_icon pimcore_icon_gray_info" style="cursor: help;" data-qtip="' + - t('neusta_pimcore_import_export_exclude_ids_info') + '"></span>', + boxLabel: `${t('neusta_pimcore_import_export_exclude_ids_label')} <span class="pimcore_object_label_icon pimcore_icon_gray_info" style="cursor: help;" data-qtip="${t('neusta_pimcore_import_export_exclude_ids_info')}"></span>`,And for more explicit boolean conversion:
- ids_included: !!values.includeIds + ids_included: Boolean(values.includeIds)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
public/js/exportAsset.js(1 hunks)src/Command/Base/AbstractExportBaseCommand.php(1 hunks)src/Command/ExportAssetsCommand.php(1 hunks)src/Command/ExportDataObjectsCommand.php(1 hunks)src/Command/ExportDocumentsCommand.php(1 hunks)tests/app/TestKernel.php(1 hunks)tests/app/config/services.yaml(0 hunks)
🧠 Learnings (1)
📓 Common learnings
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
💤 Files with no reviewable changes (1)
- tests/app/config/services.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Command/Base/AbstractExportBaseCommand.php
- src/Command/ExportDataObjectsCommand.php
- src/Command/ExportDocumentsCommand.php
- src/Command/ExportAssetsCommand.php
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
🔇 Additional comments (2)
tests/app/TestKernel.php (2)
5-5: LGTM: Import statement correctly added.The import for
PimcoreApplicationLoggerBundleis properly added to support the logging infrastructure used by the import/export functionality.
12-12: LGTM: Bundle registration aligns with dependency requirements.The
PimcoreApplicationLoggerBundleis correctly registered before theNeustaPimcoreImportExportBundle, ensuring the logging infrastructure is available for the import/export controllers and services that now depend on theApplicationLogger.
mike4git
force-pushed
the
feature/include-ids-optionally
branch
from
e80df33 to
0907c93
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
src/Import/Strategy/ReplaceExistingElementStrategy.php (1)
3-3: Consider namespace placement for generic strategy.The namespace
Pagesuggests this strategy is page-specific, but it operates onAbstractElementand appears to be a generic replacement strategy. Consider moving it to a more generic namespace likeNeusta\Pimcore\ImportExportBundle\Import\Strategy\.src/Import/Strategy/Page/UpdateExistingPageStrategy.php (1)
30-34: Consider making version note configurable.The hardcoded version note 'overwritten by pimcore-import-export-bundle' could be made configurable to provide more context about the specific import operation.
Consider injecting a configurable version note or building it dynamically:
$versionNote = sprintf('Updated via import (strategy: %s)', static::class); $oldElement->save(['versionNote' => $versionNote]);src/Import/Event/ImportEvent.php (1)
12-19: Consider making readonly properties private.The class design is excellent with proper typing and readonly properties. However, since this appears to be a final data structure without inheritance intentions, consider making the properties
private readonlyinstead ofprotected readonlyfor better encapsulation.public function __construct( - protected readonly ImportStatus $status, - protected readonly string $type, - protected readonly array $yamlContent, - protected readonly ?AbstractElement $newElement, - protected readonly ?AbstractElement $oldElement, + private readonly ImportStatus $status, + private readonly string $type, + private readonly array $yamlContent, + private readonly ?AbstractElement $newElement, + private readonly ?AbstractElement $oldElement, ) {src/Import/Importer.php (1)
49-49: Update method signature documentation.The new
$overwriteparameter should be documented in the docblock to clarify its purpose and behavior./** * @return array<TTarget> * + * @param bool $overwrite Whether to overwrite existing elements with matching paths * @throws InconsistencyException * @throws ConverterException * @throws DuplicateFullPathException * @throws \DomainException * @throws \InvalidArgumentException */ public function import(string $yamlInput, string $format, bool $forcedSave, bool $overwrite): arraysrc/Controller/Admin/Base/AbstractImportBaseController.php (1)
70-70: Remove unused variable.The
$elementsvariable is assigned but never used, as correctly identified by static analysis.- $elements = $this->importByFile($file, $format, true, $this->overwrite); + $this->importByFile($file, $format, true, $this->overwrite);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
config/pimcore/import/documents/converters_populators.yaml(2 hunks)config/services.yaml(3 hunks)src/Command/ImportAssetsCommand.php(1 hunks)src/Command/ImportDataObjectsCommand.php(1 hunks)src/Command/ImportDocumentsCommand.php(1 hunks)src/Controller/Admin/Base/AbstractImportBaseController.php(7 hunks)src/Controller/Admin/ImportAssetsController.php(4 hunks)src/Controller/Admin/ImportDataObjectsController.php(3 hunks)src/Controller/Admin/ImportDocumentsController.php(3 hunks)src/DependencyInjection/CompilerPass/RegisterTaggedConverterPass.php(1 hunks)src/Exception/InconsistencyException.php(1 hunks)src/Import/Event/ImportEvent.php(1 hunks)src/Import/Event/ImportStatus.php(1 hunks)src/Import/EventSubscriber/ImportLoggingEventSubscriber.php(1 hunks)src/Import/Importer.php(3 hunks)src/Import/Strategy/MergeElementStrategy.php(1 hunks)src/Import/Strategy/Page/UpdateExistingPageStrategy.php(1 hunks)src/Import/Strategy/ReplaceExistingElementStrategy.php(1 hunks)src/NeustaPimcoreImportExportBundle.php(2 hunks)tests/Integration/Import/ImporterTest.php(8 hunks)tests/app/config/services.yaml(2 hunks)
🧠 Learnings (10)
📓 Common learnings
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/DependencyInjection/CompilerPass/RegisterTaggedConverterPass.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Import/Event/ImportEvent.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
config/pimcore/import/documents/converters_populators.yaml (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Import/EventSubscriber/ImportLoggingEventSubscriber.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/ImportDataObjectsController.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Import/Importer.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
tests/app/config/services.yaml (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
config/services.yaml (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/Base/AbstractImportBaseController.php (2)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
Learnt from: jan888adams
PR: #10
File: src/Controller/Admin/PageImportController.php:23-23
Timestamp: 2025-04-01T09:57:30.185Z
Learning: We should use PHP enums instead of integer constants with message mapping arrays where possible, as they provide better type safety, code completion, and self-documentation.
🪛 GitHub Check: Quality Checks
src/Import/Importer.php
[failure] 101-101:
Method Neusta\Pimcore\ImportExportBundle\Import\Importer::import() should return array<TTarget of Pimcore\Model\Element\AbstractElement> but returns array<int<0, max>, Pimcore\Model\Element\AbstractElement>.
🪛 PHPMD (2.15.0)
src/Controller/Admin/Base/AbstractImportBaseController.php
70-70: Avoid unused local variables such as '$elements'. (Unused Code Rules)
(UnusedLocalVariable)
✅ Files skipped from review due to trivial changes (3)
- src/Exception/InconsistencyException.php
- src/Import/Event/ImportStatus.php
- src/Import/Strategy/MergeElementStrategy.php
🚧 Files skipped from review as they are similar to previous changes (4)
- src/NeustaPimcoreImportExportBundle.php
- tests/Integration/Import/ImporterTest.php
- src/Controller/Admin/ImportAssetsController.php
- src/Controller/Admin/ImportDocumentsController.php
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/DependencyInjection/CompilerPass/RegisterTaggedConverterPass.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Import/Event/ImportEvent.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
config/pimcore/import/documents/converters_populators.yaml (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Import/EventSubscriber/ImportLoggingEventSubscriber.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/ImportDataObjectsController.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Import/Importer.php (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
tests/app/config/services.yaml (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
config/services.yaml (1)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
src/Controller/Admin/Base/AbstractImportBaseController.php (2)
Learnt from: mike4git
PR: #13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
Learnt from: jan888adams
PR: #10
File: src/Controller/Admin/PageImportController.php:23-23
Timestamp: 2025-04-01T09:57:30.185Z
Learning: We should use PHP enums instead of integer constants with message mapping arrays where possible, as they provide better type safety, code completion, and self-documentation.
🪛 GitHub Check: Quality Checks
src/Import/Importer.php
[failure] 101-101:
Method Neusta\Pimcore\ImportExportBundle\Import\Importer::import() should return array<TTarget of Pimcore\Model\Element\AbstractElement> but returns array<int<0, max>, Pimcore\Model\Element\AbstractElement>.
🪛 PHPMD (2.15.0)
src/Controller/Admin/Base/AbstractImportBaseController.php
70-70: Avoid unused local variables such as '$elements'. (Unused Code Rules)
(UnusedLocalVariable)
🔇 Additional comments (31)
src/Command/ImportAssetsCommand.php (1)
70-70: LGTM! Parameter addition aligns with configurable import behavior.The addition of the
overwriteparameter to the importer method call correctly implements the configurable import functionality described in the PR objectives.src/Command/ImportDocumentsCommand.php (1)
63-63: LGTM! Consistent with other import commands.The addition of the
overwriteparameter follows the same pattern as other import commands and correctly implements the configurable import behavior.src/Import/Strategy/Page/UpdateExistingPageStrategy.php (2)
23-35: LGTM! Comprehensive property merging logic.The merge strategy correctly handles the various properties and editables synchronization between page elements, with appropriate type checking for Page-specific properties like title.
21-22: Add runtime type validation for type safety.The method parameters are typed as
AbstractElementbut the implementation expectsDocument\PageSnippetinstances. Based on the retrieved learning that Populators are used exclusively within Converters ensuring type safety, add runtime validation here for defensive programming.public function mergeAndSave(AbstractElement $oldElement, AbstractElement $newElement): void { + if (!$oldElement instanceof Document\PageSnippet || !$newElement instanceof Document\PageSnippet) { + throw new \InvalidArgumentException('Both elements must be PageSnippet instances'); + } + $oldElement->setPublished($newElement->getPublished());⛔ Skipped due to learnings
Learnt from: mike4git PR: teamneusta/pimcore-import-export-bundle#13 File: src/Populator/PageImportPopulator.php:32-34 Timestamp: 2025-04-04T07:19:54.077Z Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.src/Controller/Admin/ImportDataObjectsController.php (4)
7-7: LGTM: Import statements updated appropriately.The new imports for
ParentRelationResolverandApplicationLoggerare consistent with the constructor changes and align with the enhanced import functionality.Also applies to: 9-9
24-31: LGTM: Constructor properly updated for enhanced import functionality.The constructor changes appropriately:
- Switches to
ApplicationLoggerfor better Pimcore integration- Adds
ParentRelationResolverfor handling parent-child relationships- Correctly passes all parameters to the parent constructor with the proper type identifier
43-43: LGTM: Method signature enhanced with configurable parameters.The addition of
$forcedSaveand$overwriteparameters with sensible defaults provides better control over import behavior while maintaining backward compatibility.
48-48: LGTM: Implementation properly utilizes new parameters and logging.The changes correctly:
- Pass the new
$forcedSaveand$overwriteparameters to the importer- Use the
ApplicationLoggerfor consistent error logging within the Pimcore ecosystemAlso applies to: 50-50
config/pimcore/import/documents/converters_populators.yaml (1)
9-17: LGTM: New converter configurations properly extend document import capabilities.The new converters follow the established pattern:
import_documentprovides a generic document converter with core populatorsimport_page_snippetextends support to PageSnippet documents with appropriate populators including thePageImportPopulatorBoth configurations are consistent with the existing
import_pageconverter structure.Also applies to: 32-42
src/Import/Event/ImportEvent.php (1)
21-47: LGTM: Getter methods are properly implemented.All getter methods follow standard conventions with correct return types and provide appropriate access to the readonly properties.
src/Import/EventSubscriber/ImportLoggingEventSubscriber.php (1)
56-63: LGTM: Comprehensive error message formatting.The error message provides excellent context for debugging data inconsistencies, including type, key, path, and both old and new IDs. The sprintf formatting is appropriate for the structured message.
src/Import/Importer.php (4)
7-10: LGTM! Good architectural additions.The new imports support the enhanced import functionality with event dispatching, exception handling, and merge strategies. These align well with the PR objectives for conditional ID handling.
26-38: Excellent refactoring to service locators.The transition from a static array-based type-to-converter mapping to tagged service locators is a significant architectural improvement. This provides better type safety, flexibility, and follows dependency injection best practices.
67-94: Well-implemented consistency checks and event dispatching.The logic correctly handles the different import scenarios:
- Creates new elements when none exist
- Performs consistency checks for ID conflicts
- Merges elements when appropriate
- Dispatches appropriate events for observability
The ID consistency logic in lines 82 and 87 properly prevents accidental overwrites when IDs don't match, which aligns perfectly with the PR objectives.
104-112: Good helper methods for ID validation.These helper methods provide clear, reusable logic for ID consistency checks. The naming is descriptive and the logic is correct for identifying valid IDs and matching IDs between elements.
config/services.yaml (5)
76-78: Excellent service locator configuration.The refactoring from static array mapping to tagged service locators is well-implemented. This provides better flexibility and type safety for the import process.
99-108: Well-configured merge strategies.The merge strategy services are properly tagged for their respective Pimcore element types. The
UpdateExistingPageStrategyfor document types andReplaceElementStrategyfor assets and concrete objects makes logical sense for different element behaviors.
117-132: Good repository service configuration.Making the repository services public and properly tagging them supports the service locator pattern effectively. The multiple tags for document-related types (Document, Page, PageSnippet) provide appropriate coverage.
150-166: Nice addition of prioritized export ordering.The
PrioritizedAttributesNormalizerwith the specified attribute priorities (including the new 'filename' attribute) will provide consistent and logical ordering in export output, improving readability and predictability.
175-175: Good addition of import event subscriber.The
ImportLoggingEventSubscriberservice will provide valuable logging and observability for the import process, complementing the event dispatching added to the Importer class.tests/app/config/services.yaml (4)
1-16: Good test asset storage configuration.The Flysystem storage configuration for test assets provides a proper isolated testing environment with appropriate permissions.
18-19: Smart configuration import.Importing the main services.yaml file ensures consistency between test and production configurations while allowing test-specific overrides.
64-66: Consistent service locator configuration.The Importer service configuration matches the main configuration file, ensuring test consistency.
85-97: Proper repository service configuration.The repository services are correctly configured with appropriate tags for their respective Pimcore model types, matching the main configuration.
src/Controller/Admin/Base/AbstractImportBaseController.php (7)
24-35: Good addition of inconsistency handling.The new
FAILURE_INCONSISTENCYconstant and corresponding message provide clear feedback when ID conflicts are detected during import, supporting the PR's objective of handling ID-based imports safely.
46-48: Appropriate dependency updates.The constructor changes to use
ApplicationLoggerandParentRelationResolveralign with the broader architectural improvements and provide better structured logging and relationship handling.
97-120: Excellent ID consistency implementation.The ID consistency checks are well-implemented and align perfectly with the PR objectives:
- Allows import when new element has no valid ID (null or 0)
- Allows import when IDs match between old and new elements
- Prevents import with clear error message when IDs conflict
- The child reassignment logic properly handles hierarchical relationships
The error message is now appropriately generic (addressing the previous review comment).
156-156: Good method signature update.The updated
importByFilemethod signature withforcedSaveandoverwriteparameters supports the enhanced import functionality and provides clear control over import behavior.
163-178: Excellent structured logging addition.The
writeApplicationLogmethod provides consistent, structured logging with relevant context (type, key, path, ID) and optional related object information. This greatly improves debugging and audit capabilities.
138-147: Nice improvement to result formatting.The HTML table format for result messages provides much better readability compared to plain text, making it easier for users to understand import outcomes.
98-104: Ensure child reassignment is covered by testsI didn’t find any existing test cases or merge‐strategy implementations that verify children are preserved when an element is overwritten. To fully address the original German comment’s concern:
- Add a unit or integration test around
src/Controller/Admin/Base/AbstractImportBaseController.php(lines 98–104) that:
- Creates an element with children
- Runs the import/overwrite path
- Asserts that the children’s parent was updated to the new element
- Asserts that no child records were deleted
- Optionally, perform a quick manual end-to-end import to confirm this behavior in your development environment.
mike4git
force-pushed
the
feature/include-ids-optionally
branch
from
0907c93 to
0d7cc8e
Compare
mike4git
force-pushed
the
feature/include-ids-optionally
branch
7 times, most recently
from
8544d8b to
052ad86
Compare
mike4git
force-pushed
the
feature/include-ids-optionally
branch
from
052ad86 to
fd5b1dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
tests/app/config/services.yaml (1)
52-55: Syntax error has been properly fixed.The missing
tags:key issue from the previous review has been correctly addressed. The service configuration now follows proper YAML syntax.src/Import/Importer.php (1)
116-117: Fix generic type annotation syntax.The type annotation should use proper PHPDoc syntax with double slash and asterisk.
- /* @var array<TTarget> $elements */ + /** @var array<TTarget> $elements */ return $elements; // @phpstan-ignore-line
🧹 Nitpick comments (3)
tests/Integration/Import/ImporterExporterTest.php (1)
15-52: Expand test coverage for comprehensive validation.The current test only covers the happy path. Consider adding tests for:
- Error conditions (invalid YAML, missing files)
- Edge cases (empty documents, special characters in keys)
- Different document types (Page vs PageSnippet)
- Import with and without IDs (as mentioned in PR objectives)
This would better validate the new ID inclusion/exclusion functionality mentioned in the PR summary.
src/Import/EventSubscriber/StatisticsEventSubscriber.php (1)
36-43: Simplify counter increment logic.The array key existence check is unnecessary as the increment operation can be simplified.
public function incrementCounter(ImportStatus $status): void { - if (\array_key_exists($status->value, self::$statistics)) { - ++self::$statistics[$status->value]; - } else { - self::$statistics[$status->value] = 1; - } + self::$statistics[$status->value] = (self::$statistics[$status->value] ?? 0) + 1; }This is more concise and achieves the same result.
src/Populator/PageImportPopulator.php (1)
32-59: Enhance error handling and validation.The populate method handles the core logic well but could benefit from additional safety measures.
Consider these improvements:
- Add null safety for repository calls:
foreach ($source['properties'] ?? [] as $property) { + if (!isset($property['key']) || !isset($property['type'])) { + $this->logger?->warning('Skipping property with missing key or type', ['property' => $property]); + continue; + } + if ($property['value'] && 'asset' === $property['type']) { - $value = $this->assetRepository->getByPath($property['value']); + $value = $this->assetRepository->getByPath($property['value']); + if ($value === null) { + $this->logger?->warning('Asset not found', ['path' => $property['value']]); + } }
- The current implementation looks solid - the repository resolution logic is appropriate and the PageSnippet type check follows the established pattern from the retrieved learnings.
The error handling for editables is well implemented with proper logging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
composer.json(1 hunks)config/pimcore/export/documents/converters_populators.yaml(4 hunks)config/pimcore/import/documents/converters_populators.yaml(2 hunks)config/services.yaml(3 hunks)public/js/exportAsset.js(1 hunks)public/js/exportDataObjects.js(1 hunks)public/js/exportDocument.js(1 hunks)src/Command/Base/AbstractExportBaseCommand.php(5 hunks)src/Command/Base/AbstractImportBaseCommand.php(1 hunks)src/Command/ExportAssetsCommand.php(2 hunks)src/Command/ExportDataObjectsCommand.php(2 hunks)src/Command/ExportDocumentsCommand.php(2 hunks)src/Command/ImportAssetsCommand.php(1 hunks)src/Command/ImportDataObjectsCommand.php(3 hunks)src/Command/ImportDocumentsCommand.php(1 hunks)src/Controller/Admin/Base/AbstractImportBaseController.php(6 hunks)src/Controller/Admin/ExportAssetsController.php(5 hunks)src/Controller/Admin/ExportDataObjectsController.php(5 hunks)src/Controller/Admin/ExportDocumentsController.php(5 hunks)src/Controller/Admin/ImportAssetsController.php(4 hunks)src/Controller/Admin/ImportDataObjectsController.php(3 hunks)src/Controller/Admin/ImportDocumentsController.php(3 hunks)src/DependencyInjection/CompilerPass/RegisterTaggedConverterPass.php(1 hunks)src/Exception/InconsistencyException.php(1 hunks)src/Export/Exporter.php(1 hunks)src/Import/Event/ImportEvent.php(1 hunks)src/Import/Event/ImportStatus.php(1 hunks)src/Import/EventSubscriber/ImportLoggingEventSubscriber.php(1 hunks)src/Import/EventSubscriber/StatisticsEventSubscriber.php(1 hunks)src/Import/Importer.php(3 hunks)src/Import/Strategy/Document/UpdateExistingPageStrategy.php(1 hunks)src/Import/Strategy/MergeElementStrategy.php(1 hunks)src/Import/Strategy/ReplaceExistingElementStrategy.php(1 hunks)src/Model/Document/Document.php(1 hunks)src/Model/Document/Folder.php(1 hunks)src/Model/Document/Page.php(1 hunks)src/Model/Document/PageSnippet.php(1 hunks)src/Model/Element.php(1 hunks)src/Model/Property.php(1 hunks)src/NeustaPimcoreImportExportBundle.php(2 hunks)src/Populator/PageImportPopulator.php(3 hunks)src/Populator/PropertyValuePopulator.php(1 hunks)src/Serializer/Normalizer/PrioritizedAttributesNormalizer.php(1 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_simple_saved_pages_export__1.yaml(1 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_simple_unsaved_pages_export__1.yaml(1 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_single_image_export__1.yaml(1 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_single_image_export_with_ids__1.yaml(1 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_single_page_export__1.yaml(1 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_single_page_export_json__1.json(1 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_single_page_export_with_ids__1.yaml(1 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_tree_pages_export__1.yaml(1 hunks)tests/Integration/Export/__snapshots__/ExporterTest__test_tree_pages_export_with_ids__1.yaml(1 hunks)tests/Integration/Import/ImporterExporterTest.php(1 hunks)tests/Integration/Import/ImporterTest.php(8 hunks)tests/Integration/data/Text Editor.yaml(1 hunks)tests/Unit/Controller/Admin/ExportDocumentsControllerTest.php(2 hunks)tests/app/config/services.yaml(2 hunks)translations/admin.de.yaml(1 hunks)translations/admin.en.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- src/Model/Document/Page.php
- src/Model/Property.php
- composer.json
- src/Model/Document/Folder.php
- src/Model/Document/PageSnippet.php
- tests/Integration/data/Text Editor.yaml
🚧 Files skipped from review as they are similar to previous changes (36)
- src/Controller/Admin/ExportDocumentsController.php
- src/Import/Strategy/MergeElementStrategy.php
- tests/Integration/Export/snapshots/ExporterTest__test_single_page_export__1.yaml
- src/Controller/Admin/ImportDocumentsController.php
- tests/Integration/Export/snapshots/ExporterTest__test_single_page_export_with_ids__1.yaml
- public/js/exportDataObjects.js
- src/Command/ImportDocumentsCommand.php
- src/Import/Event/ImportEvent.php
- translations/admin.en.yaml
- src/Command/ExportDocumentsCommand.php
- public/js/exportAsset.js
- translations/admin.de.yaml
- tests/Integration/Export/snapshots/ExporterTest__test_simple_unsaved_pages_export__1.yaml
- tests/Integration/Export/snapshots/ExporterTest__test_single_page_export_json__1.json
- src/Controller/Admin/ExportDataObjectsController.php
- config/pimcore/export/documents/converters_populators.yaml
- config/services.yaml
- tests/Integration/Export/snapshots/ExporterTest__test_single_image_export_with_ids__1.yaml
- src/Command/ImportAssetsCommand.php
- config/pimcore/import/documents/converters_populators.yaml
- src/Command/Base/AbstractExportBaseCommand.php
- tests/Integration/Export/snapshots/ExporterTest__test_tree_pages_export__1.yaml
- src/Command/ImportDataObjectsCommand.php
- src/Controller/Admin/ImportAssetsController.php
- src/Controller/Admin/Base/AbstractImportBaseController.php
- src/DependencyInjection/CompilerPass/RegisterTaggedConverterPass.php
- src/Serializer/Normalizer/PrioritizedAttributesNormalizer.php
- src/Controller/Admin/ImportDataObjectsController.php
- src/Command/ExportAssetsCommand.php
- tests/Unit/Controller/Admin/ExportDocumentsControllerTest.php
- tests/Integration/Import/ImporterTest.php
- tests/Integration/Export/snapshots/ExporterTest__test_tree_pages_export_with_ids__1.yaml
- tests/Integration/Export/snapshots/ExporterTest__test_single_image_export__1.yaml
- src/NeustaPimcoreImportExportBundle.php
- src/Command/ExportDataObjectsCommand.php
- src/Controller/Admin/ExportAssetsController.php
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
📚 Learning: we should use php enums instead of integer constants with message mapping arrays where possible, as ...
Learnt from: jan888adams
PR: teamneusta/pimcore-import-export-bundle#10
File: src/Controller/Admin/PageImportController.php:23-23
Timestamp: 2025-04-01T09:57:30.185Z
Learning: We should use PHP enums instead of integer constants with message mapping arrays where possible, as they provide better type safety, code completion, and self-documentation.
Applied to files:
src/Import/Event/ImportStatus.php
📚 Learning: in the neusta\pimcore\importexportbundle, populators are designed to be used exclusively within conv...
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
Applied to files:
src/Import/Event/ImportStatus.phptests/app/config/services.yamlsrc/Import/Importer.phpsrc/Exception/InconsistencyException.phpsrc/Import/EventSubscriber/ImportLoggingEventSubscriber.phpsrc/Populator/PropertyValuePopulator.phptests/Integration/Import/ImporterExporterTest.phpsrc/Populator/PageImportPopulator.phpsrc/Export/Exporter.php
🧬 Code Graph Analysis (5)
src/Model/Document/Document.php (2)
src/Model/Property.php (1)
Property(5-10)src/Model/Element.php (1)
Element(5-14)
src/Import/EventSubscriber/StatisticsEventSubscriber.php (1)
src/Import/Event/ImportEvent.php (2)
ImportEvent(7-54)getStatus(22-25)
src/Populator/PropertyValuePopulator.php (3)
src/Model/Property.php (1)
Property(5-10)src/Model/Element.php (1)
Element(5-14)src/Populator/PageImportPopulator.php (1)
populate(31-59)
src/Import/Strategy/Document/UpdateExistingPageStrategy.php (4)
src/Model/Document/Document.php (1)
Document(8-29)src/Import/Strategy/ReplaceExistingElementStrategy.php (1)
mergeAndSave(15-24)src/Model/Document/PageSnippet.php (1)
PageSnippet(5-10)src/Model/Document/Page.php (1)
Page(5-8)
src/Export/Exporter.php (5)
src/Import/Event/ImportEvent.php (2)
ImportEvent(7-54)__construct(12-20)src/Model/Element.php (1)
Element(5-14)src/Import/Importer.php (1)
__construct(30-38)src/Converter/ExtendedConverter.php (1)
convert(35-44)src/Converter/TypeStrategyConverter.php (1)
convert(25-33)
🪛 PHPMD (2.15.0)
src/Populator/PropertyValuePopulator.php
15-15: Avoid unused parameters such as '$ctx'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (23)
tests/app/config/services.yaml (2)
35-37: LGTM! Service locator refactor improves flexibility.The change from static type-to-service maps to tagged service locators allows for more dynamic service discovery and better separation of concerns. This aligns with the broader architectural improvements in the PR.
43-70: Service tagging configuration is well-structured.The merge strategy and repository services are properly tagged with appropriate type mappings, enabling the tagged service locators to resolve services based on Pimcore model types. The multi-tag approach for documents (base Document, Page, PageSnippet) ensures comprehensive coverage.
src/Exception/InconsistencyException.php (1)
5-7: LGTM! Exception class follows previous feedback correctly.The class properly extends
LogicExceptionas suggested in previous reviews. This is appropriate since inconsistency exceptions represent logical errors in the import process rather than runtime errors. The minimal implementation is sufficient for its purpose.src/Import/Event/ImportStatus.php (1)
5-12: Excellent use of PHP enum following best practices.This implementation aligns perfectly with the retrieved learning about preferring PHP enums over integer constants. The string-backed enum provides better type safety, code completion, and self-documentation. The status cases comprehensively cover import outcomes and the consistent naming (case name matches string value) enhances readability.
public/js/exportDocument.js (1)
25-80: Excellent UI refactor improving user experience.The replacement of the simple prompt with a proper ExtJS modal dialog significantly enhances the user experience. The implementation includes:
- Required filename field with sensible default
- Checkbox for ID inclusion/exclusion with helpful tooltip
- Proper form validation before submission
- Clean ExtJS patterns with appropriate event handling
The integration with the backend via the
include_idsparameter supports the PR's core objective of configurable ID handling during export.src/Command/Base/AbstractImportBaseCommand.php (1)
40-46: LGTM! Overwrite option aligns with PR objectives.The new
overwriteoption provides the configurable import behavior described in the PR objectives. The implementation is well-designed:
- Uses
VALUE_NONEappropriately for a boolean flag- Defaults to
falsefor safety (non-destructive by default)- Clear description explaining key-path-id matching behavior
- Integrates with the enhanced import framework
This supports the PR's goal of allowing users to choose whether to overwrite existing elements during import operations.
src/Model/Element.php (1)
9-10: Good approach to prevent automatic null value exports.The removal of default values from nullable
$idand$parentIdproperties is well-reasoned and aligns with the PR objective of conditional ID inclusion. The comments clearly explain the rationale for this change.tests/Integration/Export/__snapshots__/ExporterTest__test_simple_saved_pages_export__1.yaml (1)
1-22: Snapshot correctly reflects the new export structure.The updated snapshot properly shows:
- Removal of ID fields (
id,parentId) consistent with conditional ID export- Introduction of empty
propertiesarrays aligning with the new Property model- Clean separation of page elements with core attributes only
This validates that the export refactoring is working as expected.
src/Model/Document/Document.php (2)
6-6: Good addition of Property import.The import statement for the Property class is correctly added to support the new properties structure.
12-13: Excellent refactoring to centralize document properties.The consolidation of individual properties (
navigation_name,navigation_title,title,controller,editables) into a typedpropertiesarray is a well-designed improvement that:
- Provides better type safety with
Propertyobjects- Centralizes property management
- Aligns with the new Property model structure
- Maintains flexibility for various property types
The PHPDoc annotation clearly documents the array type.
src/Populator/PropertyValuePopulator.php (2)
10-12: Well-documented generic interface implementation.The PHPDoc annotation correctly specifies the generic types for the Populator interface, making the intended usage clear.
15-22: Sound logic for property value population.The implementation correctly handles different data types:
- For
AbstractElementinstances: concatenates path and key to create element references- For other data: directly assigns the value
This approach aligns with Pimcore's property system where element references are stored as path strings.
Note: The unused
$ctxparameter is required by thePopulatorinterface and can be safely ignored in this context per the static analysis hint.src/Import/Strategy/ReplaceExistingElementStrategy.php (2)
7-9: Proper generic interface implementation.The PHPDoc annotation correctly specifies the generic type
AbstractElementfor theMergeElementStrategyinterface.
15-24: Excellent error handling for element replacement.The implementation properly addresses potential failure scenarios:
- Wraps both delete and save operations in try-catch
- Provides meaningful error context in the exception message
- Preserves the original exception as the previous exception
- Documents the
RuntimeExceptionin the method signatureThis prevents inconsistent state issues that could occur if the delete succeeds but save fails.
src/Import/Strategy/Document/UpdateExistingPageStrategy.php (2)
10-13: LGTM!The class definition with proper generic type annotation and interface implementation looks correct.
25-32: LGTM!The type-specific handling for PageSnippet and Page instances is appropriate and follows the expected flow. Based on the retrieved learnings, runtime type validation is not necessary here as Converters ensure type safety.
src/Import/EventSubscriber/StatisticsEventSubscriber.php (1)
14-34: LGTM!The event subscription and handling logic is correctly implemented following Symfony EventSubscriber patterns.
src/Populator/PageImportPopulator.php (1)
18-24: LGTM!The constructor properly injects required repositories and optional logger, following good dependency injection practices.
src/Export/Exporter.php (3)
24-29: LGTM!The constructor properly adds the EventDispatcher dependency needed for the new event-driven architecture.
39-44: LGTM!The context parameter handling is well implemented. The GenericContext creation and population from the ctxParams array is clean and follows the expected pattern for passing export options like 'include-ids' to converters.
50-56: Excellent error handling implementation.The try-catch wrapper around converter calls with event dispatching for failures is well designed. It allows exports to continue even when individual elements fail, while properly tracking failures through the event system.
The approach of skipping failed elements rather than failing the entire export operation provides better user experience and aligns with the statistics tracking introduced in the PR.
src/Import/EventSubscriber/ImportLoggingEventSubscriber.php (2)
19-26: Event subscription properly configured.Good job fixing the previously commented-out event subscription. The subscriber now correctly listens to
ImportEventwith an appropriate priority.
44-64: Null safety properly implemented.Good implementation of null-safe operators with appropriate fallback values. This addresses the previous review feedback about potential null pointer exceptions.
Co-authored-by: Jacob Dreesen <jacob@hdreesen.de>
mike4git
force-pushed
the
feature/include-ids-optionally
branch
2 times, most recently
from
cebf4bd to
4ff4229
Compare
mike4git
force-pushed
the
feature/include-ids-optionally
branch
from
4ff4229 to
bdb2635
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (2)
80-81: Convert bare URL into Markdown link to satisfy MD034 and improve readabilitymarkdownlint flagged the plain URL (MD034). Wrap the link in Markdown syntax:
-https://docs.pimcore.com/platform/2024.4/Pimcore/Documents/Editables/WYSIWYG#extending-symfony-html-sanitizer-configuration +[Pimcore docs – Extending the Symfony HTML sanitizer configuration](https://docs.pimcore.com/platform/2024.4/Pimcore/Documents/Editables/WYSIWYG#extending-symfony-html-sanitizer-configuration)
85-87: Use plain‐text code fence or embed actual YAML for clarityThe block only shows the path, not YAML content, yet it’s annotated as
yaml. Either:
- Change the fence to plain
text(or omit the language tag) to avoid misleading syntax highlighting, or- Paste the relevant YAML snippet so the example is self-contained.
-```yaml -tests/app/config/packages/pimcore.yaml -``` +```text +tests/app/config/packages/pimcore.yaml +```composer.json (1)
37-37: Upgrade to PHPUnit 9.6 looks good, but consider roadmap to v10Bumping the dev-dependency to
^9.6keeps the test suite compatible with PHP 8.3 and avoids older bug-fix branches.
Long-term, PHPUnit 10 brings typed expectations and deprecations removal; worth adding to the technical debt list for a future major bump when you drop PHP 8.1.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)composer.json(1 hunks)tests/app/config/packages/pimcore.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/app/config/packages/pimcore.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mike4git
PR: teamneusta/pimcore-import-export-bundle#13
File: src/Populator/PageImportPopulator.php:32-34
Timestamp: 2025-04-04T07:19:54.077Z
Learning: In the Neusta\Pimcore\ImportExportBundle, Populators are designed to be used exclusively within Converters, which ensures type safety without needing additional runtime type checks in the Populator classes.
🪛 markdownlint-cli2 (0.17.2)
README.md
81-81: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (2)
composer.json (2)
38-38: Lowered minimum forpimcore/admin-ui-classic-bundle– double-check feature coverageChanging the constraint from
^1.6to^1.0now allows installing 1.0 – 1.5. If your code relies on functionality introduced ≥ 1.6, this could break consumers who runcomposer updateon a fresh project.- "pimcore/admin-ui-classic-bundle": "^1.0", + "pimcore/admin-ui-classic-bundle": "^1.6",Verify whether 1.0–1.5 satisfy all current usages; if not, restore the higher lower-bound or add explicit
>=1.6in the constraint.
42-44: 👍 Conflict rule prevents vulnerablemasterminds/html5versionsBlocking versions
< 2.8.1is a neat proactive move against CVE-2024-23653 and related issues.
3 participants
In some situations, you may want to export Assets, Documents, and DataObjects based solely on their values (without technical IDs).
Keep in mind - key+path is a domain-driven identifier in Pimcore. No element will have the same pair of it.
This simplifies the transfer of data into foreign systems where IDs may already be assigned. You won't overwrite existing elements which have accidentally the same ID.
However, in rare cases — such as backups or 1:1 migrations — it makes sense to export the IDs (id / parentId) and consider them during import.
For this reason, commands and controller actions can be configured accordingly.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores