From 289457c302e6e5211d2e3400aa5016c607831da1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dennis=20Br=C3=A4ndle?= Date: Fri, 3 Jul 2026 13:22:07 +0200 Subject: [PATCH 1/2] [BUGFIX] Fix drop-at-top sorting inversion (#738) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the drop-at-top target for a container child column falls back to -container_uid (either through the Path 1 rewrite from a positive page target, or through Path 2 commands that already carry target = -container_uid), DataHandler used the container record's own page-level sorting as the anchor to compute the moved record's new sorting. Whenever the invariant container.sorting < min(child.sorting) does not hold — which arises naturally on populated pages, in workspaces with MOVE_POINTER children keeping low sortings, or after editorial moves — the computed sorting sat above the existing children, so the moved element appeared at the *bottom* of the children list even though the editor visually dropped it at the top. Before returning the -container_uid anchor the hook now renumbers the container's children so that all their sortings sit strictly above the container's own sorting. This restores the invariant and lets DataHandler place the moved record inside the children's sort space. Path 2 (target = -container_uid) was previously skipped entirely by `rewriteCommandMapTargetForTopAtContainer` because of the `target > 0` gate; it is now covered by a companion branch. The renumber uses the current BE user's active workspace to scope both read and update. Fixes #738 --- .../Datahandler/CommandMapBeforeStartHook.php | 90 +++++++++++++++++++ .../MoveIntoInvertedContainerAtTopResult.csv | 9 ++ .../MoveElementSortingInverted/setup.csv | 9 ++ .../MoveElementSortingInvertedTest.php | 84 +++++++++++++++++ 4 files changed, 192 insertions(+) create mode 100644 Tests/Functional/Datahandler/DefaultLanguage/Fixtures/MoveElementSortingInverted/MoveIntoInvertedContainerAtTopResult.csv create mode 100644 Tests/Functional/Datahandler/DefaultLanguage/Fixtures/MoveElementSortingInverted/setup.csv create mode 100644 Tests/Functional/Datahandler/DefaultLanguage/MoveElementSortingInvertedTest.php diff --git a/Classes/Hooks/Datahandler/CommandMapBeforeStartHook.php b/Classes/Hooks/Datahandler/CommandMapBeforeStartHook.php index 91287333..adf821b8 100644 --- a/Classes/Hooks/Datahandler/CommandMapBeforeStartHook.php +++ b/Classes/Hooks/Datahandler/CommandMapBeforeStartHook.php @@ -14,10 +14,14 @@ use B13\Container\Domain\Factory\ContainerFactory; use B13\Container\Domain\Factory\Exception; +use B13\Container\Domain\Model\Container; use B13\Container\Domain\Service\ContainerService; use B13\Container\Tca\Registry; use Symfony\Component\DependencyInjection\Attribute\Autoconfigure; +use TYPO3\CMS\Core\Database\Connection; +use TYPO3\CMS\Core\Database\ConnectionPool; use TYPO3\CMS\Core\DataHandling\DataHandler; +use TYPO3\CMS\Core\Utility\GeneralUtility; #[Autoconfigure(public: true)] class CommandMapBeforeStartHook @@ -162,6 +166,7 @@ protected function rewriteCommandMapTargetForTopAtContainer(array $cmdmap): arra continue; } + // Path 1: drop-at-top rewritten from a positive page-uid target. if ( isset($value['update']) && isset($value['update']['tx_container_parent']) && @@ -173,17 +178,102 @@ protected function rewriteCommandMapTargetForTopAtContainer(array $cmdmap): arra try { $container = $this->containerFactory->buildContainer((int)$value['update']['tx_container_parent']); $target = $this->containerService->getNewContentElementAtTopTargetInColumn($container, (int)$value['update']['colPos']); + if ($target === -$container->getUid()) { + // Fallback anchor active. Ensure the invariant + // container.sorting < min(child.sorting) so DataHandler + // computes the new sorting within the children's sort space + // instead of around the container record itself (see #738). + $this->normalizeContainerChildrenSorting($container); + } $cmd[$operation]['target'] = $target; } catch (Exception $e) { // not a container } } + + // Path 2: command that already arrives with target = -container_uid. + // The Path 1 branch skips this case (target > 0 gate), so the + // -container_uid anchor reaches DataHandler unchanged. Ensure the + // invariant here as well (see #738). + if ( + isset($value['update']) && + isset($value['update']['tx_container_parent']) && + $value['update']['tx_container_parent'] > 0 && + isset($value['update']['colPos']) && + $value['update']['colPos'] > 0 && + isset($value['target']) && + (int)$value['target'] < 0 && + abs((int)$value['target']) === (int)$value['update']['tx_container_parent'] + ) { + try { + $container = $this->containerFactory->buildContainer((int)$value['update']['tx_container_parent']); + $this->normalizeContainerChildrenSorting($container); + } catch (Exception $e) { + // not a container + } + } } } } return $cmdmap; } + /** + * Renumber the container's children so that all sorting values sit strictly + * above the container's own sorting. Invoked from the drop-at-top path when + * the anchor falls back to -container_uid. Without this, DataHandler would + * compute the new sorting from the container record's own page-level + * sorting, which is outside the children's sort range whenever + * container.sorting > min(child.sorting) (see #738). + * + * Writes are workspace-aware: the current BE user's active workspace scopes + * both the read and the update. + */ + protected function normalizeContainerChildrenSorting(Container $container): void + { + $containerRecord = $container->getContainerRecord(); + $containerUid = (int)$containerRecord['uid']; + $containerSorting = (int)$containerRecord['sorting']; + $workspace = (int)($GLOBALS['BE_USER']->workspace ?? 0); + + $connection = GeneralUtility::makeInstance(ConnectionPool::class)->getConnectionForTable('tt_content'); + $qb = $connection->createQueryBuilder(); + $qb->getRestrictions()->removeAll(); + $qb->select('uid', 'sorting') + ->from('tt_content') + ->where( + $qb->expr()->eq('tx_container_parent', $qb->createNamedParameter($containerUid, Connection::PARAM_INT)), + $qb->expr()->eq('deleted', $qb->createNamedParameter(0, Connection::PARAM_INT)), + $qb->expr()->eq('t3ver_wsid', $qb->createNamedParameter($workspace, Connection::PARAM_INT)), + ) + ->orderBy('sorting', 'ASC') + ->addOrderBy('uid', 'ASC'); + $children = $qb->executeQuery()->fetchAllAssociative(); + + if ($children === []) { + return; + } + if ((int)$children[0]['sorting'] > $containerSorting) { + // invariant already holds + return; + } + + $newSorting = $containerSorting + 256; + $tstamp = time(); + foreach ($children as $child) { + $childUid = (int)$child['uid']; + if ((int)$child['sorting'] !== $newSorting) { + $connection->update( + 'tt_content', + ['sorting' => $newSorting, 'tstamp' => $tstamp], + ['uid' => $childUid], + ['sorting' => Connection::PARAM_INT, 'tstamp' => Connection::PARAM_INT, 'uid' => Connection::PARAM_INT] + ); + } + $newSorting += 256; + } + } + protected function rewriteSimpleCommandMap(array $cmdmap): array { if (!empty($cmdmap['tt_content'])) { diff --git a/Tests/Functional/Datahandler/DefaultLanguage/Fixtures/MoveElementSortingInverted/MoveIntoInvertedContainerAtTopResult.csv b/Tests/Functional/Datahandler/DefaultLanguage/Fixtures/MoveElementSortingInverted/MoveIntoInvertedContainerAtTopResult.csv new file mode 100644 index 00000000..f3a036c1 --- /dev/null +++ b/Tests/Functional/Datahandler/DefaultLanguage/Fixtures/MoveElementSortingInverted/MoveIntoInvertedContainerAtTopResult.csv @@ -0,0 +1,9 @@ +"pages" +,"uid","pid","title" +,1,0,"page-1" +"tt_content" +,"uid","pid","CType","header","sorting","sys_language_uid","colPos","tx_container_parent" +,1,1,"b13-2cols-with-header-container","container",10000,0,0,0 +,2,1,"header","child A",10256,0,200,1 +,3,1,"header","child B",10512,0,200,1 +,4,1,"header","test subject",10128,0,200,1 diff --git a/Tests/Functional/Datahandler/DefaultLanguage/Fixtures/MoveElementSortingInverted/setup.csv b/Tests/Functional/Datahandler/DefaultLanguage/Fixtures/MoveElementSortingInverted/setup.csv new file mode 100644 index 00000000..59186c1d --- /dev/null +++ b/Tests/Functional/Datahandler/DefaultLanguage/Fixtures/MoveElementSortingInverted/setup.csv @@ -0,0 +1,9 @@ +"pages" +,"uid","pid","title","slug" +,1,0,"page-1","/" +"tt_content" +,"uid","pid","CType","header","sorting","sys_language_uid","colPos","tx_container_parent" +,1,1,"b13-2cols-with-header-container","container",10000,0,0,0 +,2,1,"header","child A",100,0,200,1 +,3,1,"header","child B",200,0,200,1 +,4,1,"header","test subject",10500,0,0,0 diff --git a/Tests/Functional/Datahandler/DefaultLanguage/MoveElementSortingInvertedTest.php b/Tests/Functional/Datahandler/DefaultLanguage/MoveElementSortingInvertedTest.php new file mode 100644 index 00000000..831891e8 --- /dev/null +++ b/Tests/Functional/Datahandler/DefaultLanguage/MoveElementSortingInvertedTest.php @@ -0,0 +1,84 @@ + min(child.sorting). The hook now normalises the + * children's sorting before the -container_uid fallback anchor is used, so + * the moved element lands at the top of the children as intended. + */ +class MoveElementSortingInvertedTest extends AbstractDatahandler +{ + #[Test] + public function moveIntoInvertedContainerAtTopViaNegativeTarget(): void + { + // Path 2: cmd already carries target = -container_uid. + // Upstream, the sorting rewrite skipped this branch because of the + // `target > 0` gate, so the anchor reached DataHandler unchanged. + $this->importCSVDataSet(__DIR__ . '/Fixtures/MoveElementSortingInverted/setup.csv'); + $cmdmap = [ + 'tt_content' => [ + 4 => [ + 'move' => [ + 'action' => 'paste', + 'target' => -1, + 'update' => [ + 'colPos' => 200, + 'tx_container_parent' => 1, + 'sys_language_uid' => 0, + ], + ], + ], + ], + ]; + $this->dataHandler->start([], $cmdmap, $this->backendUser); + $this->dataHandler->process_datamap(); + $this->dataHandler->process_cmdmap(); + self::assertCSVDataSet(__DIR__ . '/Fixtures/MoveElementSortingInverted/MoveIntoInvertedContainerAtTopResult.csv'); + } + + #[Test] + public function moveIntoInvertedContainerAtTopViaPositivePageTarget(): void + { + // Path 1: cmd arrives with target = positive page uid + update carries + // tx_container_parent + colPos. The rewrite path resolves this via + // ContainerService::getNewContentElementAtTopTargetInColumn(), which + // falls back to -container_uid for the first configured column when + // no preceding column has records. + $this->importCSVDataSet(__DIR__ . '/Fixtures/MoveElementSortingInverted/setup.csv'); + $cmdmap = [ + 'tt_content' => [ + 4 => [ + 'move' => [ + 'action' => 'paste', + 'target' => 1, + 'update' => [ + 'colPos' => 200, + 'tx_container_parent' => 1, + 'sys_language_uid' => 0, + ], + ], + ], + ], + ]; + $this->dataHandler->start([], $cmdmap, $this->backendUser); + $this->dataHandler->process_datamap(); + $this->dataHandler->process_cmdmap(); + self::assertCSVDataSet(__DIR__ . '/Fixtures/MoveElementSortingInverted/MoveIntoInvertedContainerAtTopResult.csv'); + } +} From ea13d090c2ed6dea11ccc6578b08c282cb0f952a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dennis=20Br=C3=A4ndle?= Date: Fri, 3 Jul 2026 14:29:37 +0200 Subject: [PATCH 2/2] Extract renumber into Integrity\Sorting, skip in workspaces Adjustments after review: - Move the drop-at-top renumber out of CommandMapBeforeStartHook into Integrity\Sorting::normalizeChildrenSortingForContainer(). The hook now delegates via the injected Sorting service. - Read children through the Container model (via ContainerFactory), which keeps the read side language- and configured column-order-scoped. Renumbering iterates tcaRegistry->getAvailableColumns() in configured order and preserves within-column sorting order. - Extract the sort interval into a Sorting::SORT_INTERVAL constant that mirrors DataHandler::\$sortIntervals. - Explicitly skip in non-live workspaces. In a non-live workspace the ContainerFactory can return live records for children that have no workspace overlay yet; a direct SQL update would then silently mutate live rows from a workspace DataHandler run. Workspace handling is intentionally left for a follow-up. --- .../Datahandler/CommandMapBeforeStartHook.php | 68 ++------------ Classes/Integrity/Sorting.php | 89 +++++++++++++++++++ 2 files changed, 94 insertions(+), 63 deletions(-) diff --git a/Classes/Hooks/Datahandler/CommandMapBeforeStartHook.php b/Classes/Hooks/Datahandler/CommandMapBeforeStartHook.php index adf821b8..a9a7cdba 100644 --- a/Classes/Hooks/Datahandler/CommandMapBeforeStartHook.php +++ b/Classes/Hooks/Datahandler/CommandMapBeforeStartHook.php @@ -14,14 +14,11 @@ use B13\Container\Domain\Factory\ContainerFactory; use B13\Container\Domain\Factory\Exception; -use B13\Container\Domain\Model\Container; use B13\Container\Domain\Service\ContainerService; +use B13\Container\Integrity\Sorting; use B13\Container\Tca\Registry; use Symfony\Component\DependencyInjection\Attribute\Autoconfigure; -use TYPO3\CMS\Core\Database\Connection; -use TYPO3\CMS\Core\Database\ConnectionPool; use TYPO3\CMS\Core\DataHandling\DataHandler; -use TYPO3\CMS\Core\Utility\GeneralUtility; #[Autoconfigure(public: true)] class CommandMapBeforeStartHook @@ -30,7 +27,8 @@ public function __construct( protected ContainerFactory $containerFactory, protected Registry $tcaRegistry, protected Database $database, - protected ContainerService $containerService + protected ContainerService $containerService, + protected Sorting $sorting ) { } @@ -183,7 +181,7 @@ protected function rewriteCommandMapTargetForTopAtContainer(array $cmdmap): arra // container.sorting < min(child.sorting) so DataHandler // computes the new sorting within the children's sort space // instead of around the container record itself (see #738). - $this->normalizeContainerChildrenSorting($container); + $this->sorting->normalizeChildrenSortingForContainer($container); } $cmd[$operation]['target'] = $target; } catch (Exception $e) { @@ -207,7 +205,7 @@ protected function rewriteCommandMapTargetForTopAtContainer(array $cmdmap): arra ) { try { $container = $this->containerFactory->buildContainer((int)$value['update']['tx_container_parent']); - $this->normalizeContainerChildrenSorting($container); + $this->sorting->normalizeChildrenSortingForContainer($container); } catch (Exception $e) { // not a container } @@ -218,62 +216,6 @@ protected function rewriteCommandMapTargetForTopAtContainer(array $cmdmap): arra return $cmdmap; } - /** - * Renumber the container's children so that all sorting values sit strictly - * above the container's own sorting. Invoked from the drop-at-top path when - * the anchor falls back to -container_uid. Without this, DataHandler would - * compute the new sorting from the container record's own page-level - * sorting, which is outside the children's sort range whenever - * container.sorting > min(child.sorting) (see #738). - * - * Writes are workspace-aware: the current BE user's active workspace scopes - * both the read and the update. - */ - protected function normalizeContainerChildrenSorting(Container $container): void - { - $containerRecord = $container->getContainerRecord(); - $containerUid = (int)$containerRecord['uid']; - $containerSorting = (int)$containerRecord['sorting']; - $workspace = (int)($GLOBALS['BE_USER']->workspace ?? 0); - - $connection = GeneralUtility::makeInstance(ConnectionPool::class)->getConnectionForTable('tt_content'); - $qb = $connection->createQueryBuilder(); - $qb->getRestrictions()->removeAll(); - $qb->select('uid', 'sorting') - ->from('tt_content') - ->where( - $qb->expr()->eq('tx_container_parent', $qb->createNamedParameter($containerUid, Connection::PARAM_INT)), - $qb->expr()->eq('deleted', $qb->createNamedParameter(0, Connection::PARAM_INT)), - $qb->expr()->eq('t3ver_wsid', $qb->createNamedParameter($workspace, Connection::PARAM_INT)), - ) - ->orderBy('sorting', 'ASC') - ->addOrderBy('uid', 'ASC'); - $children = $qb->executeQuery()->fetchAllAssociative(); - - if ($children === []) { - return; - } - if ((int)$children[0]['sorting'] > $containerSorting) { - // invariant already holds - return; - } - - $newSorting = $containerSorting + 256; - $tstamp = time(); - foreach ($children as $child) { - $childUid = (int)$child['uid']; - if ((int)$child['sorting'] !== $newSorting) { - $connection->update( - 'tt_content', - ['sorting' => $newSorting, 'tstamp' => $tstamp], - ['uid' => $childUid], - ['sorting' => Connection::PARAM_INT, 'tstamp' => Connection::PARAM_INT, 'uid' => Connection::PARAM_INT] - ); - } - $newSorting += 256; - } - } - protected function rewriteSimpleCommandMap(array $cmdmap): array { if (!empty($cmdmap['tt_content'])) { diff --git a/Classes/Integrity/Sorting.php b/Classes/Integrity/Sorting.php index dcf4b08e..5a9ab27d 100644 --- a/Classes/Integrity/Sorting.php +++ b/Classes/Integrity/Sorting.php @@ -17,11 +17,20 @@ use B13\Container\Domain\Model\Container; use B13\Container\Domain\Service\ContainerService; use B13\Container\Tca\Registry; +use TYPO3\CMS\Core\Database\Connection; +use TYPO3\CMS\Core\Database\ConnectionPool; use TYPO3\CMS\Core\DataHandling\DataHandler; use TYPO3\CMS\Core\Utility\GeneralUtility; class Sorting { + /** + * Default sort interval used when renumbering children. Matches + * DataHandler::$sortIntervals (which is instance state, not a constant, + * so we mirror it here). + */ + public const SORT_INTERVAL = 256; + protected array $errors = []; public function __construct( @@ -65,6 +74,86 @@ protected function unsetContentDefenderConfiguration(string $cType): void } } + /** + * Renumber the container's children so that all sorting values sit strictly + * above the container's own sorting, preserving the configured container + * column order (`tcaRegistry` order) and the within-column sorting order. + * + * Intended for the drop-at-top path in `CommandMapBeforeStartHook`: when + * the `-container_uid` fallback anchor is used, the invariant + * `container.sorting < min(child.sorting)` must hold, otherwise + * DataHandler computes the moved record's new sorting from the container's + * own page-level sorting instead of the children's sort space + * (see #738). + * + * The read side reuses the `Container` model for language and configured + * column-order scoping. + * + * Writes are direct `Connection::update()`s of `sorting` and `tstamp` — + * intentionally not routed through DataHandler to avoid re-entering the + * hook stack from within a hook. Only touches the container's own direct + * children. + * + * Workspaces are intentionally out of scope for this initial fix: the + * method short-circuits when the current BE user is not in workspace 0. + * The reason is that in a non-live workspace `ContainerFactory` can + * return live records for children that have no workspace overlay yet; + * a direct SQL update would then silently mutate live rows from a + * workspace DataHandler run. The workspace path deserves its own, + * overlay-aware follow-up. + */ + public function normalizeChildrenSortingForContainer(Container $container): void + { + $workspace = (int)($GLOBALS['BE_USER']->workspace ?? 0); + if ($workspace !== 0) { + // Non-live workspace: skip. See method docblock for rationale. + return; + } + + $containerRecord = $container->getContainerRecord(); + $containerSorting = (int)$containerRecord['sorting']; + $cType = $container->getCType(); + + $orderedChildren = []; + foreach ($this->tcaRegistry->getAvailableColumns($cType) as $column) { + $children = $container->getChildrenByColPos((int)$column['colPos']); + foreach ($children as $child) { + $orderedChildren[] = $child; + } + } + + if ($orderedChildren === []) { + return; + } + + $minChildSorting = null; + foreach ($orderedChildren as $child) { + $sorting = (int)$child['sorting']; + if ($minChildSorting === null || $sorting < $minChildSorting) { + $minChildSorting = $sorting; + } + } + if ($minChildSorting !== null && $minChildSorting > $containerSorting) { + return; + } + + $connection = GeneralUtility::makeInstance(ConnectionPool::class)->getConnectionForTable('tt_content'); + $newSorting = $containerSorting + self::SORT_INTERVAL; + $tstamp = time(); + foreach ($orderedChildren as $child) { + $childUid = (int)$child['uid']; + if ((int)$child['sorting'] !== $newSorting) { + $connection->update( + 'tt_content', + ['sorting' => $newSorting, 'tstamp' => $tstamp], + ['uid' => $childUid], + ['sorting' => Connection::PARAM_INT, 'tstamp' => Connection::PARAM_INT, 'uid' => Connection::PARAM_INT] + ); + } + $newSorting += self::SORT_INTERVAL; + } + } + protected function fixChildrenSortingUpdateRequired(Container $container, array $colPosByCType): bool { $containerRecord = $container->getContainerRecord();