[BUGFIX] Fix drop-at-top sorting inversion (#738)#755
Draft
dbraendle wants to merge 2 commits into
Draft
Conversation
added 2 commits
July 3, 2026 13:22
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 b13#738
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Idea
Instead of returning a different anchor from
ContainerService::getNewContentElementAtTopTargetInColumn()— which would require a DataHandler concept of "insert before X" that does not really exist — this tries to restore the invariant before the existing-container_uidanchor is used:If that invariant holds,
-container_uidbecomes a valid top-of-children anchor again: DataHandler computes a sorting value between the container and the next record in that global ordering, which puts the moved record before the existing children.Rough shape
CommandMapBeforeStartHook::rewriteCommandMapTargetForTopAtContainer()now covers two paths:target > 0, drop-at-top from a positive page uid. IfgetNewContentElementAtTopTargetInColumn()returns the-container_uidfallback, the hook delegates the renumbering toIntegrity\Sorting::normalizeChildrenSortingForContainer(), then uses the anchor as before.target = -container_uidalready at hook time. This was previously skipped by thetarget > 0gate. A companion branch now applies the same renumbering. The anchor itself remains unchanged.The renumbering lives in
Integrity\Sorting. It reads children through theContainermodel and uses the configured container column order fromtcaRegistry. It writes onlysortingandtstampdirectly viaConnection::update()— intentionally not through DataHandler, to avoid re-entering the hook stack from inside a hook.Sorting::SORT_INTERVALmirrors DataHandler's default sort interval instead of keeping the number inline.Scope: live workspace only
The renumbering method short-circuits in any non-live workspace.
Rationale: in a non-live workspace,
ContainerFactorycan return live records for children that have no workspace overlay yet. A direct SQL update would then risk mutating live rows from a workspace DataHandler run. Workspace handling therefore needs an overlay-aware follow-up and is intentionally not part of this draft.Verified only for the live workspace / default-language path.
Test coverage included
Two functional tests are included as a starting point:
target = -container_uid-container_uidfallbackBoth use a container with
sorting=10000, existing children atsorting=100/200, and a moved record that should land at the top of the children after the operation.Not covered yet
sorting/tstampare acceptable here.If the general direction — restoring the invariant before using the existing anchor — is wrong, I'd rather hear that early before polishing the implementation further.