From 1da563c51122eb882bc1a665d52a1fc91a41153f Mon Sep 17 00:00:00 2001 From: Daniel Gohlke Date: Tue, 25 Jun 2024 10:26:13 +0200 Subject: [PATCH 1/4] [BUGFIX] Fix integrity sorting in page with nested containers Relates: #458 --- Classes/Domain/Service/ContainerService.php | 18 +++++++++++++----- Classes/Integrity/SortingInPage.php | 2 +- ...er_with_nested_changed_children_sorting.csv | 10 ++++++++++ .../Functional/Integrity/SortingInPageTest.php | 13 +++++++++++++ 4 files changed, 37 insertions(+), 6 deletions(-) create mode 100644 Tests/Functional/Integrity/Fixtures/SortingInPage/container_is_sorted_before_child_of_previous_container_with_nested_changed_children_sorting.csv diff --git a/Classes/Domain/Service/ContainerService.php b/Classes/Domain/Service/ContainerService.php index 58687908..1256cb34 100644 --- a/Classes/Domain/Service/ContainerService.php +++ b/Classes/Domain/Service/ContainerService.php @@ -53,18 +53,26 @@ public function getNewContentElementAtTopTargetInColumn(Container $container, in return $target; } - public function getAfterContainerElementTarget(Container $container): int + public function getAfterContainerRecord(Container $container): array { - $target = -$container->getUid(); $childRecords = $container->getChildRecords(); if (empty($childRecords)) { - return $target; + return $container->getContainerRecord(); } + $lastChild = array_pop($childRecords); if (!$this->tcaRegistry->isContainerElement($lastChild['CType'])) { - return -(int)$lastChild['uid']; + return $lastChild; } + $container = $this->containerFactory->buildContainer((int)$lastChild['uid']); - return $this->getAfterContainerElementTarget($container); + return $this->getAfterContainerRecord($container); + } + + public function getAfterContainerElementTarget(Container $container): int + { + $target = $this->getAfterContainerRecord($container); + + return -$target['uid']; } } diff --git a/Classes/Integrity/SortingInPage.php b/Classes/Integrity/SortingInPage.php index 39f48868..18520073 100644 --- a/Classes/Integrity/SortingInPage.php +++ b/Classes/Integrity/SortingInPage.php @@ -77,7 +77,7 @@ public function run(bool $dryRun = true, bool $enableLogging = false, ?int $pid if (empty($children)) { $sorting = $record['sorting']; } else { - $lastChild = array_pop($children); + $lastChild = $this->containerService->getAfterContainerRecord($container); $sorting = $lastChild['sorting']; if ($prevChild === null || $prevContainer === null) { diff --git a/Tests/Functional/Integrity/Fixtures/SortingInPage/container_is_sorted_before_child_of_previous_container_with_nested_changed_children_sorting.csv b/Tests/Functional/Integrity/Fixtures/SortingInPage/container_is_sorted_before_child_of_previous_container_with_nested_changed_children_sorting.csv new file mode 100644 index 00000000..7e11d2aa --- /dev/null +++ b/Tests/Functional/Integrity/Fixtures/SortingInPage/container_is_sorted_before_child_of_previous_container_with_nested_changed_children_sorting.csv @@ -0,0 +1,10 @@ +"pages" +,"uid","pid" +,1,0 +"tt_content" +,"uid","pid","colPos","CType","sorting","tx_container_parent" +,1,1,0,b13-2cols-with-header-container,1, +,2,1,0,b13-2cols-with-header-container,2,1 +,3,1,202,,5,2 +,4,1,0,"b13-2cols-with-header-container",3, +,5,1,202,,4,3 diff --git a/Tests/Functional/Integrity/SortingInPageTest.php b/Tests/Functional/Integrity/SortingInPageTest.php index 406f22a9..b7cee097 100644 --- a/Tests/Functional/Integrity/SortingInPageTest.php +++ b/Tests/Functional/Integrity/SortingInPageTest.php @@ -77,6 +77,19 @@ public function containerIsSortedAfterChildOfPreviousContainerWithChangedChildre self::assertTrue($rows[2]['sorting'] > $rows[3]['sorting'], 'container should be sorted after child of previous container'); } + /** + * @test + */ + public function containerIsSortedAfterChildOfPreviousContainerWithNestedChangedChildrenSorting(): void + { + $this->importCSVDataSet(__DIR__ . '/Fixtures/SortingInPage/container_is_sorted_before_child_of_previous_container_with_nested_changed_children_sorting.csv'); + $errors = $this->sorting->run(false); + self::assertTrue(count($errors) === 1, 'should get one error'); + $rows = $this->getContentsByUid(); + self::assertTrue($rows[2]['sorting'] > $rows[5]['sorting'], 'container should be sorted after last nested child of previous container'); + self::assertTrue($rows[3]['sorting'] > $rows[2]['sorting'], 'child should be sorted after its own parent container after resorting'); + } + /** * @test */ From 02d27998018f2d298a23823cf1b524c6e33e82ef Mon Sep 17 00:00:00 2001 From: Daniel Gohlke Date: Tue, 25 Jun 2024 15:59:47 +0200 Subject: [PATCH 2/4] [TASK] Refactor code $container->getChildRecords() only used for the condition. We can remove this, because the new getAfterContainerRecord() returns the container itself if ce has no children. We than can check if the uids are equal. This improvement allows to extract the $sorting from the condition so we only have the else block. --- Classes/Integrity/SortingInPage.php | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/Classes/Integrity/SortingInPage.php b/Classes/Integrity/SortingInPage.php index 18520073..efa98d10 100644 --- a/Classes/Integrity/SortingInPage.php +++ b/Classes/Integrity/SortingInPage.php @@ -73,13 +73,10 @@ public function run(bool $dryRun = true, bool $enableLogging = false, ?int $pid foreach ($recordsPerPageAndColPos as $record) { if (in_array($record['CType'], $cTypes, true)) { $container = $this->containerFactory->buildContainer($record['uid']); - $children = $container->getChildRecords(); - if (empty($children)) { - $sorting = $record['sorting']; - } else { - $lastChild = $this->containerService->getAfterContainerRecord($container); - $sorting = $lastChild['sorting']; + $lastChild = $this->containerService->getAfterContainerRecord($container); + $sorting = $lastChild['sorting']; + if ($record['uid'] !== $lastChild['uid']) { if ($prevChild === null || $prevContainer === null) { $prevChild = $lastChild; $prevContainer = $container; From 49d2cf5bfcaba85c3ee49863c655ae1f58a20196 Mon Sep 17 00:00:00 2001 From: Daniel Gohlke Date: Thu, 27 Jun 2024 08:47:56 +0200 Subject: [PATCH 3/4] [BUGFIX] Fix provided test setup --- ...revious_container_with_nested_changed_children_sorting.csv | 4 ++-- Tests/Functional/Integrity/SortingInPageTest.php | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Tests/Functional/Integrity/Fixtures/SortingInPage/container_is_sorted_before_child_of_previous_container_with_nested_changed_children_sorting.csv b/Tests/Functional/Integrity/Fixtures/SortingInPage/container_is_sorted_before_child_of_previous_container_with_nested_changed_children_sorting.csv index 7e11d2aa..08971172 100644 --- a/Tests/Functional/Integrity/Fixtures/SortingInPage/container_is_sorted_before_child_of_previous_container_with_nested_changed_children_sorting.csv +++ b/Tests/Functional/Integrity/Fixtures/SortingInPage/container_is_sorted_before_child_of_previous_container_with_nested_changed_children_sorting.csv @@ -4,7 +4,7 @@ "tt_content" ,"uid","pid","colPos","CType","sorting","tx_container_parent" ,1,1,0,b13-2cols-with-header-container,1, -,2,1,0,b13-2cols-with-header-container,2,1 +,2,1,202,b13-2cols-with-header-container,2,1 ,3,1,202,,5,2 ,4,1,0,"b13-2cols-with-header-container",3, -,5,1,202,,4,3 +,5,1,202,,4,4 diff --git a/Tests/Functional/Integrity/SortingInPageTest.php b/Tests/Functional/Integrity/SortingInPageTest.php index b7cee097..4c3798e8 100644 --- a/Tests/Functional/Integrity/SortingInPageTest.php +++ b/Tests/Functional/Integrity/SortingInPageTest.php @@ -86,8 +86,8 @@ public function containerIsSortedAfterChildOfPreviousContainerWithNestedChangedC $errors = $this->sorting->run(false); self::assertTrue(count($errors) === 1, 'should get one error'); $rows = $this->getContentsByUid(); - self::assertTrue($rows[2]['sorting'] > $rows[5]['sorting'], 'container should be sorted after last nested child of previous container'); - self::assertTrue($rows[3]['sorting'] > $rows[2]['sorting'], 'child should be sorted after its own parent container after resorting'); + self::assertTrue($rows[4]['sorting'] > $rows[3]['sorting'], 'container should be sorted after last nested child of previous container'); + self::assertTrue($rows[5]['sorting'] > $rows[4]['sorting'], 'child should be sorted after its own parent container after resorting'); } /** From 85396448d27d38cec2912696b407cc6d9b47adc7 Mon Sep 17 00:00:00 2001 From: Daniel Gohlke Date: Thu, 27 Jun 2024 07:04:20 +0200 Subject: [PATCH 4/4] [BUGFIX] Fix sorting for translated content in free mode --- Classes/Command/SortingInPageCommand.php | 7 ++- Classes/Integrity/Database.php | 14 +++-- Classes/Integrity/SortingInPage.php | 4 +- ..._previous_container_on_translated_page.csv | 15 ++++++ .../Integrity/SortingInPageTest.php | 51 +++++++++++++++++++ 5 files changed, 84 insertions(+), 7 deletions(-) create mode 100644 Tests/Functional/Integrity/Fixtures/SortingInPage/container_is_sorted_before_child_of_previous_container_on_translated_page.csv diff --git a/Classes/Command/SortingInPageCommand.php b/Classes/Command/SortingInPageCommand.php index acc736c8..f3a960ea 100644 --- a/Classes/Command/SortingInPageCommand.php +++ b/Classes/Command/SortingInPageCommand.php @@ -32,6 +32,7 @@ class SortingInPageCommand extends Command protected function configure() { $this->addArgument('pid', InputArgument::OPTIONAL, 'limit to this pid', 0); + $this->addArgument('languageId', InputArgument::OPTIONAL, 'limit to this languageId', 0); $this->addOption('apply', null, InputOption::VALUE_NONE, 'apply migration'); $this->addOption( 'enable-logging', @@ -51,13 +52,17 @@ public function execute(InputInterface $input, OutputInterface $output): int { $dryrun = $input->getOption('apply') !== true; $pid = (int)$input->getArgument('pid'); + if ($input->getArgument('languageId') !== 'all') { + $languageId = (int)$input->getArgument('languageId'); + } Bootstrap::initializeBackendAuthentication(); $GLOBALS['LANG'] = GeneralUtility::makeInstance(LanguageServiceFactory::class)->createFromUserPreferences($GLOBALS['BE_USER']); $errors = $this->sorting->run( $dryrun, $input->getOption('enable-logging'), - $pid + $pid, + $languageId, ); foreach ($errors as $error) { $output->writeln($error); diff --git a/Classes/Integrity/Database.php b/Classes/Integrity/Database.php index 22a905ca..99f19de5 100644 --- a/Classes/Integrity/Database.php +++ b/Classes/Integrity/Database.php @@ -106,7 +106,7 @@ public function getChildrenByContainerAndColPos(int $containerId, int $colPos, i return $rows; } - public function getNonContainerChildrenPerColPos(array $containerUsedColPosArray, ?int $pid = null): array + public function getNonContainerChildrenPerColPos(array $containerUsedColPosArray, ?int $pid = null, ?int $languageId = null): array { $queryBuilder = $this->getQueryBuilder(); $stm = $queryBuilder @@ -116,12 +116,18 @@ public function getNonContainerChildrenPerColPos(array $containerUsedColPosArray $queryBuilder->expr()->notIn( 'colPos', $queryBuilder->createNamedParameter($containerUsedColPosArray, Connection::PARAM_INT_ARRAY) - ), + ) + ); + + if (!is_null($languageId)) { + $stm->andWhere( $queryBuilder->expr()->eq( 'sys_language_uid', - $queryBuilder->createNamedParameter(0, Connection::PARAM_INT) + $queryBuilder->createNamedParameter($languageId, Connection::PARAM_INT) ) ); + } + if (!empty($pid)) { $stm->andWhere( $queryBuilder->expr()->eq( @@ -136,7 +142,7 @@ public function getNonContainerChildrenPerColPos(array $containerUsedColPosArray $results = $stm->executeQuery()->fetchAllAssociative(); $rows = []; foreach ($results as $result) { - $key = $result['pid'] . '-' . $result['colPos']; + $key = $result['pid'] . '-' . $result['sys_language_uid'] . '-' . $result['colPos']; if (!isset($rows[$key])) { $rows[$key] = []; } diff --git a/Classes/Integrity/SortingInPage.php b/Classes/Integrity/SortingInPage.php index efa98d10..22d3f048 100644 --- a/Classes/Integrity/SortingInPage.php +++ b/Classes/Integrity/SortingInPage.php @@ -52,7 +52,7 @@ public function __construct(Database $database, Registry $tcaRegistry, Container $this->containerService = $containerService; } - public function run(bool $dryRun = true, bool $enableLogging = false, ?int $pid = null): array + public function run(bool $dryRun = true, bool $enableLogging = false, ?int $pid = null, ?int $languageId = null): array { $this->unsetContentDefenderConfiguration(); $dataHandler = GeneralUtility::makeInstance(DataHandler::class); @@ -65,7 +65,7 @@ public function run(bool $dryRun = true, bool $enableLogging = false, ?int $pid $containerUsedColPosArray[] = $column['colPos']; } } - $rows = $this->database->getNonContainerChildrenPerColPos($containerUsedColPosArray, $pid); + $rows = $this->database->getNonContainerChildrenPerColPos($containerUsedColPosArray, $pid, $languageId); foreach ($rows as $recordsPerPageAndColPos) { $prevSorting = 0; $prevContainer = null; diff --git a/Tests/Functional/Integrity/Fixtures/SortingInPage/container_is_sorted_before_child_of_previous_container_on_translated_page.csv b/Tests/Functional/Integrity/Fixtures/SortingInPage/container_is_sorted_before_child_of_previous_container_on_translated_page.csv new file mode 100644 index 00000000..148dbfe6 --- /dev/null +++ b/Tests/Functional/Integrity/Fixtures/SortingInPage/container_is_sorted_before_child_of_previous_container_on_translated_page.csv @@ -0,0 +1,15 @@ +"pages" +,"uid","pid","sys_language_uid" +,1,0,0 +,2,1,1 +,3,1,2 +"tt_content" +,"uid","pid","colPos","CType","sorting","tx_container_parent","sys_language_uid" +,1,2,0,"b13-2cols-with-header-container",1,,1 +,2,2,0,"b13-2cols-with-header-container",2,,1 +,3,2,202,,4,1,1 +,4,2,202,,3,2,1 +,5,3,0,"b13-2cols-with-header-container",5,,2 +,6,3,0,"b13-2cols-with-header-container",6,,2 +,7,3,202,,8,5,2 +,8,3,202,,7,6,2 diff --git a/Tests/Functional/Integrity/SortingInPageTest.php b/Tests/Functional/Integrity/SortingInPageTest.php index 4c3798e8..38cbd5ee 100644 --- a/Tests/Functional/Integrity/SortingInPageTest.php +++ b/Tests/Functional/Integrity/SortingInPageTest.php @@ -65,6 +65,57 @@ public function containerIsSortedAfterChildOfPreviousContainer(): void self::assertTrue($rows[2]['sorting'] > $rows[3]['sorting'], 'container should be sorted after child of previous container'); } + public static function getPossibleTranslations(): iterable + { + yield 'with language id 1' => [ + 'languageId' => 1, + 'expected' => [ + 'errors' => 1, + 'sortings' => [ + 2 => 3, + ] + ] + ]; + yield 'with language id 2' => [ + 'languageId' => 2, + 'expected' => [ + 'errors' => 1, + 'sortings' => [ + 6 => 7, + ] + ] + ]; + yield 'with all languages' => [ + 'languageId' => null, + 'expected' => [ + 'errors' => 2, + 'sortings' => [ + 2 => 3, + 6 => 7, + ] + ] + ]; + } + + /** + * @test + * @dataProvider getPossibleTranslations + */ + public function containerIsSortedAfterChildOfPreviousContainerOnTranslatedPage(?int $languageId = null, array $expected): void + { + $this->importCSVDataSet(__DIR__ . '/Fixtures/SortingInPage/container_is_sorted_before_child_of_previous_container_on_translated_page.csv'); + $errors = $this->sorting->run(false, false, null, 0); + self::assertTrue(count($errors) === 0, 'different number of errors for default language'); + + $errors = $this->sorting->run(false, false, null, $languageId); + self::assertTrue(count($errors) === $expected['errors'], 'different number of errors for given language'); + + $rows = $this->getContentsByUid(); + foreach ($expected['sortings'] as $before => $after) { + self::assertTrue($rows[$before]['sorting'] > $rows[$after]['sorting'], 'container should be sorted after child of previous container'); + } + } + /** * @test */