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

@@ -426,21 +426,7 @@ class Event extends AbstractEntity
      */
     public function getCategories(): array
     {
-        $categories = $this->categories->toArray();
+        return $this->getSortedCategory($this->categories);
-
-        usort($categories, function (Category $catA, Category $catB) {
-            return $catA->getSorting() <=> $catB->getSorting();
-        });
-
-        return $categories;
-    }
-
-    /**
-     * @param ObjectStorage<Category> $categories
-     */
-    public function setCategories(ObjectStorage $categories): void
-    {
-        $this->categories = $categories;
     }
 
     /**
@@ -448,21 +434,7 @@ class Event extends AbstractEntity
      */
     public function getFeatures(): array
     {
-        $features = $this->features->toArray();
+        return $this->getSortedCategory($this->features);
-
-        usort($features, function (Category $catA, Category $catB) {
-            return $catA->getSorting() <=> $catB->getSorting();
-        });
-
-        return $features;
-    }
-
-    /**
-     * @param ObjectStorage<Category> $features
-     */
-    public function setFeatures(ObjectStorage $features): void
-    {
-        $this->features = $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;
+    }
 }

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(

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;
     }
 }

Classes/Service/DestinationDataImportService/DataHandler.php

@@ -0,0 +1,68 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * Copyright (C) 2023 Daniel Siepmann <coding@daniel-siepmann.de>
+ *
+ * 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,
+            ]);
+        }
+    }
+}

Classes/Service/DestinationDataImportService/DataHandler/Assignment.php

@@ -0,0 +1,82 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * Copyright (C) 2023 Daniel Siepmann <coding@daniel-siepmann.de>
+ *
+ * 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;
+    }
+}

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
 -----
 

Tests/Functional/Import/DestinationDataTest/Assertions/ImportsFeaturesAddsNewFeatures.csv

@@ -1,14 +1,14 @@
 "tx_events_domain_model_event",,,,,,
 ,"uid","pid","features",,,
-,1,2,1,,,
+,1,2,6,,,
-,2,2,1,,,
+,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,

Tests/Functional/Import/DestinationDataTest/Fixtures/Database/ExistingFeatures.php

@@ -0,0 +1,22 @@
+<?php
+
+return [
+    'tx_events_domain_model_event' => [
+        [
+            '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,
+        ],
+    ],
+];

Tests/Functional/Import/DestinationDataTest/Fixtures/Database/FeaturesImportConfiguration.php

@@ -42,7 +42,7 @@ return [
             'pid' => '3',
             'title' => 'vorhandenes Feature',
             'parent' => '4',
-            'hidden' => '0',
+            'hidden' => '1',
         ],
     ],
 ];

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();
+    }
 }

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());
     }

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\\<TYPO3\\\\CMS\\\\Extbase\\\\DomainObject\\\\AbstractDomainObject\\>, array\\<int, TEntity\\> given\\.$#"
+			count: 2
+			path: Classes/Service/DestinationDataImportService.php
+
 		-
 			message: "#^Method Wrm\\\\Events\\\\Service\\\\DestinationDataImportService\\\\CategoriesAssignment\\:\\:getCategories\\(\\) should return TYPO3\\\\CMS\\\\Extbase\\\\Persistence\\\\ObjectStorage\\<Wrm\\\\Events\\\\Domain\\\\Model\\\\Category\\> but returns TYPO3\\\\CMS\\\\Extbase\\\\Persistence\\\\ObjectStorage\\<mixed\\>\\.$#"
 			count: 2