Skip to content

BUGFIX: Avoid orphans when publishing node move#5764

Open
kitsunet wants to merge 4 commits into8.3from
bugfix/83-move-broken-5763
Open

BUGFIX: Avoid orphans when publishing node move#5764
kitsunet wants to merge 4 commits into8.3from
bugfix/83-move-broken-5763

Conversation

@kitsunet
Copy link
Copy Markdown
Member

@kitsunet kitsunet commented Mar 18, 2026

See issue for detailed explanation. In a nutshell: NodeData::setPath is dangerous as it mixes DB queries with memory state and can lead to unexpected results. Using only NodeData::move avoids the problem.

Fixes: #5763

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the correct branch:
    • If it's a bugfix, use the lowest maintained branch which has the bug
    • If it's a non-breaking feature, use the branch of the next version (might be either minor or major)
    • If it's a breaking feature it should typically go into the next major version
  • 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

See issue for detailed explanation.
`NodeData::setPath` is dangerous as it mixes DB
queries with memory state and can lead to unexpected
results.
Using only `NodeData::move` avoids the problem.

Fixes: #5763
@kitsunet
Copy link
Copy Markdown
Member Author

Right, that's what I feared to see. I guess this needs a more sophisticated solution. I'll look into it.

@kitsunet
Copy link
Copy Markdown
Member Author

I added a new behat test that shows the problem without any changes to the CR. This and the other failing tests are green now. I know persisting after every node publish will slow down publishing, but I don't see a way to avoid this while also fixing the problems. If anyone finds a way, I am happy to see it. But for now, this seems safer.

@kitsunet
Copy link
Copy Markdown
Member Author

Ah funny, EventLog tests fail, I didn't run them locally, but that test is fragile anyways as it checks the number of events, but that is nothing the EventLog even controls or should have information about.

This avoids multiple Publish events being recorded for the same
publishing operation in a world where we persist after every node
being published.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant