From 8c03de1aae23171a42a80abecdcde535d6a66d7f Mon Sep 17 00:00:00 2001 From: Daniel Siepmann Date: Thu, 2 Nov 2023 14:38:04 +0100 Subject: [PATCH] Fix broken assignment of features and categories We migrated the part of the import to use DataHandler. We didn't invest too much time as budgets are low. Still the bugs are covered with tests and fixed. Relates: #10782 --- Classes/Domain/Model/Event.php | 43 +++------- .../Service/DestinationDataImportService.php | 42 +++++++--- .../CategoriesAssignment.php | 12 ++- .../DataHandler.php | 68 +++++++++++++++ .../DataHandler/Assignment.php | 82 +++++++++++++++++++ Documentation/Changelog/3.5.2.rst | 15 ++++ .../ImportsFeaturesAddsNewFeatures.csv | 6 +- .../Fixtures/Database/ExistingFeatures.php | 22 +++++ .../Database/FeaturesImportConfiguration.php | 2 +- .../ImportsFeaturesTest.php | 19 +++++ Tests/Unit/Domain/Model/EventTest.php | 4 +- phpstan-baseline.neon | 10 +++ 12 files changed, 276 insertions(+), 49 deletions(-) create mode 100644 Classes/Service/DestinationDataImportService/DataHandler.php create mode 100644 Classes/Service/DestinationDataImportService/DataHandler/Assignment.php create mode 100644 Tests/Functional/Import/DestinationDataTest/Fixtures/Database/ExistingFeatures.php diff --git a/Classes/Domain/Model/Event.php b/Classes/Domain/Model/Event.php index 721a80d..c072e21 100644 --- a/Classes/Domain/Model/Event.php +++ b/Classes/Domain/Model/Event.php @@ -426,21 +426,7 @@ public function addCategory(Category $category): void */ public function getCategories(): array { - $categories = $this->categories->toArray(); - - usort($categories, function (Category $catA, Category $catB) { - return $catA->getSorting() <=> $catB->getSorting(); - }); - - return $categories; - } - - /** - * @param ObjectStorage $categories - */ - public function setCategories(ObjectStorage $categories): void - { - $this->categories = $categories; + return $this->getSortedCategory($this->categories); } /** @@ -448,21 +434,7 @@ public function setCategories(ObjectStorage $categories): void */ public function getFeatures(): array { - $features = $this->features->toArray(); - - usort($features, function (Category $catA, Category $catB) { - return $catA->getSorting() <=> $catB->getSorting(); - }); - - return $features; - } - - /** - * @param ObjectStorage $features - */ - public function setFeatures(ObjectStorage $features): void - { - $this->features = $features; + return $this->getSortedCategory($this->features); } public function setLanguageUid(int $languageUid): void @@ -489,4 +461,15 @@ public function setSourceUrl(string $url): void { $this->sourceUrl = $url; } + + private function getSortedCategory(ObjectStorage $categories): array + { + $categories = $categories->toArray(); + + usort($categories, function (Category $catA, Category $catB) { + return $catA->getSorting() <=> $catB->getSorting(); + }); + + return $categories; + } } diff --git a/Classes/Service/DestinationDataImportService.php b/Classes/Service/DestinationDataImportService.php index 32d615e..d54305b 100644 --- a/Classes/Service/DestinationDataImportService.php +++ b/Classes/Service/DestinationDataImportService.php @@ -22,6 +22,8 @@ use Wrm\Events\Service\DestinationDataImportService\CategoriesAssignment; use Wrm\Events\Service\DestinationDataImportService\CategoriesAssignment\Import as CategoryImport; use Wrm\Events\Service\DestinationDataImportService\DataFetcher; +use Wrm\Events\Service\DestinationDataImportService\DataHandler; +use Wrm\Events\Service\DestinationDataImportService\DataHandler\Assignment; use Wrm\Events\Service\DestinationDataImportService\DatesFactory; use Wrm\Events\Service\DestinationDataImportService\Events\CategoriesAssignEvent; use Wrm\Events\Service\DestinationDataImportService\Events\EventImportEvent; @@ -111,6 +113,11 @@ class DestinationDataImportService */ private $cacheManager; + /** + * @var DataHandler + */ + private $dataHandler; + /** * @var EventDispatcher */ @@ -131,6 +138,7 @@ class DestinationDataImportService * @param LocationAssignment $locationAssignment * @param Slugger $slugger * @param CacheManager $cacheManager + * @param DataHandler $dataHandler * @param EventDispatcher $eventDispatcher */ public function __construct( @@ -147,6 +155,7 @@ public function __construct( LocationAssignment $locationAssignment, Slugger $slugger, CacheManager $cacheManager, + DataHandler $dataHandler, EventDispatcher $eventDispatcher ) { $this->eventRepository = $eventRepository; @@ -162,6 +171,7 @@ public function __construct( $this->locationAssignment = $locationAssignment; $this->slugger = $slugger; $this->cacheManager = $cacheManager; + $this->dataHandler = $dataHandler; $this->eventDispatcher = $eventDispatcher; } @@ -236,16 +246,6 @@ public function processData(array $data): int $this->locationAssignment->getLocation($event) ); - // Set Categories - if ($event['categories'] ?? false) { - $this->setCategories($event['categories']); - } - - // Set Features - if ($event['features']) { - $this->setFeatures($event['features']); - } - // Set Organizer if ($event['addresses'] ?? false) { $this->setOrganizer($event['addresses']); @@ -291,6 +291,16 @@ public function processData(array $data): int $this->logger->info('Persist database'); $this->eventRepository->update($this->tmpCurrentEvent); $this->persistenceManager->persistAll(); + + // Apply changes via DataHandler (The new way) + $this->logger->info('Apply changes via DataHandler'); + if ($event['categories'] ?? false) { + $this->setCategories($event['categories']); + } + if ($event['features']) { + $this->setFeatures($event['features']); + } + $this->logger->info('Update slugs'); $this->slugger->update('tx_events_domain_model_event'); $this->slugger->update('tx_events_domain_model_date'); @@ -317,7 +327,11 @@ private function setCategories(array $categories): void ); $this->eventDispatcher->dispatch($event); - $this->tmpCurrentEvent->setCategories($event->getCategories()); + $this->dataHandler->storeAssignments(new Assignment( + $this->tmpCurrentEvent->getUid(), + 'categories', + $event->getCategories()->toArray() + )); } private function setFeatures(array $features): void @@ -329,7 +343,11 @@ private function setFeatures(array $features): void true )); - $this->tmpCurrentEvent->setFeatures($features); + $this->dataHandler->storeAssignments(new Assignment( + $this->tmpCurrentEvent->getUid(), + 'features', + $features->toArray() + )); } private function setDates( diff --git a/Classes/Service/DestinationDataImportService/CategoriesAssignment.php b/Classes/Service/DestinationDataImportService/CategoriesAssignment.php index ab290b1..4ddf8ed 100644 --- a/Classes/Service/DestinationDataImportService/CategoriesAssignment.php +++ b/Classes/Service/DestinationDataImportService/CategoriesAssignment.php @@ -2,6 +2,7 @@ namespace Wrm\Events\Service\DestinationDataImportService; +use TYPO3\CMS\Extbase\Persistence\Generic\PersistenceManager; use TYPO3\CMS\Extbase\Persistence\ObjectStorage; use Wrm\Events\Domain\Model\Category; use Wrm\Events\Domain\Repository\CategoryRepository; @@ -20,10 +21,17 @@ class CategoriesAssignment */ private $repository; + /** + * @var PersistenceManager + */ + private $persistenceManager; + public function __construct( - CategoryRepository $repository + CategoryRepository $repository, + PersistenceManager $persistenceManager ) { $this->repository = $repository; + $this->persistenceManager = $persistenceManager; } /** @@ -58,6 +66,8 @@ public function getCategories( $categories->attach($category); } + $this->persistenceManager->persistAll(); + return $categories; } } diff --git a/Classes/Service/DestinationDataImportService/DataHandler.php b/Classes/Service/DestinationDataImportService/DataHandler.php new file mode 100644 index 0000000..36e53f5 --- /dev/null +++ b/Classes/Service/DestinationDataImportService/DataHandler.php @@ -0,0 +1,68 @@ + + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301, USA. + */ + +namespace Wrm\Events\Service\DestinationDataImportService; + +use TYPO3\CMS\Core\DataHandling\DataHandler as Typo3DataHandler; +use TYPO3\CMS\Core\Log\Logger; +use TYPO3\CMS\Core\Log\LogManager; +use TYPO3\CMS\Core\Utility\GeneralUtility; +use Wrm\Events\Service\DestinationDataImportService\DataHandler\Assignment; + +final class DataHandler +{ + /** + * @var Logger + */ + private $logger; + + public function __construct( + LogManager $logManager + ) { + $this->logger = $logManager->getLogger(__CLASS__); + } + + public function storeAssignments( + Assignment $assignment + ): void { + $data = [ + 'tx_events_domain_model_event' => [ + $assignment->getUid() => [ + $assignment->getColumnName() => implode(',', $assignment->getUids()), + ], + ], + ]; + + $this->logger->debug('Import assignment.', $data); + $dataHandler = GeneralUtility::makeInstance(Typo3DataHandler::class); + $dataHandler->start($data, []); + $dataHandler->process_datamap(); + + if ($dataHandler->errorLog !== []) { + $this->logger->error('Error during import of assignments.', [ + 'assignment' => $assignment, + 'errors' => $dataHandler->errorLog, + ]); + } + } +} diff --git a/Classes/Service/DestinationDataImportService/DataHandler/Assignment.php b/Classes/Service/DestinationDataImportService/DataHandler/Assignment.php new file mode 100644 index 0000000..205d68b --- /dev/null +++ b/Classes/Service/DestinationDataImportService/DataHandler/Assignment.php @@ -0,0 +1,82 @@ + + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301, USA. + */ + +namespace Wrm\Events\Service\DestinationDataImportService\DataHandler; + +use InvalidArgumentException; +use TYPO3\CMS\Extbase\DomainObject\AbstractDomainObject; + +final class Assignment +{ + /** + * @var int + */ + private $uid; + + /** + * @var string + */ + private $columnName; + + /** + * @var int[] + */ + private $uids; + + /** + * @param AbstractDomainObject[] $assignments + */ + public function __construct( + int $uid, + string $columnName, + array $assignments + ) { + $this->uid = $uid; + $this->columnName = $columnName; + $this->uids = array_map(static function (AbstractDomainObject $model): int { + $uid = $model->getUid(); + if (is_int($uid) === false) { + throw new InvalidArgumentException('Only object with uid supported.', 1698936965); + } + return $uid; + }, $assignments); + } + + public function getUid(): int + { + return $this->uid; + } + + public function getColumnName(): string + { + return $this->columnName; + } + + /** + * @return int[] + */ + public function getUids(): array + { + return $this->uids; + } +} diff --git a/Documentation/Changelog/3.5.2.rst b/Documentation/Changelog/3.5.2.rst index 09e265b..579597e 100644 --- a/Documentation/Changelog/3.5.2.rst +++ b/Documentation/Changelog/3.5.2.rst @@ -19,6 +19,21 @@ Fixes An unexpected value (3) was submitted on selection. We now migrated the property to look and act the same as hidden input. +* Fix broken assignment of features and categories. + The import contained a bug that lead to summing up relations between events and + categories and features. + The same relations would be added over and over again with every new import. + This leads to performance issues when editing or storing events through TYPO3 + backend. + The import is adjusted to no longer use Extbase but DataHandler for this task. + This circumvents the issues introduced by Extbase. + This is the first step to migration of import to be fully using DataHandler + + doctrine DBAL in the future. + One drawback is the slowdown of the import. + But we don't expect people to run it that often. + Also a working import with proper logging (history, permissions, etc.) seems better + than a broken but fast import. + Tasks ----- diff --git a/Tests/Functional/Import/DestinationDataTest/Assertions/ImportsFeaturesAddsNewFeatures.csv b/Tests/Functional/Import/DestinationDataTest/Assertions/ImportsFeaturesAddsNewFeatures.csv index 36a05a3..5b3e33f 100644 --- a/Tests/Functional/Import/DestinationDataTest/Assertions/ImportsFeaturesAddsNewFeatures.csv +++ b/Tests/Functional/Import/DestinationDataTest/Assertions/ImportsFeaturesAddsNewFeatures.csv @@ -1,14 +1,14 @@ "tx_events_domain_model_event",,,,,, ,"uid","pid","features",,, -,1,2,1,,, -,2,2,1,,, +,1,2,6,,, +,2,2,4,,, ,3,2,0,,, "sys_category",,,,,, ,"uid","pid","title","parent","hidden", ,1,2,"Top Category",0,0, ,2,2,"Event Category Parent",1,0, ,4,2,"Event Feature Parent",1,0, -,5,3,"vorhandenes Feature",4,0, +,5,3,"vorhandenes Feature",4,1, ,6,3,"Barrierefrei",4,1, ,7,3,"Zielgruppe Jugendliche",4,1, ,8,3,"Karten an der Abendkasse",4,1, diff --git a/Tests/Functional/Import/DestinationDataTest/Fixtures/Database/ExistingFeatures.php b/Tests/Functional/Import/DestinationDataTest/Fixtures/Database/ExistingFeatures.php new file mode 100644 index 0000000..6a43fa2 --- /dev/null +++ b/Tests/Functional/Import/DestinationDataTest/Fixtures/Database/ExistingFeatures.php @@ -0,0 +1,22 @@ + [ + [ + 'uid' => 1, + 'pid' => 2, + 'global_id' => 'e_100347853', + 'features' => 1, + ], + ], + 'sys_category_record_mm' => [ + [ + 'uid_local' => 5, + 'uid_foreign' => 1, + 'tablenames' => 'tx_events_domain_model_event', + 'fieldname' => 'features', + 'sorting' => 0, + 'sorting_foreign' => 1, + ], + ], +]; diff --git a/Tests/Functional/Import/DestinationDataTest/Fixtures/Database/FeaturesImportConfiguration.php b/Tests/Functional/Import/DestinationDataTest/Fixtures/Database/FeaturesImportConfiguration.php index adf7472..ecde423 100644 --- a/Tests/Functional/Import/DestinationDataTest/Fixtures/Database/FeaturesImportConfiguration.php +++ b/Tests/Functional/Import/DestinationDataTest/Fixtures/Database/FeaturesImportConfiguration.php @@ -42,7 +42,7 @@ 'pid' => '3', 'title' => 'vorhandenes Feature', 'parent' => '4', - 'hidden' => '0', + 'hidden' => '1', ], ], ]; diff --git a/Tests/Functional/Import/DestinationDataTest/ImportsFeaturesTest.php b/Tests/Functional/Import/DestinationDataTest/ImportsFeaturesTest.php index 932ae8a..a2b037f 100644 --- a/Tests/Functional/Import/DestinationDataTest/ImportsFeaturesTest.php +++ b/Tests/Functional/Import/DestinationDataTest/ImportsFeaturesTest.php @@ -27,4 +27,23 @@ public function addsNewFeatures(): void $this->assertCSVDataSet('EXT:events/Tests/Functional/Import/DestinationDataTest/Assertions/ImportsFeaturesAddsNewFeatures.csv'); $this->assertEmptyLog(); } + + /** + * @test + */ + public function addsNewFeaturesToExistingOnes(): void + { + $this->setUpConfiguration([ + 'restUrl = https://example.com/some-path/', + ]); + $this->importPHPDataSet(__DIR__ . '/Fixtures/Database/FeaturesImportConfiguration.php'); + $this->importPHPDataSet(__DIR__ . '/Fixtures/Database/ExistingFeatures.php'); + $this->setUpResponses([ + new Response(200, [], file_get_contents(__DIR__ . '/Fixtures/ResponseWithFeatures.json') ?: ''), + ]); + $tester = $this->executeCommand(); + + $this->assertCSVDataSet('EXT:events/Tests/Functional/Import/DestinationDataTest/Assertions/ImportsFeaturesAddsNewFeatures.csv'); + $this->assertEmptyLog(); + } } diff --git a/Tests/Unit/Domain/Model/EventTest.php b/Tests/Unit/Domain/Model/EventTest.php index 924a3a5..fd72c32 100644 --- a/Tests/Unit/Domain/Model/EventTest.php +++ b/Tests/Unit/Domain/Model/EventTest.php @@ -42,7 +42,7 @@ public function returnsSortedFeatures(): void $storage->attach($feature2); $subject = new Event(); - $subject->setFeatures($storage); + $subject->_setProperty('features', $storage); self::assertSame([ $feature2, @@ -56,7 +56,7 @@ public function returnsSortedFeatures(): void public function returnsEmptyFeaturesStorage(): void { $subject = new Event(); - $subject->setFeatures(new ObjectStorage()); + $subject->_setProperty('features', new ObjectStorage()); self::assertSame([], $subject->getFeatures()); } diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 7db735c..e447268 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -90,6 +90,16 @@ parameters: count: 1 path: Classes/Service/Cleanup/Files.php + - + message: "#^Parameter \\#1 \\$uid of class Wrm\\\\Events\\\\Service\\\\DestinationDataImportService\\\\DataHandler\\\\Assignment constructor expects int, int\\|null given\\.$#" + count: 2 + path: Classes/Service/DestinationDataImportService.php + + - + message: "#^Parameter \\#3 \\$assignments of class Wrm\\\\Events\\\\Service\\\\DestinationDataImportService\\\\DataHandler\\\\Assignment constructor expects array\\, array\\ given\\.$#" + count: 2 + path: Classes/Service/DestinationDataImportService.php + - message: "#^Method Wrm\\\\Events\\\\Service\\\\DestinationDataImportService\\\\CategoriesAssignment\\:\\:getCategories\\(\\) should return TYPO3\\\\CMS\\\\Extbase\\\\Persistence\\\\ObjectStorage\\ but returns TYPO3\\\\CMS\\\\Extbase\\\\Persistence\\\\ObjectStorage\\\\.$#" count: 2