Skip to content

Fix moving nodes to a different parent node #5561

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: 8.3
Choose a base branch
from

Conversation

Benjamin-K
Copy link
Contributor

@Benjamin-K Benjamin-K commented May 26, 2025

Upgrade instructions

Currently, when moving nodes to a different parent node, the publishing fails, as the referenced shadow node can not be found. This happens, because NodeData::move() method tries to find the node data by the source node instead of the movedTo-property.

Review instructions

Try to move a node to a different parent node (move page inside another page for example) without this PR. Then try it again with this fix (mainly replace findOneByMovedTo($sourceNodeData) with findOneByMovedTo($sourceNodeData->getMovedTo())).

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

** Thanks **

Thanks to @robinroloff for also digging into this.

@github-actions github-actions bot added the 8.3 label May 26, 2025
@Benjamin-K Benjamin-K force-pushed the fix/8.3-move-nodes branch 2 times, most recently from f8ca820 to e60f1f5 Compare May 26, 2025 09:25
@dlubitz
Copy link
Contributor

dlubitz commented Jun 6, 2025

Thanks @Benjamin-K for your PR. Can you please check why the tests are failing?

@mhsdesign
Copy link
Member

there is a big discussion in slack ... https://neos-project.slack.com/archives/C050C8FEK/p1748421463568619?thread_ts=1748016029.156139&cid=C050C8FEK ... we just have to find someone with a working behat setup for neos 8 ... let me check if i can get it working.

@Benjamin-K
Copy link
Contributor Author

@robinroloff only changed this line:

$referencedShadowNode = $this->nodeDataRepository->findOneByMovedTo($sourceNodeData);

I will try if it works with only changing this line. Otherwise we will need someone who can run the Behat tests and check, why they are failing...

@Benjamin-K Benjamin-K force-pushed the fix/8.3-move-nodes branch from e60f1f5 to 42f12f3 Compare June 6, 2025 09:11
@Benjamin-K
Copy link
Contributor Author

I will try if it works with only changing this line.

Ok, this didn't help. But it's hard to debug because the code is really nested...

What Behat does in the failing test:

  1. Call $node->setName($newName), which calls
  2. $node->setPath(NodePaths::addNodePathSegment($node->getParentPath(), $newName)).
  3. As a Document node is an aggregate, $node->setPathInternalForAggregate($path, recursive: false) will be called.
  4. This load all $nodeDataVariantsAndChildren (in all dimensions) and call $node->moveNodeData($nodeData, $originalPath, $destinationPath, recursiveCall: false); for each nodeData found.
  5. After checks, if it is a variant or the node itself, $node->moveVariantOrChild($originalPath, $destinationPath, $nodeVariant) is called.
  6. Abstract of Method Node::moveVariantChild(...):
    // $nodeToMove === $nodeVariant
    $variantOriginalPath = $nodeToMove->getPath();
    $relativePathSegment = NodePaths::getRelativePathBetween($aggregateOriginalPath, $variantOriginalPath);
    $variantDestinationPath = NodePaths::addNodePathSegment($aggregateDestinationPath, $relativePathSegment);
    $this->moveNodeToDestinationPath($nodeToMove, $variantDestinationPath);
  7. Node::moveNodeToDestinationPath(...) finally calls $node->getNodeData()->move($destinationPath, $node->context->getWorkspace())

So far, this doesn't look like sth. wrong. So the error must occur in the move method itself... 😮‍💨
Maybe, the original code was already correct but doctrine didn't execute all queries before updating the path and we got some duplicates...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants