Skip to content

BUGFIX: Snyc nodes on shutdown #66

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

Merged
merged 6 commits into from
Jul 4, 2025
Merged

Conversation

gradinarufelix
Copy link
Collaborator

@gradinarufelix gradinarufelix commented Jul 3, 2025

If you move document nodes in sync mode, it leads to an error because it tries to move the content elements first. If you try to move document-2 into document-1 this leads to an error like Node path /sites/site/document-2/content does not start with /sites/site/document-1/document-2.

Another example from my test when moving a document node into another document node: Given path "/sites/neosdemo/features/other-elements" is not the beginning of "/sites/neosdemo/features/forms/other-elements/main/node53a18f52e9726", cannot get a relative path between them.

This PR solves the problem by collecting all nodes to be synced/translated and executing all of that on shutdown in an optimized order.

Because removed nodes are not available anymore at this stage, removed nodes must be stored as objects and not just as identifiers.

@gradinarufelix gradinarufelix requested a review from Copilot July 3, 2025 22:14
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 fixes errors when moving document nodes in sync mode by batching node sync/translation operations at shutdown in an optimized order and storing removed nodes as objects.

  • Switches signal connections from immediate publish to collection before publish and final translation on persistence flush
  • Introduces a buffer (nodesToBeTranslated) and a translateNodes method triggered on allObjectsPersisted
  • Injects the persistence manager and ensures all buffered nodes are persisted after translation

Reviewed Changes

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

File Description
Classes/Package.php Updated signal-slot connections to collect and translate nodes on shutdown, injecting PersistenceManager
Classes/ContentRepository/NodeTranslationService.php Added nodesToBeTranslated buffer, collectNodesToBeTranslated and translateNodes methods; injected persistence manager and adjusted sync logic
Comments suppressed due to low confidence (3)

Classes/ContentRepository/NodeTranslationService.php:218

  • translateNodes contains critical batch-sync logic but appears untested; consider adding unit or integration tests to validate the ordering and handling of buffered nodes on shutdown.
    public function translateNodes(): void

Classes/Package.php:11

  • Consider injecting the PersistenceManagerInterface instead of the concrete Doctrine PersistenceManager to follow interface-based dependency injection and improve testability.
use Neos\Flow\Persistence\Doctrine\PersistenceManager;

Classes/Package.php:29

  • Verify that the PersistenceManager actually emits an 'allObjectsPersisted' signal—if the event name is incorrect or not emitted, the translateNodes method will never run.
        $dispatcher->connect(PersistenceManager::class, 'allObjectsPersisted', NodeTranslationService::class, 'translateNodes', false);

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.

Fine by 👀

@gradinarufelix gradinarufelix merged commit f76366c into 2.0 Jul 4, 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