Conversation
mike4git
force-pushed
the
feature/import-export-multiple-pages
branch
5 times, most recently
from
a3db095 to
0bffe45
Compare
Add Menu and Controller route for export page incl. subpages
mike4git
force-pushed
the
feature/import-export-multiple-pages
branch
2 times, most recently
from
0786cc8 to
a542252
Compare
mike4git
force-pushed
the
feature/import-export-multiple-pages
branch
from
a542252 to
f0f8f47
Compare
different converters for folder, snippet & pages
src/Documents/Export/Converter/DocumentTypeStrategyConverter.php
Outdated
Show resolved
Hide resolved
| $newPath = $existingParent->getPath() . $page->getKey() . '/'; | ||
| foreach ($configPages as $configPage) { | ||
| if (\array_key_exists('path', $configPage[YamlExportPage::PAGE]) && \is_string($oldPath)) { | ||
| str_replace($oldPath, $newPath, $configPage[YamlExportPage::PAGE]['path']); |
There was a problem hiding this comment.
Neben dem refactoring das ich unten vorgschlagen habe, muss zuätzlich dieser Teil hier noch gefixt werden.
Das pass by reference klappt so nicht. Siehe hier https://onlinephp.io/c/ce4a2
Ich habe überlegt einen Test dafür zu schreiben, aber die parseYaml methode tut zu viel. Kannst du vielleicht das yaml parsen und den Aufruf für checkAndUpdatePage kapseln? Vielleicht in zwei Klassen? Dann wäre es wesentlich einfacher testbar.
There was a problem hiding this comment.
Have you had a look on the PageImportTest::testSinglePageImport_tree_case ???
There was a problem hiding this comment.
For the first glance, i would prefer to not break backwards compatibility.
What about trigger deprecation using https://github.com/symfony/deprecation-contracts
function parseYaml() {
trigger_deprecation('teamneusta/pimcore-import-export-bundle', '1.2', 'Using "%s" is deprecated, use "%s" instead.', 'parseYaml', 'importYaml');
...
}
plus
function importYaml() {
$this->parseYaml();
}
Something like this. If you are willing to wait until Thuesday, let us discuss it then
Co-authored-by: Luka Dschaak <l.dschaak@neusta.de>
thx for translations Co-authored-by: Luka Dschaak <l.dschaak@neusta.de>
thx for translations Co-authored-by: Luka Dschaak <l.dschaak@neusta.de>
thx for translations Co-authored-by: Luka Dschaak <l.dschaak@neusta.de>
thx for translations Co-authored-by: Luka Dschaak <l.dschaak@neusta.de>
thx for translations Co-authored-by: Luka Dschaak <l.dschaak@neusta.de>
thx for translations Co-authored-by: Luka Dschaak <l.dschaak@neusta.de>
thx for translations Co-authored-by: Luka Dschaak <l.dschaak@neusta.de>
* [Export All][Refactoring] fix it * [Export All][Refactoring] check import by path inheritance except parentId * [Export All][Refactoring] add option for output file * [Export All][Refactoring] rename property for Luka * [Export All][Refactoring] add some docs * [Export All][Refactoring][New Commands] for import and export * [Export All][Refactoring][New Commands] review by Bundle Dev Group * [Export All][Refactoring][New Commands] some refactorings and an additional menu for the export filename * [Export All][Refactoring][New Commands] change inlining for better readability and remove old export method * [Export All][Refactoring][New Commands] generalize serialization and add JSON input/output * [Export All][Refactoring][New Commands] fix tests * [Export All][Refactoring][New Commands] add parameter for format to Controllers and js Scripts
Co-authored-by: Jacob Dreesen <jacob@hdreesen.de>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe pull request updates documentation and configuration while adding new features for importing and exporting pages. New Symfony CLI commands for export and import have been implemented along with enhancements in JavaScript for dynamic menu management. The codebase has been refactored to support multi-page operations in both controllers and document conversion, with new serializer services for YAML and JSON. Additional configuration files, tests, and translation updates ensure improved maintainability and broader functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as Command Line
participant EPC as ExportPagesCommand
participant PR as PageRepository
participant PE as PageExporter
participant FS as File System
U->>CLI: Run export command
CLI->>EPC: Execute command (neusta:pimcore:export:pages:all)
EPC->>PR: Retrieve page(s) (including subpages)
PR-->>EPC: Return pages list
EPC->>PE: Convert pages to specified format (YAML/JSON)
PE-->>EPC: Return serialized data
EPC->>FS: Write output file
FS-->>EPC: File write confirmation
EPC->>CLI: Display success message
sequenceDiagram
participant U as User
participant CLI as Command Line
participant IPC as ImportPagesCommand
participant FS as File System
participant PI as PageImporter
participant S as Serializer
U->>CLI: Run import command with input file
CLI->>IPC: Execute command (neusta:pimcore:import:pages)
IPC->>FS: Read input YAML/JSON file
FS-->>IPC: Return file content
IPC->>PI: Import pages (content, format, dry run flag)
PI->>S: Deserialize input data
S-->>PI: Return configuration array
PI->>PI: Process each page & verify hierarchy
PI-->>IPC: Return import summary
IPC->>CLI: Display import results and success message
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 7
♻️ Duplicate comments (1)
tests/Integration/Documents/Export/PageExporterTest.php (1)
44-45:⚠️ Potential issueFix the test assertion error in testTreePagesExport method
The pipeline is failing with the error:
Failed asserting that two strings are equal. Expected 'test_document_2' but got 'test_document_3'on line 126. This issue is related to the snapshot assertion in thetestTreePagesExportmethod, implying a mismatch between the generated YAML and the expected snapshot.Please verify that your snapshot files are correct and up-to-date.
🧹 Nitpick comments (25)
compose.yaml (1)
18-20: Ensure the use of the debug image is intentional.
The PHP service now uses the debug image (pimcore/pimcore:php8.3-debug-latest), which is ideal for development and debugging. Please verify that this image is not accidentally deployed in a production environment.tests/app/config/services.yaml (1)
1-8: Clear test service configuration.
The service definitions with autowiring, autoconfiguration, and public access are set up cleanly. The registration of the test serviceNeusta\Pimcore\ImportExportBundle\Tests\Integration\Documents\ImportExportYamlDriveris straightforward. Consider adding a brief inline comment about its purpose for future maintainability if it’s extended further.src/Serializer/JsonSerializer.php (2)
21-24: Format parameter is ignored in the serialize methodThe
serializemethod takes a$formatparameter but ignores it, always using 'json' as the format. While this works for a JSON serializer, it doesn't fully respect the interface contract which suggests the format parameter should influence the serialization process.Consider one of these approaches:
- Use the provided format parameter (if it matches 'json')
- Throw an exception if the format is not 'json'
- Document the ignored parameter with a comment
public function serialize(mixed $data, string $format): string { + // Format parameter is ignored, always using 'json' return $this->jsonSerializer->serialize($data, 'json'); }
26-29: Inconsistent implementation approach between serialize and deserializeWhile
serialize()uses Symfony's serializer implementation,deserialize()bypasses it and directly uses PHP'sjson_decode(). This inconsistency could lead to subtle differences in handling edge cases.Consider using the Symfony serializer consistently for both operations:
public function deserialize(string $data, string $format): mixed { - return json_decode($data, true); + // Format parameter is ignored, always using 'json' + return $this->jsonSerializer->deserialize($data, 'json', 'array'); }tests/Integration/Documents/ImportExportYamlDriver.php (1)
14-16: Error message references JSON instead of YAMLThe error message mentions JSON serialization but this is a YAML driver.
if (!\is_string($data)) { - throw new \InvalidArgumentException('Only strings can be serialized to json'); + throw new \InvalidArgumentException('Only strings can be serialized to yaml'); }src/Serializer/SerializerStrategy.php (1)
12-15: Use typed property for format map in PHP 8+Consider enhancing type safety by using a more specific type hint for the
$formatToSerializerMapproperty:- private array $formatToSerializerMap, + private array $formatToSerializerMap, /** @var array<string, SerializerInterface> */For PHP 8.1+, you could use:
private readonly array $formatToSerializerMap, /** @var array<string, SerializerInterface> */src/Serializer/YamlSerializer.php (1)
22-29: Consider adding format validationThe
serializemethod doesn't use or validate the provided$formatparameter. Since this is a YAML-specific serializer, consider validating that the format is 'yaml' to prevent misuse.public function serialize(mixed $data, string $format): string { + if ($format !== 'yaml') { + throw new \InvalidArgumentException('YamlSerializer only supports yaml format'); + } return $this->serializer->serialize($data, 'yaml', [ 'yaml_inline' => 6, 'yaml_indent' => 0, 'yaml_flags' => self::YAML_DUMP_FLAGS, ]); }tests/Integration/Documents/Export/PageExporterTest.php (3)
71-87: Lack of assertions for multiple documentsWhile the
testSimpleSavedPagesExportmethod correctly exports two pages to YAML, it only asserts the result against a snapshot without verifying properties of individual documents or their relationships. Consider adding assertions to validate specific properties and structure of the exported pages.
105-127: Missing validation for tree structure relationshipsThe
testTreePagesExportmethod creates a hierarchical structure with parent-child relationships, but doesn't explicitly validate these relationships in the exported YAML beyond using the snapshot. Consider adding specific assertions to verify that the parent-child relationships are correctly preserved in the export.$yaml = $this->exporter->export([$page1, $page2, $page3], 'yaml'); $this->assertMatchesSnapshot($yaml, new ImportExportYamlDriver()); + +// Additional assertions to verify hierarchy +$yamlArray = \Symfony\Component\Yaml\Yaml::parse($yaml); +$exportedPages = $yamlArray['documents']; + +// Verify the parent-child relationships are preserved +$this->assertEquals($page1->getId(), $exportedPages[1]['document']['parentId'], 'Page2 should have Page1 as parent'); +$this->assertEquals($page2->getId(), $exportedPages[2]['document']['parentId'], 'Page3 should have Page2 as parent');🧰 Tools
🪛 GitHub Actions: Tests
[error] 126-126: Failed asserting that two strings are equal. Expected 'test_document_2' but got 'test_document_3'.
89-103: Validate path consistency in unsaved pages exportThe
testSimpleUnsavedPagesExportmethod tests exporting unsaved pages, but both pages have the same path ("/"). Consider testing with distinct paths to ensure path handling works correctly for unsaved pages.$page1 = new Page(); $page1->setParentId(1); $page1->setKey('test_document_1'); -$page1->setPath('/'); +$page1->setPath('/path1/'); $page2 = new Page(); $page2->setParentId(1); $page2->setKey('test_document_2'); -$page2->setPath('/'); +$page2->setPath('/path2/');tests/Integration/Documents/Import/PageImporterTest.php (3)
72-115: Avoid duplication in JSON testThe JSON import test largely duplicates the logic and assertions from the YAML parent_id test. Consider refactoring to extract common assertion logic into a private helper method to reduce code duplication.
+ private function assertCommonPageProperties($page, $id, $path, $key, $title, $type, $controller, $language, $navigationName, $navigationTitle): void + { + self::assertEquals($id, $page->getId()); + self::assertEquals($path, $page->getPath()); + self::assertEquals($key, $page->getKey()); + self::assertEquals($title, $page->getTitle()); + self::assertEquals($type, $page->getType()); + self::assertEquals($controller, $page->getController()); + self::assertEquals($language, $page->getProperty('language')); + self::assertEquals($navigationName, $page->getProperty('navigation_name')); + self::assertEquals($navigationTitle, $page->getProperty('navigation_title')); + }
151-179: Add validation for parent-child relationshipsIn the tree case import test, consider adding assertions to validate that parent-child relationships are correctly maintained after import, by checking the actual parentId values of the imported pages.
$pages = $this->importer->import($yaml, 'yaml'); self::assertEquals('/test_document_1/test_document_1_1/', $pages[2]->getPath()); +self::assertEquals($pages[0]->getId(), $pages[1]->getParentId(), 'Second page should have first page as parent'); +self::assertEquals($pages[1]->getId(), $pages[2]->getParentId(), 'Third page should have second page as parent');
181-215: Add test case for conflicting parent/path scenariosConsider adding a test case that specifically tests what happens when there's a conflict between the parentId and path information, to ensure the resolver logic properly handles this edge case according to your documentation.
README.md (3)
26-28: Improve readability with proper comma placementAdd a comma after the first sentence to improve readability.
-After that you will be asked for a file name and the export will start: +After that, you will be asked for a file name and the export will start:🧰 Tools
🪛 LanguageTool
[typographical] ~26-~26: Consider adding a comma here.
Context: ...](docs/images/context_menu_export.png) After that you will be asked for a file name and t...(FOR_THAT_COMMA)
169-176: Improve readability by varying sentence structureThe current paragraph has several consecutive sentences beginning with "If". Consider varying the sentence structure for better readability.
-If `forcedSave` is set to `true`, the ID will be retained (Caution – this can overwrite an existing page). -If a `parentId` is specified, the corresponding document will be searched for. -If it exists, it will be set as the parent (Note: This may override the `path` specification). -If the `parentId` does not exist, an attempt will be made to find a parent using the `path` specification. -If such a parent exists, the `parentId` will be set accordingly and saved. - -If neither is found, an InvalidArgumentException will be thrown, and the save operation will be aborted. +When `forcedSave` is set to `true`, the ID will be retained (Caution – this can overwrite an existing page). +For cases where a `parentId` is specified, the corresponding document will be searched for. +Should it exist, it will be set as the parent (Note: This may override the `path` specification). +In case the `parentId` does not exist, an attempt will be made to find a parent using the `path` specification. +When such a parent exists, the `parentId` will be set accordingly and saved. + +However, if neither a valid parent ID nor path is found, an InvalidArgumentException will be thrown, and the save operation will be aborted.🧰 Tools
🪛 LanguageTool
[style] ~169-~169: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...– this can overwrite an existing page). If aparentIdis specified, the correspo...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~170-~170: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...sponding document will be searched for. If it exists, it will be set as the parent...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~171-~171: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...may override thepathspecification). If theparentIddoes not exist, an attem...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~172-~172: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... parent using thepathspecification. If such a parent exists, theparentIdwi...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~174-~174: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Id` will be set accordingly and saved. If neither is found, an InvalidArgumentExc...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~176-~176: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nd the save operation will be aborted. If multiple pages are imported and a path ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
210-210: Add missing comma for better readabilityAdd a comma after "With that technique" to improve readability.
-With that technique you can export test pages for Fixtures, change values into placeholders (e.g. for assets and data objects) and replace them with the actual values in your tests. +With that technique, you can export test pages for Fixtures, change values into placeholders (e.g. for assets and data objects) and replace them with the actual values in your tests.🧰 Tools
🪛 LanguageTool
[typographical] ~210-~210: It appears that a comma is missing.
Context: ...o implement your own filter. With that technique you can export test pages for Fixtures,...(DURING_THAT_TIME_COMMA)
src/Command/ImportPagesCommand.php (1)
58-58: Improve success message for dry-run modeThe current success message doesn't differentiate between actual imports and dry runs. Update the message to clarify when pages would have been imported in dry-run mode.
-$this->io->success(\sprintf('%d pages have been imported successfully', \count($pages))); +if ($input->getOption('dry-run')) { + $this->io->success(\sprintf('%d pages would be imported (dry-run)', \count($pages))); +} else { + $this->io->success(\sprintf('%d pages have been imported successfully', \count($pages))); +}src/Documents/Export/PageExporter.php (2)
26-32: Minor typo in the DocBlock.
There is an extra parenthesis in “(yaml, json, ...)).” Consider removing the redundant parenthesis for clarity.- * Exports one or more pages in the given format (yaml, json, ...)). + * Exports one or more pages in the given format (yaml, json, ...).
35-45: Ensure proper handling of unsupported Document types.
Currently, onlyPimcorePage,PageSnippet, andFolderinstances are processed. Other Document types are silently skipped. If that behavior is intentional, consider logging or documenting it for clarity.public/js/importPage.js (2)
20-57: Well-implemented import dialogThe dialog implementation follows Ext JS best practices with proper layout, modal behavior, and form structure. The file input correctly restricts uploads to YAML/YML files and provides appropriate labels and validation.
One suggestion - consider adding a help text or tooltip explaining the expected YAML structure for better user guidance.
110-113: Error handling could be enhancedThe error handler correctly displays the error message, but consider providing more guidance to users on how to resolve common issues.
- pimcore.helpers.showNotification(t('neusta_pimcore_import_export_import_dialog_notification_error'), response.message || 'Import failed', 'error'); + pimcore.helpers.showNotification( + t('neusta_pimcore_import_export_import_dialog_notification_error'), + response.message || t('neusta_pimcore_import_export_import_dialog_default_error'), + 'error' + ); + console.error('Import error details:', response);config/converters_populators.yaml (2)
6-9: Simple import populator configurationThe import populator is defined without additional configuration, which suggests a straightforward implementation. This is fine if the populator handles all necessary logic internally, but consider documenting its responsibilities for better maintainability.
54-59: Specialized editable data populatorThe dedicated EditableDataPopulator for handling editable entities to YamlEditable format separation of concerns. Consider adding a comment describing its specific responsibility for better documentation.
src/Command/ExportPagesCommand.php (1)
50-74: Potential optimization for page collectionThe code for collecting pages works correctly but could be optimized for better memory usage and performance.
For very large document trees, consider:
- Adding a depth limit option
- Implementing iterator-based collection instead of building a large array
- Adding a progress bar for visual feedback during collection
src/Controller/Admin/PageImportController.php (1)
15-28: Well-defined constants and message mappingGood use of constants for error and success states with a clear message mapping array. This improves code readability and maintainability.
Based on previous review comments, you could consider using class constants for the messages as well:
- private array $messagesMap = [ - self::ERR_NO_FILE_UPLOADED => 'No file uploaded', - self::SUCCESS_DOCUMENT_REPLACEMENT => 'replaced successfully', - self::SUCCESS_WITHOUT_REPLACEMENT => 'not replaced', - self::SUCCESS_NEW_DOCUMENT => 'imported successfully', - ]; + private const MESSAGE_MAP = [ + self::ERR_NO_FILE_UPLOADED => 'No file uploaded', + self::SUCCESS_DOCUMENT_REPLACEMENT => 'replaced successfully', + self::SUCCESS_WITHOUT_REPLACEMENT => 'not replaced', + self::SUCCESS_NEW_DOCUMENT => 'imported successfully', + ]; + + private array $messagesMap; + + public function __construct( + private PageImporter $pageImporter, + private PageRepository $pageRepository, + ) { + $this->messagesMap = self::MESSAGE_MAP; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
docs/images/context_menu_export.pngis excluded by!**/*.pngdocs/images/context_menu_import_export.pngis excluded by!**/*.pngdocs/images/filename_dialog.pngis excluded by!**/*.pngdocs/images/import_menu.pngis excluded by!**/*.png
📒 Files selected for processing (36)
README.md(3 hunks)compose.yaml(1 hunks)composer.json(1 hunks)config/converters_populators.yaml(1 hunks)config/pimcore/config.yaml(3 hunks)config/services.yaml(3 hunks)public/js/exportPage.js(1 hunks)public/js/importPage.js(1 hunks)src/Command/ExportPagesCommand.php(1 hunks)src/Command/ImportPagesCommand.php(1 hunks)src/Controller/Admin/PageExportController.php(2 hunks)src/Controller/Admin/PageImportController.php(2 hunks)src/Documents/Export/Converter/DocumentTypeStrategyConverter.php(1 hunks)src/Documents/Export/PageExporter.php(1 hunks)src/Documents/Export/Populator/EditableDataPopulator.php(1 hunks)src/Documents/Import/PageImporter.php(1 hunks)src/Documents/Import/Populator/PageImportPopulator.php(1 hunks)src/Documents/Model/Editable.php(1 hunks)src/Documents/Model/Page.php(1 hunks)src/Serializer/JsonSerializer.php(1 hunks)src/Serializer/SerializerInterface.php(1 hunks)src/Serializer/SerializerStrategy.php(1 hunks)src/Serializer/YamlSerializer.php(1 hunks)src/Toolbox/Repository/PageRepository.php(1 hunks)tests/Integration/Documents/Export/PageExporterTest.php(1 hunks)tests/Integration/Documents/Export/__snapshots__/PageExporterTest__testSimpleSavedPagesExport__1.yaml(1 hunks)tests/Integration/Documents/Export/__snapshots__/PageExporterTest__testSimpleUnsavedPagesExport__1.yaml(1 hunks)tests/Integration/Documents/Export/__snapshots__/PageExporterTest__testSinglePageExportAsJson__1.json(1 hunks)tests/Integration/Documents/Export/__snapshots__/PageExporterTest__testSinglePageExport__1.yaml(1 hunks)tests/Integration/Documents/Export/__snapshots__/PageExporterTest__testTreePagesExport__1.yaml(1 hunks)tests/Integration/Documents/Import/PageImporterTest.php(1 hunks)tests/Integration/Documents/ImportExportYamlDriver.php(1 hunks)tests/Unit/Controller/Admin/PageExportControllerTest.php(2 hunks)tests/app/config/services.yaml(1 hunks)translations/admin.de.yaml(1 hunks)translations/admin.en.yaml(1 hunks)
🧰 Additional context used
🧬 Code Definitions (16)
tests/Unit/Controller/Admin/PageExportControllerTest.php (1)
src/Documents/Export/PageExporter.php (1) (1)
export(33-47)
src/Serializer/JsonSerializer.php (1)
src/Serializer/SerializerInterface.php (2) (2)
serialize(9-9)deserialize(11-11)
public/js/exportPage.js (1)
public/js/importPage.js (1) (1)
menu(9-9)
src/Serializer/YamlSerializer.php (1)
src/Serializer/SerializerInterface.php (2) (2)
serialize(9-9)deserialize(11-11)
src/Documents/Export/Populator/EditableDataPopulator.php (2)
src/Documents/Model/Editable.php (1) (1)
Editable(5-10)src/Documents/Import/Populator/PageImportPopulator.php (1) (1)
populate(20-31)
src/Serializer/SerializerInterface.php (3)
src/Serializer/JsonSerializer.php (2) (2)
serialize(21-24)deserialize(26-29)src/Serializer/SerializerStrategy.php (2) (2)
serialize(17-23)deserialize(25-31)src/Serializer/YamlSerializer.php (2) (2)
serialize(22-29)deserialize(31-34)
tests/Integration/Documents/Import/PageImporterTest.php (1)
src/Documents/Import/PageImporter.php (2) (2)
PageImporter(14-68)import(31-56)
tests/Integration/Documents/Export/PageExporterTest.php (3)
src/Documents/Export/PageExporter.php (2) (2)
PageExporter(15-48)export(33-47)src/Documents/Model/Editable.php (1) (1)
Editable(5-10)src/Documents/Model/Page.php (1) (1)
Page(5-38)
src/Command/ImportPagesCommand.php (1)
src/Documents/Import/PageImporter.php (3) (3)
PageImporter(14-68)__construct(19-23)import(31-56)
src/Documents/Export/Converter/DocumentTypeStrategyConverter.php (1)
src/Documents/Model/Page.php (2) (2)
Page(5-38)__construct(27-37)
src/Controller/Admin/PageExportController.php (2)
src/Toolbox/Repository/PageRepository.php (1) (1)
findAllPagesWithSubPages(26-35)src/Documents/Export/PageExporter.php (1) (1)
export(33-47)
src/Documents/Import/PageImporter.php (2)
src/Documents/Model/Page.php (2) (2)
Page(5-38)__construct(27-37)src/Serializer/SerializerInterface.php (1) (1)
deserialize(11-11)
public/js/importPage.js (1)
public/js/exportPage.js (2) (2)
document(10-10)menu(9-9)
src/Documents/Export/PageExporter.php (3)
src/Documents/Model/Page.php (2) (2)
Page(5-38)__construct(27-37)src/Documents/Export/Converter/DocumentTypeStrategyConverter.php (2) (2)
__construct(18-21)convert(23-30)src/Serializer/SerializerInterface.php (1) (1)
serialize(9-9)
src/Controller/Admin/PageImportController.php (2)
src/Documents/Import/PageImporter.php (2) (2)
PageImporter(14-68)import(31-56)src/Toolbox/Repository/PageRepository.php (1) (1)
PageRepository(16-36)
src/Command/ExportPagesCommand.php (1)
src/Documents/Export/PageExporter.php (3) (3)
PageExporter(15-48)__construct(20-24)export(33-47)
🪛 LanguageTool
README.md
[typographical] ~26-~26: Consider adding a comma here.
Context: ...](docs/images/context_menu_export.png) After that you will be asked for a file name and t...
(FOR_THAT_COMMA)
[style] ~169-~169: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...– this can overwrite an existing page). If a parentId is specified, the correspo...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~170-~170: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...sponding document will be searched for. If it exists, it will be set as the parent...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~171-~171: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...may override the path specification). If the parentId does not exist, an attem...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~172-~172: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... parent using the path specification. If such a parent exists, the parentId wi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~174-~174: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Id` will be set accordingly and saved. If neither is found, an InvalidArgumentExc...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~176-~176: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nd the save operation will be aborted. If multiple pages are imported and a path ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[typographical] ~210-~210: It appears that a comma is missing.
Context: ...o implement your own filter. With that technique you can export test pages for Fixtures,...
(DURING_THAT_TIME_COMMA)
🪛 GitHub Actions: Tests
tests/Integration/Documents/Export/PageExporterTest.php
[error] 126-126: Failed asserting that two strings are equal. Expected 'test_document_2' but got 'test_document_3'.
tests/Integration/Documents/ImportExportYamlDriver.php
[error] 28-28: PHPUnit test failed with exit code 1.
🪛 PHPStan (2.0.3)
src/Controller/Admin/PageImportController.php
51-51: Parameter #2 $format of method Neusta\Pimcore\ImportExportBundle\Documents\Import\PageImporter::import() expects string, bool|float|int|string|null given.
(argument.type)
src/Documents/Import/Populator/PageImportPopulator.php
22-22: Call to function property_exists() with Neusta\Pimcore\ImportExportBundle\Documents\Model\Page and 'language' will always evaluate to true.
Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.
(function.alreadyNarrowedType)
🔇 Additional comments (87)
composer.json (1)
27-29: New snapshot assertions dependency added.
The new development dependency"spatie/phpunit-snapshot-assertions": "^4.2"looks correctly added in therequire-devsection. Ensure your tests utilizing snapshots are updated and compatible with version 4.2 of this library.tests/Integration/Documents/Export/__snapshots__/PageExporterTest__testSinglePageExport__1.yaml (1)
1-20: Snapshot for single page export is well-structured.
The YAML snapshot correctly encapsulates the expected export format for a single document, including all key attributes and nested properties. No issues found; ensure that any updates to the exporter logic keep the snapshot in sync.tests/Integration/Documents/Export/__snapshots__/PageExporterTest__testSimpleSavedPagesExport__1.yaml (1)
1-30: Snapshot for multiple pages export is comprehensive.
The snapshot accurately represents two documents, each with their distinct properties, and uses YAML null indicators (~) consistently for empty fields. This file should serve well for validating multi-page export functionality.src/Toolbox/Repository/PageRepository.php (1)
22-35: Well-implemented recursive page traversal with generators.The
findAllPagesWithSubPagesmethod effectively uses generator functions (yieldandyield from) to efficiently traverse the page hierarchy without loading all pages into memory at once. This approach is ideal for potentially large page structures.A few observations:
- The method correctly yields the parent page first before recursively processing children
- The type check ensures only proper Page objects are included
- The return type hint
iterable<Page>clearly communicates what's being returnedtests/Integration/Documents/Export/__snapshots__/PageExporterTest__testSinglePageExportAsJson__1.json (1)
1-26: Test snapshot looks good with appropriate document structure.The JSON structure correctly represents a document with all necessary properties for testing page export functionality. It includes document metadata (ID, path, type), content properties (title, navigation settings), and editables array for page content.
tests/Integration/Documents/Export/__snapshots__/PageExporterTest__testTreePagesExport__1.yaml (1)
1-43:Details
❓ Verification inconclusive
Test snapshot correctly models hierarchical page structure.
The YAML snapshot properly demonstrates a three-level page hierarchy with parent-child relationships:
- Document 2 (parent: 1)
- Document 3 (parent: 2)
- Document 4 (parent: 3)
This is essential for testing the tree-based export functionality that's being implemented.
One minor inconsistency to verify:
🏁 Script executed:
#!/bin/bash # Check if page with id 4 might have a duplicate key grep -A 15 "id: 4" tests/Integration/Documents/Export/__snapshots__/PageExporterTest__testTreePagesExport__1.yaml grep -A 3 "key: test_document_2" tests/Integration/Documents/Export/__snapshots__/PageExporterTest__testTreePagesExport__1.yamlLength of output: 997
Action: Verify Duplicate Key in YAML Snapshot
The snapshot correctly models a three-level page hierarchy:
- Document 2 (parent: 1)
- Document 3 (parent: 2)
- Document 4 (parent: 3)
However, verification confirms that both Document 3 and Document 4 are using the same key (
test_document_2). Please verify if this duplicate key is intentional in your export functionality or if it requires correction.tests/Integration/Documents/Export/__snapshots__/PageExporterTest__testSimpleUnsavedPagesExport__1.yaml (1)
1-29: Test snapshot correctly handles unsaved pages.The YAML structure properly represents unsaved pages with null IDs (
~) while maintaining all other required properties. This correctly tests the exporter's ability to handle pages that haven't been persisted yet.A minor observation: both documents have the same path (
/), which might be expected for unsaved documents.src/Documents/Model/Editable.php (1)
3-5: Good refactoring to a more generic model structure.The namespace and class name changes correctly move this from being YAML-export specific to a more generic model entity. This change aligns well with supporting multiple serialization formats in the export/import functionality.
src/Serializer/SerializerInterface.php (1)
7-12: Well-designed serialization interface.This interface is cleanly designed, focusing solely on serialization concerns. The format parameter enables support for multiple serialization formats, which aligns perfectly with the PR objective of improving import/export capabilities. The use of strategy pattern in implementation (seen in related files) makes the system easily extensible for future formats.
tests/Unit/Controller/Admin/PageExportControllerTest.php (2)
46-46: Tests properly updated for new multi-page export API.The test has been correctly updated to use the new
exportmethod which accepts an array of pages and a format parameter. This change maintains test coverage while adapting to the enhanced API that supports exporting multiple pages.
60-60: Error handling test correctly updated.The exceptional case test has been properly updated to use the new
exportmethod signature while maintaining the same error handling test coverage.translations/admin.en.yaml (3)
1-1: Good addition for improved user experience.This translation key enhances the UX by providing clear guidance for filename entry with a sensible default.
3-3: Appropriate label for the new export with children feature.This translation supports the new functionality for exporting pages with their children, which aligns with the PR objective of enhancing export capabilities.
Note: The value matches the suggestion from a previous review comment.
10-10: Improved feedback with more descriptive import message.Changing from 'Import successful' to 'Import Summary' suggests that the import process now provides more detailed feedback, which is appropriate for a feature handling multiple pages.
public/js/exportPage.js (3)
14-15: LGTM! Improved menu items for export optionsThe implementation now separates regular export from "export with children" functionality, enhancing user options.
18-30: Great refactoring with the new addMenuItem methodThe refactoring improves code maintainability by extracting the common menu item creation logic into a separate method, making it easier to add new export options in the future.
23-27: UX improvement with filename promptAllowing users to specify the export filename enhances the user experience. The default filename using the document key is a sensible choice.
translations/admin.de.yaml (2)
3-3: Alternative translation for export with children labelA previous reviewer suggested a slightly different translation for this string.
neusta_pimcore_import_export_export_with_children_menu_label: 'Exportiere in YAML mit Kindelementen'
10-10: Changed meaning in success message translationThe translation has been changed from "Import erfolgreich" (Import successful) to "Import Zusammenfassung" (Import summary), which changes the meaning of the message. This might affect user expectations.
Is this change intentional? The English equivalent should also be checked to ensure consistency between languages.
src/Documents/Export/Populator/EditableDataPopulator.php (1)
15-22: Clearly defined populator for Pimcore editablesThe
populatemethod effectively extracts data from Pimcore editable objects with specialized handling for relation types. This is a well-structured implementation that correctly differentiates between regular editables and relation editables.For future enhancement, consider adding null checks before data assignment for improved robustness.
src/Serializer/SerializerStrategy.php (2)
17-23: Well-designed serializer delegation patternThe strategy pattern implementation for serialization is clean and elegant. It properly delegates to specialized serializers based on the format and handles error cases appropriately.
25-31: Consistent implementation for deserializationThe deserialization method follows the same pattern as the serialization method, providing a symmetrical and predictable API.
src/Serializer/YamlSerializer.php (1)
12-16: Clear YAML configuration flagsThe constant definition combines appropriate YAML serialization flags for handling invalid types, multi-line blocks, and null values representation.
src/Documents/Export/Converter/DocumentTypeStrategyConverter.php (2)
18-21: Style suggestion for property definitionThere was a previous review comment suggesting a change to the property definition style:
- public function __construct( - private array $typeToConverterMap, - ) { + public function __construct( + private array $typeToConverterMap, + ) {This is a stylistic change that maintains consistency with the codebase style.
23-30: Well-implemented converter strategy patternThe converter correctly delegates to specialized converters based on the source class and handles missing converters appropriately. This implementation follows the strategy pattern effectively.
tests/Integration/Documents/Import/PageImporterTest.php (1)
22-37: LGTM: Good exceptional case testingExcellent approach in testing the exceptional case when a non-existent path is provided. This ensures the importer correctly handles invalid inputs and provides appropriate error messages.
README.md (2)
38-88: LGTM: Well-documented export commandThe export command documentation is thorough and well-structured, with clear examples and explanations of all options. This will greatly help users understand how to use the command effectively.
88-130: LGTM: Well-documented import commandThe import command documentation is clear, concise, and provides good examples of both normal usage and dry-run mode. This comprehensive documentation will make it easy for users to understand and utilize the command.
src/Documents/Export/PageExporter.php (4)
7-13: Imports are appropriate and consistent.
These imports align perfectly with the classes used later in the file. No issues spotted.
18-18: DocBlock param synchronization.
The updated docblock correctly reflects the new converter signature (Converter<Document, Page, GenericContext|null>).
33-33: Method signature appropriately widened.
The export method now accepts multiple pages, improving reusability for batch operations.
46-46: Successful delegation to the serializer.
Returning the serialized data for[Page::PAGES => $yamlExportPages]meets the multi-document export requirement.src/Documents/Import/PageImporter.php (9)
7-13: Imports align with new functionality.
The added imports forConverterException,Page,SerializerInterface, andDuplicateFullPathExceptionare relevant to the updated import logic.
17-17: DocBlock update acknowledges converter’s types.
Correctly specifies that$yamlToPageConverterinvolvesPageandPimcorePage.
21-21: Injection of SerializerInterface.
Injecting the serializer fosters flexibility for multiple formats (YAML, JSON, etc.).
25-32: Method signature and DocBlock are coherent.
Theimportmethod returns anarray<PimcorePage>and throws only documented exceptions. Nicely done.
33-33: Deserializer usage is well-structured.
Deserializing based on$formatallows a clean handoff to the converter.
35-37: Domain validation ensures correct data shape.
The guard clause checks forPage::PAGESin the data array, avoiding deeper errors.
39-53: Loop correctly converts and persistently saves pages.
Your approach to convert each config item to aPageand optionallysave()if$forcedSaveis true meets typical import expectations.
55-55: Returning the pages array completes the import sequence.
Ensuring a consistent return type fosters predictability for callers.
58-67: checkAndUpdatePage ensures valid parent references.
If a given parent doesn’t exist, the fallback path-based lookup is logical. Throwing an exception for unresolved parents clarifies the import failure point.src/Controller/Admin/PageExportController.php (8)
7-7: Consistent aliasing of PimcorePage.
The aliasPimcorePagehelps distinguish between domainPageand Pimcore’s native Document class.
29-38: Graceful 404 handling on missing page.
UsingJsonResponsewith a “not found” status is a clean way to handle invalid IDs.
40-45: New route for exporting page plus subpages is clear.
Introducing a separate endpoint clarifies explicit behaviors for recursive exports.
47-51: Similarity in error response.
Ensuring consistent 404 logic helps standardize user expectations across endpoints.
55-55: Efficient recursion for subpage discovery.
UsingfindAllPagesWithSubPagesyields a comprehensive subtree for export. Good approach.
57-57: Parameterized format.
Allowing the user to specify theformatin the query enhances flexibility for multiple export options.
60-65: Extracted helper method for retrieving pages by ID.
Having a dedicated function for this retrieval reduces duplication and centralizes the logic.
67-81: try/catch block ensures robust error reporting.
Wrapping the export in a try/catch and returning a JSON error fosters consistent, user-friendly error handling.public/js/importPage.js (4)
5-5: Improved event handling approachThe change from
prepareDocumentTreeContextMenutopreMenuBuildis a good architectural improvement. This aligns with modern event handling patterns and provides more flexibility in menu construction.
8-18: Well-structured menu integrationThe menu configuration is properly defined with all necessary properties (label, icon, priority, handler). The priority value of 50 gives appropriate placement in the menu hierarchy. The
noSubmenus: trueproperty ensures a clean UI.
59-78: Robust form submission implementationThe form submission includes proper validation, CSRF protection, and appropriate headers. The loading message provides good user feedback during the import process.
80-108: Comprehensive success handlingThe success handler properly decodes the response, displays a confirmation dialog, and refreshes the document tree. This provides complete feedback to the user and ensures the UI is updated appropriately.
config/converters_populators.yaml (4)
1-4: Well-configured defaultsGood use of Symfony's autowiring and autoconfiguration features, which simplifies service management and promotes a clean dependency injection approach.
11-21: Excellent use of strategy patternThe DocumentTypeStrategyConverter with type-to-converter mapping is a great implementation of the strategy pattern. This approach makes it easy to add support for new document types in the future without modifying existing code.
26-32: Well-structured array conversion populatorThe array converting populator for editables is properly configured with source and target property names. This makes the conversion process clear and maintainable.
33-52: Consistent property mapping configurationThe property-based mapping populators follow a consistent pattern with clear naming conventions. The
skipNull: truesetting across all populators is a good practice that avoids cluttering exported data with null values.src/Command/ExportPagesCommand.php (3)
16-27: Well-structured command classThe class follows Symfony command best practices with proper annotation, inheritance, and dependency injection. Constructor dependencies are correctly typed and initialized.
29-44: Clear command configurationThe command options and arguments are well-defined with appropriate descriptions and defaults. The array argument for IDs provides good flexibility.
94-109:Details
❓ Verification inconclusive
Recursive collection with potential stack overflow risk
The recursive approach works but could cause stack overflow with very deep document trees.
Consider implementing an iterative approach using a queue for very deep document trees:
private function addPagesIteratively(Document $rootPage): array { $allPages = []; $queue = [$rootPage]; while (!empty($queue)) { $page = array_shift($queue); $allPages[] = $page; foreach ($page->getChildren(true) as $childPage) { if ($childPage instanceof Document) { $queue[] = $childPage; } } } return $allPages; }
🏁 Script executed:
#!/bin/bash # Check potential depth of document tree rg -i "getChildren.*instanceof Document" --include "*.php" -A 5 -B 5 # Look for any recursive traversal of document tree to compare approaches rg -i "recursive.*document" --include "*.php" -A 5 -B 5Length of output: 1012
Attention: Verify Deep Document Tree Traversal
The current implementation of
addPagesuses recursion, which is correct for most document trees but may lead to a stack overflow if the tree is very deep. The initial verification usingrg --includefailed due to an unsupported flag; please run the above corrected commands to check for deep recursive patterns within the PHP files.If you find that deep document trees are a potential use case, consider refactoring the method to use an iterative approach (for example, using a queue) as demonstrated below:
private function addPagesIteratively(Document $rootPage): array { $allPages = []; $queue = [$rootPage]; while (!empty($queue)) { $page = array_shift($queue); $allPages[] = $page; foreach ($page->getChildren(true) as $childPage) { if ($childPage instanceof Document) { $queue[] = $childPage; } } } return $allPages; }Please manually verify the output of the new search commands and review if the iterative approach is warranted based on the depth of your document trees.
🏁 Script executed:
#!/bin/bash # Re-run the search using the -g flag (instead of --include) to filter PHP files. rg -i "getChildren.*instanceof Document" -g "*.php" -A 5 -B 5 rg -i "recursive.*document" -g "*.php" -A 5 -B 5Length of output: 236
Attention: Confirm Deep Document Tree Usage
The recursive approach in the current implementation of
addPages()appears appropriate for typical document trees. However, there is still a potential risk of stack overflow if document trees become extremely deep. Our automated grep verification did not return evidence of problematic deep recursions, so manual verification is advised.Action Items:
Manually verify the maximum depth of document trees in your application to confirm whether the recursive approach might hit stack overflow limits.
If you anticipate or encounter extremely deep trees, consider refactoring to an iterative version, such as:
private function addPagesIteratively(Document $rootPage): array { $allPages = []; $queue = [$rootPage]; while (!empty($queue)) { $page = array_shift($queue); $allPages[] = $page; foreach ($page->getChildren(true) as $childPage) { if ($childPage instanceof Document) { $queue[] = $childPage; } } } return $allPages; }Please perform a manual check of your document tree depths to decide if this change is necessary.
src/Documents/Model/Page.php (4)
3-5: Improved class naming and namespaceThe namespace change from Export to Model and class rename from YamlExportPage to Page improves reusability. This class now appropriately serves as a model for both import and export operations rather than being export-specific.
7-8: Good use of constantsAdding the PAGES and PAGE constants provides standardized keys for document structures in serialized formats. This reduces string literals throughout the codebase and improves maintainability.
11-20:Details
❓ Verification inconclusive
Improved property defaults
The addition of sensible default values for properties ensures consistency when creating new instances. The change from non-nullable to nullable properties (like navigation_name, title, controller) provides more flexibility.
Verify that any code that relies on these properties is updated to handle null values appropriately.
🏁 Script executed:
#!/bin/bash # Check for any code that might assume non-null values rg -i "navigation_name|title|controller" --include="*.php" -A 2 -B 2 | grep -v "null"Length of output: 527
Attention: Verify Dependent Code Handles Nullable Values
The changes in
src/Documents/Model/Page.php(lines 11–20) introduce sensible default values by making properties such asnavigation_name,title, andcontrollernullable for additional flexibility. However, we need to ensure that any code referencing these properties properly handles potentialnullvalues.The previous verification script using
--includeresulted in an error. Please re-run the verification using an updated command:#!/bin/bash rg -i 'navigation_name|title|controller' -g "*.php" -A2 -B2 | grep -v "null"If the output is inconclusive or if you suspect any code assumes non-null values for these properties, please perform a manual verification.
21-22:Details
❓ Verification inconclusive
Updated type hint for consistency
The type hint change from YamlExportEditable to Editable aligns with the class renaming pattern. Ensure that all related classes and references have been similarly updated.
🏁 Script executed:
#!/bin/bash # Check for any remaining references to YamlExportEditable rg -i "YamlExportEditable" --include="*.php"Length of output: 486
Ensure Consistent Type Hint Update
The type hint change from YamlExportEditable to Editable in
src/Documents/Model/Page.php(lines 21–22) aligns with the class renaming convention. Please verify that no remaining references toYamlExportEditableexist across the codebase by running a search using the revised command above. If any occurrences are found, update them accordingly.--- 🏁 Script executed: ```shell #!/bin/bash # Re-run reference check for YamlExportEditable with corrected flag rg -i "YamlExportEditable" -g "*.php"Length of output: 159
Type Hint Update Verification
- The updated type hint in
src/Documents/Model/Page.php(lines 21–22) now correctly usesEditableinstead ofYamlExportEditable.- A revised search command (
rg -i "YamlExportEditable" -g "*.php") returned no matches, suggesting that all references to the old type have been removed.- Please manually verify the search results to ensure there are no lingering references in the codebase.
src/Documents/Import/Populator/PageImportPopulator.php (5)
3-3: Improved namespace organizationThe namespace has been organized more logically by moving the PageImportPopulator to a dedicated
Populatorsub-namespace, which better reflects its role in the architecture.
7-8: Appropriate model imports with clear aliasingGood use of aliasing
Pimcore\Model\Document\Page as PimcorePageto clearly distinguish between your internal Page model and Pimcore's model, improving code readability and reducing confusion.
11-11: Updated type annotationsThe PHPDoc has been correctly updated to reflect the new model structure with proper generic type parameters.
29-29: Improved type safety with explicit castingThe explicit string casting for the key parameter ensures type safety when passing to
setRawEditable(), preventing potential type-related issues.
25-26:Details
❓ Verification inconclusive
Property naming change
Changed from
titleandkeyto more semanticnavigation_titleandnavigation_name, which better describes their purpose.Verify that all consumers of these properties have been updated:
🏁 Script executed:
#!/bin/bash # Search for uses of the old property names in the codebase echo "Searching for old property references..." rg "->title" --type php rg "->key" --type phpLength of output: 950
Please update the property consumer search commands and manually verify old references
The renaming from
title/keytonavigation_title/navigation_nameis a semantic improvement. However, the verification script encountered errors because the patterns were interpreted as options. Please update the commands as follows to ensure proper searching:
- Replace the current commands with:
rg -- "->title" --type php rg -- "->key" --type php- Manually verify that no consumers of these properties still reference the old names.
config/pimcore/config.yaml (7)
12-12: Updated populator path consistent with namespace changesThe populator path has been updated to match the namespace reorganization in the codebase, maintaining consistency across files.
34-34: Added null handling for path propertyAdded
skip_null: truefor the path property, which is a defensive improvement that prevents errors when paths are null.
42-54: Added new export converter for foldersGood addition of an export converter for document folders, extending the functionality to support the multi-page export/import feature. This enables handling different document types within the same framework.
60-60: Updated target model for exportThe export converter now targets the new model structure, which is consistent with the changes in the Populator class.
79-93: New export converter for page snippetsGood addition of support for exporting page snippets, which further extends the functionality and provides a more complete document export/import solution.
96-98: Updated editable converter target and added data populatorThe editable converter has been updated to use the new model structure and includes a new populator for editable data, which is a necessary change to support the new models.
7-9:Details
❓ Verification inconclusive
Renamed import converter for broader scope
The rename from
import_pagetoimport_documentsuggests a more generic approach to handle different document types, which aligns with the multi-page functionality being implemented.Verify that all references to the old converter name have been updated:
🏁 Script executed:
#!/bin/bash # Search for references to the old converter name rg "import_page" --type yaml --type phpLength of output: 39
I'll run a slightly broader search—one that covers all files, including hidden and ignored files—to ensure no lingering references to the old converter name "import_page" remain. Please execute the following command and verify its output:
Once you have the results, please let us know if any references appear so we can finalize the review accordingly.
🏁 Script executed:
#!/bin/bash # Re-run search for "import_page" across the entire repository including hidden and ignored files. rg "import_page" --hidden --no-ignoreLength of output: 37
Verified Converter Rename – Please Manually Confirm
The search commands did not return any instances of
"import_page", which indicates that the old converter name appears to have been fully replaced by"import_document". The updated snippet inconfig/pimcore/config.yaml(lines 7–9) now reads:# Import Converter (Page -> PimcorePage) ########################################################### neusta_pimcore_import_export.import_document:While the automated searches support that the rename has been applied consistently, please manually verify across the codebase that no lingering references to
"import_page"remain.config/services.yaml (5)
1-2: Improved configuration modularityGood organization by extracting converter and populator configurations to a separate file, which improves maintainability and follows the Single Responsibility Principle.
15-21: Added command services for CLI functionalityExcellent addition of export and import command services, which enables command-line operations for the multi-page functionality. This improves automation capabilities.
37-47: Added serializer strategy with format mappingStrong improvement by implementing a serializer strategy pattern with format-to-serializer mapping for YAML and JSON. This allows for flexible serialization formats and better extensibility.
53-55: Updated PageExporter with serializer strategyThe PageExporter now uses the serializer strategy and has been updated to reference the new converter name, ensuring consistency with other changes.
59-60: Updated PageImporter with serializer strategyThe PageImporter configuration has been properly updated to use the serializer strategy and the renamed converter, maintaining consistency across the codebase.
src/Controller/Admin/PageImportController.php (6)
43-46: Improved file upload handlingGood extraction of file upload logic to a separate method, and appropriate error handling with the new message mapping.
53-63: Enhanced results handling for multiple pagesGood implementation of a results tracking mechanism for multiple page imports, with appropriate counters for different outcome categories.
70-73: Extracted file upload handlingGood extraction of upload file retrieval to a dedicated method, improving code organization and readability.
75-78: Standardized JSON response creationExcellent extraction of JSON response creation to a dedicated method, which standardizes the format and reduces code duplication.
80-96: Updated result codes for consistencyThe method now returns constants instead of strings, which is more consistent and type-safe.
101-118: Message formatting for multiple resultsGood implementation of message formatting to handle multiple results with appropriate singular/plural forms.
Based on previous review comments, an alternative implementation was suggested but your choice for readability is reasonable.
| public function match($expected, $actual) | ||
| { | ||
| Assert::assertEquals($expected, $actual); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Pipeline test failure in match method
There's a pipeline failure reported for line 28, which might indicate an issue with how the assertion is made or with test expectations.
Please investigate the failing test. The error might be related to how YAML comparisons are made. Consider checking:
- Are there format differences in the expected vs. actual YAML?
- Is the serialization/deserialization process preserving all necessary data?
🏁 Script executed:
#!/bin/bash
# Run the specific failing test with verbose output to identify the exact issue
./vendor/bin/phpunit tests/Integration/Documents/ImportExportYamlDriver.php -vLength of output: 246
ENVIRONMENT ISSUE: Missing Required PHP Extensions
Based on the verbose test output, the failure in tests/Integration/Documents/ImportExportYamlDriver.php is not due to the YAML comparison logic in the match() method. Instead, the pipeline is failing because PHPUnit is missing several PHP extensions (dom, xml, and xmlwriter). This indicates the issue lies in the CI environment configuration rather than the code in the match() method.
- The reported failure is caused by missing PHP extensions required by PHPUnit.
- The
match()method's assertion (Assert::assertEquals($expected, $actual);) appears to be functioning correctly. - Action Required: Update the CI environment configuration to enable the required PHP extensions. Re-run the tests once the PHP environment includes these extensions to confirm that no YAML-related discrepancies remain.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 28-28: PHPUnit test failed with exit code 1.
| public function deserialize(string $data, string $format): mixed | ||
| { | ||
| return Yaml::parse($data); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent serialization approach
The deserialize method directly uses Yaml::parse() instead of the injected Symfony serializer that's used for serialization. This creates an inconsistency in the implementation approach.
If there's a specific reason for this asymmetry, consider adding a comment explaining the rationale. Otherwise, consider using the injected serializer for deserialization as well:
public function deserialize(string $data, string $format): mixed
{
- return Yaml::parse($data);
+ if ($format !== 'yaml') {
+ throw new \InvalidArgumentException('YamlSerializer only supports yaml format');
+ }
+ return $this->serializer->deserialize($data, 'yaml', 'array');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function deserialize(string $data, string $format): mixed | |
| { | |
| return Yaml::parse($data); | |
| } | |
| public function deserialize(string $data, string $format): mixed | |
| { | |
| if ($format !== 'yaml') { | |
| throw new \InvalidArgumentException('YamlSerializer only supports yaml format'); | |
| } | |
| return $this->serializer->deserialize($data, 'yaml', 'array'); | |
| } |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Controller/Admin/PageImportController.php (2)
70-74: Consider validating the uploaded fileThe
getUploadedFilemethod simply returns the file from the request without any validation. Consider adding validation to ensure the file exists and is of the expected type.private function getUploadedFile(Request $request): ?UploadedFile { - return $request->files->get('file'); + $file = $request->files->get('file'); + + if ($file instanceof UploadedFile && $file->isValid()) { + return $file; + } + + return null; }
80-80: Consider making replaceIfExists privateThe
replaceIfExistsmethod is marked asprotectedbut there are no child classes that extend this controller. Consider making itprivatesince it's only used within this class.-protected function replaceIfExists(PimcorePage $page, bool $overwrite): int +private function replaceIfExists(PimcorePage $page, bool $overwrite): int
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Controller/Admin/PageImportController.php(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Quality Assurance
src/Controller/Admin/PageImportController.php
[error] 48-48: PHP CS Fixer: Found 1 issue that can be fixed. Please run 'php-cs-fixer fix' to apply the changes.
🔇 Additional comments (5)
src/Controller/Admin/PageImportController.php (5)
15-28: Good addition of constants and message mappingThe introduction of constants for error and success states with a corresponding message map improves code maintainability and readability. This approach makes it easier to understand the different outcomes of operations and ensures consistent messaging throughout the application.
43-46: Clean refactoring with helper methodGood refactoring by extracting the file retrieval logic into a dedicated helper method. This improves code readability and follows the single responsibility principle.
51-51: Proper type casting appliedYou've correctly implemented the type casting for the format parameter as suggested in the previous review. This ensures type safety when calling the
importmethod.
53-63: Well-structured implementation for handling multiple pagesThe refactoring to handle multiple pages is well-implemented. The code now tracks counts for different result types and processes each page individually, which aligns with the PR objective of supporting multiple page imports.
105-115: Maintain your preferred readable formatI see you decided to keep your original formatting rather than accept the previous suggestion. Your approach is clear and readable - processing only entries with a count greater than zero makes the code more efficient.
| return $this->createJsonResponse(false, $this->messagesMap[self::ERR_NO_FILE_UPLOADED], 400); | ||
| } | ||
|
|
||
| $overwrite = $request->request->getBoolean('overwrite'); |
There was a problem hiding this comment.
Fix PHP CS Fixer issue
There's a PHP CS Fixer issue reported on line 48. The pipeline failure doesn't specify the exact problem, but it could be related to spacing or line ending. Run 'php-cs-fixer fix' as suggested in the pipeline failure to address this issue.
🧰 Tools
🪛 GitHub Actions: Quality Assurance
[error] 48-48: PHP CS Fixer: Found 1 issue that can be fixed. Please run 'php-cs-fixer fix' to apply the changes.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Command/ExportPagesCommand.php (2)
80-80: Use fully qualified name forbasename().
To satisfy the code style and silence pipeline warnings, prefix native function calls with a backslash.-$safeFilename = basename($exportFilename); +$safeFilename = \basename($exportFilename);🧰 Tools
🪛 GitHub Actions: Quality Assurance
[warning] 80-80: PHP CS Fixer: native_function_invocation - Please use the fully qualified name for native functions.
93-93: Use fully qualified name forfile_put_contents().
Likewise, adding a backslash ensures consistency with native function invocation rules.-if (!file_put_contents($exportFilename, $yamlContent)) { +if (!\file_put_contents($exportFilename, $yamlContent)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Command/ExportPagesCommand.php(1 hunks)src/Command/ImportPagesCommand.php(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/Command/ExportPagesCommand.php (1)
src/Documents/Export/PageExporter.php (3) (3)
PageExporter(15-48)__construct(20-24)export(33-47)
src/Command/ImportPagesCommand.php (1)
src/Documents/Import/PageImporter.php (3) (3)
PageImporter(14-68)__construct(19-23)import(31-56)
🪛 GitHub Actions: Quality Assurance
src/Command/ExportPagesCommand.php
[warning] 80-80: PHP CS Fixer: native_function_invocation - Please use the fully qualified name for native functions.
src/Command/ImportPagesCommand.php
[warning] 56-56: PHP CS Fixer: native_function_invocation, blank_line_before_statement - Please use the fully qualified name for native functions.
🪛 PHPStan (2.0.3)
src/Command/ImportPagesCommand.php
58-58: Dead catch - DomainException is never thrown in the try block.
(catch.neverThrown)
🔇 Additional comments (6)
src/Command/ImportPagesCommand.php (4)
13-16: Command declaration is clear and consistent.
The command name and description accurately reflect the functionality for importing pages from YAML.
19-23: Constructor injection is succinct and maintains good dependency encapsulation.
The private property ensures thatPageImporterremains internal to the command.
49-54: Add explicit file existence check.
While the code checks if$yamlInputis falsy, consider verifying file existence for clearer error messages.+$inputFile = $input->getOption('input'); +if (!file_exists($inputFile)) { + $this->io->error(\sprintf('Input file "%s" does not exist', $inputFile)); + return Command::FAILURE; +} + $yamlInput = file_get_contents($inputFile); if (!$yamlInput) { $this->io->error('Input file could not be read');
58-58: Static analysis warning is a false positive.
Despite the tool’s hint,PageImporter::import()does throw\DomainExceptionupon invalid data, so this catch block is valid.🧰 Tools
🪛 PHPStan (2.0.3)
58-58: Dead catch - DomainException is never thrown in the try block.
(catch.neverThrown)
src/Command/ExportPagesCommand.php (2)
16-19: Command declaration is clear and effective.
The name and description convey that pages are being exported into YAML.
50-60: Consider pagination or batching to handle large sites.
Collecting an extensive hierarchy of pages in memory at once could cause performance or memory issues.
| /** | ||
| * @var string[] Map of error codes to messages | ||
| */ | ||
| private array $messagesMap = [ |
There was a problem hiding this comment.
@coderabbitai Keep in mind that we should use enums where possible and avoid using arrays as const.
There was a problem hiding this comment.
@jan888adams You're absolutely right! Using PHP enums would be much better here. Since the project is using PHP 8.3, we can take full advantage of enums. Here's how we could refactor this:
<?php declare(strict_types=1);
namespace Neusta\Pimcore\ImportExportBundle\Enum;
enum ImportStatus: int
{
case ERR_NO_FILE_UPLOADED = 1;
case SUCCESS_DOCUMENT_REPLACEMENT = 2;
case SUCCESS_WITHOUT_REPLACEMENT = 3;
case SUCCESS_NEW_DOCUMENT = 4;
public function getMessage(): string
{
return match($this) {
self::ERR_NO_FILE_UPLOADED => 'No file uploaded',
self::SUCCESS_DOCUMENT_REPLACEMENT => 'replaced successfully',
self::SUCCESS_WITHOUT_REPLACEMENT => 'not replaced',
self::SUCCESS_NEW_DOCUMENT => 'imported successfully',
};
}
public function isSuccess(): bool
{
return match($this) {
self::ERR_NO_FILE_UPLOADED => false,
default => true,
};
}
}This provides better type safety, self-documentation, and IDE support. The controller would then use it like this:
use Neusta\Pimcore\ImportExportBundle\Enum\ImportStatus;
// Instead of using constants
$status = ImportStatus::SUCCESS_DOCUMENT_REPLACEMENT;
// Getting the message
$message = $status->getMessage();
// Checking if it's a success
if ($status->isSuccess()) {
// ...
}✏️ Learnings added
Learnt from: jan888adams
PR: teamneusta/pimcore-import-export-bundle#10
File: src/Controller/Admin/PageImportController.php:23-23
Timestamp: 2025-04-01T09:57:29.703Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Has been already merged by #18 |
4 participants
With that PR I'll try to export and import several files by ONE single YAML page.
But this PR is only a preparation step to the unanswered questions:
Anyway this first step is helpful.
Summary by CodeRabbit
New Features
Documentation
Chores