Skip to content

BUGFIX: No double moving of nodes across dimensions if synced #61

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

Conversation

gradinarufelix
Copy link
Collaborator

@gradinarufelix gradinarufelix commented Jun 27, 2025

In sync mode, moving a document node throws an error because Neos is already trying to move the document nodes across dimensions. This PR introduces an isActive flag which tells if the translation (with moving the nodes) is currently active, or not.

Then, an aspect avoids the Neos own moving across dimension if the nodes are in a dimension that is taken care of by the sync functionality of this package.

@gradinarufelix gradinarufelix changed the base branch from 3.0 to 2.0 June 27, 2025 11:58
Copilot

This comment was marked as outdated.

Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that you want to prevent recursion but consider "active" a bit confusing. Because active is a positive thing while we want to prevent bad recursion.

Can we name this flag and the methods "recursionPreventionEnabled". That way we immediately get what they are for when we stumble over them in a year or two.

Also this would allow very explicit methods "enableRecursionPrevention", "disableRecursionPrevention", "isRecurSionPreventionEnabled" that future us will likely understand faster

@gradinarufelix gradinarufelix force-pushed the bugfix/no-double-moving-of-nodes-across-dimensions-if-synced branch from ddc182b to 33f4020 Compare July 2, 2025 13:59
@gradinarufelix gradinarufelix requested review from Copilot and mficzel July 2, 2025 14:22
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR prevents Neos from moving node variants twice in sync mode by introducing a recursion-prevention flag and an AOP aspect to skip native moves when syncing.

  • Added a recursionPreventionEnabled property and getter to coordinate disabling Neos’ native dimension moves.
  • Wrapped afterAdoptNode and syncNode methods in NodeTranslationService to toggle the flag.
  • Introduced an AroundMoveNodeDataAspect to intercept moveNodeData() and skip it when sync strategy is active.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
Classes/ContentRepository/NodeTranslationService.php Added recursionPreventionEnabled flag, getter, and logic around adopt and sync operations.
Classes/Aop/AroundMoveNodeDataAspect.php New AOP aspect to bypass Neos’ moveNodeData() when recursion prevention is enabled for sync translations.
Comments suppressed due to low confidence (1)

Classes/ContentRepository/NodeTranslationService.php:113

  • Add unit tests for recursionPreventionEnabled behavior in NodeTranslationService and integration tests for AroundMoveNodeDataAspect to verify that native node moves are skipped when sync strategy is active.
    protected bool $recursionPreventionEnabled = true;

@@ -284,6 +298,8 @@ public function syncNode(NodeInterface $sourceNode, string $workspaceName = 'liv
if ($nodeSourceDimensionValue !== $defaultPreset) {
return;
}

$this->recursionPreventionEnabled = false;
Copy link
Preview

Copilot AI Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap the disabling and re-enabling of recursionPreventionEnabled in syncNode inside a try/finally block (as done in afterAdoptNode) to ensure the flag is always reset even if an exception occurs.

Copilot uses AI. Check for mistakes.

sprintf('presets.%s.options.translationStrategy', $nodeDataLanguageDimensionValue)
) === NodeTranslationService::TRANSLATION_STRATEGY_SYNC
) {
return null;
Copy link
Preview

Copilot AI Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning null here may not align with the original method’s return type (likely void). Consider using a bare return; or matching the original contract to avoid unexpected return values.

Suggested change
return null;
return;

Copilot uses AI. Check for mistakes.

@gradinarufelix gradinarufelix merged commit 02e0fcd into 2.0 Jul 3, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants