Skip to content

[Tree][NestedSet] Fix inserting multiple roots in one flush #2932

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 7 commits into
base: main
Choose a base branch
from

Conversation

sgehrig
Copy link

@sgehrig sgehrig commented Mar 18, 2025

This PR tries to solve the issue described in #2582. Currently it only provides a test case that reproduces the issue and the observations made in #2582 (comment) (persisting roots first) and in #2582 (comment) (doing two flushes).

\Gedmo\Tests\Tree\Issue\Issue2582Test::testInsertTwoRootsInOneFlush fails as expected right now because it is a direct replication of the issue. \Gedmo\Tests\Tree\Issue\Issue2582Test::testInsertTwoRootsInOneFlushRootsFirst and \Gedmo\Tests\Tree\Issue\Issue2582Test::testInsertTwoRootsInTwoFlushes both succeed.

@sgehrig
Copy link
Author

sgehrig commented Mar 18, 2025

The change in \Gedmo\Tree\Strategy\ORM\Nested fixes the issue and keeps all tests green. I am not sure however if this is in any shape or form correct.

When debugging the failing test case I noticed that the \Gedmo\Tree\Strategy\ORM\Nested::$treeEdges property is not updated when \Gedmo\Tree\Strategy\ORM\Nested::updateNode handles the second non-root node.

@sgehrig sgehrig marked this pull request as ready for review March 19, 2025 17:00
@phansys phansys added Bug A confirmed bug in Extensions that needs fixing. Tree Needs Changelog note labels Mar 24, 2025
Copy link

codecov bot commented Mar 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.44%. Comparing base (211b6fe) to head (2156abe).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2932      +/-   ##
==========================================
- Coverage   78.53%   78.44%   -0.09%     
==========================================
  Files         168      168              
  Lines        8790     8809      +19     
==========================================
+ Hits         6903     6910       +7     
- Misses       1887     1899      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sgehrig
Copy link
Author

sgehrig commented May 15, 2025

@phansys Is there anything I need to do to get this one merged? Thanks a lot for your help.

@sgehrig sgehrig force-pushed the bug/#2582-multiple-roots branch from 2156abe to cf528c8 Compare May 15, 2025 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A confirmed bug in Extensions that needs fixing. Needs Changelog note Tree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants