From ab75902a9576451b1c96dc5f41d235e6762e2f21 Mon Sep 17 00:00:00 2001 From: Daniel Siepmann Date: Mon, 6 Nov 2023 08:44:03 +0100 Subject: [PATCH] Fix broken assignment of features and categories (#41) 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 @@ class Event extends AbstractEntity */ 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 @@ class Event extends AbstractEntity */ 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 @@ class Event extends AbstractEntity { $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\Domain\Repository\OrganizerRepository; 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 @@ class DestinationDataImportService LocationAssignment $locationAssignment, Slugger $slugger, CacheManager $cacheManager, + DataHandler $dataHandler, EventDispatcher $eventDispatcher ) { $this->eventRepository = $eventRepository; @@ -162,6 +171,7 @@ class DestinationDataImportService $this->locationAssignment = $locationAssignment; $this->slugger = $slugger; $this->cacheManager = $cacheManager; + $this->dataHandler = $dataHandler; $this->eventDispatcher = $eventDispatcher; } @@ -236,16 +246,6 @@ class DestinationDataImportService $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 @@ class DestinationDataImportService $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 @@ class DestinationDataImportService ); $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 @@ class DestinationDataImportService 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 @@ class CategoriesAssignment $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 @@ return [ '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 @@ class ImportsFeaturesTest extends AbstractTest $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 @@ class EventTest extends TestCase $storage->attach($feature2); $subject = new Event(); - $subject->setFeatures($storage); + $subject->_setProperty('features', $storage); self::assertSame([ $feature2, @@ -56,7 +56,7 @@ class EventTest extends TestCase 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