Skip to content
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

Doctrine tries to delete entities in incorrect order #9376

Closed
jusephe opened this issue Jan 13, 2022 · 4 comments
Closed

Doctrine tries to delete entities in incorrect order #9376

jusephe opened this issue Jan 13, 2022 · 4 comments
Labels

Comments

@jusephe
Copy link

jusephe commented Jan 13, 2022

Bug Report

Q A
BC Break no
Version 2.11.0

Summary

In some cases, Doctrine tries to delete entities in incorrect order and it fails on foreign keys exception. It also happens in 2.10.4

Current behavior

I have 6 entities with circular reference, and I tried to delete 2 of them. But UnitOfWork::commit() fails because of ForeignKeyConstraintViolationException.

The reason is that it first tries to delete the entity set as a foreign key in the second entity, which hasn't been deleted yet.

I investigated it quite a bit and the order of these operations is determined by $commitOrder in UnitOfWork::commit(). It is calculated from all 6 entities, but for the delete itself, the order of those 2 entities is not correct within those 6 entities. It should be sufficient to compute the order for deletions only with those 2 entities set to be deleted. I prepared a test with described behavior and a possible fix. See linked pull request.

How to reproduce

Run the test in a linked pull request with a DB platform supporting foreign key constraints. (i.e., not SQLite)

Expected behavior

UnitOfWork::commit() should delete entities. I prepared a possible fix in the linked pull request. I created a new method called getCommitOrderForDeletions() for computing the correct order of entities set to be deleted. This function is similar to getCommitOrder(), but it operates only with entities set to be deleted.

@mpdude
Copy link
Contributor

mpdude commented Feb 27, 2023

The tests you provided in #9377 pass on #10547, so I assume that would fix this issue here as well.

Note your tests are much simpler than the case you describe in the OP, there is no circular dependency between 6 entities.

See eee75bf

mpdude added a commit to mpdude/doctrine2 that referenced this issue May 31, 2023
This is part of the series of issues fixed by doctrine#10547. In particular, the changes from doctrine#10566 were relevant.

See doctrine#9376 for the bug description; doctrine#9377 is obsoleted by doctrine#10547.
mpdude added a commit to mpdude/doctrine2 that referenced this issue May 31, 2023
This is part of the series of issues fixed by doctrine#10547. In particular, the changes from doctrine#10566 were relevant.

See doctrine#9376 for the bug description; doctrine#9377 is obsoleted by doctrine#10547.
mpdude added a commit to mpdude/doctrine2 that referenced this issue May 31, 2023
This is part of the series of issues fixed by doctrine#10547. In particular, the changes from doctrine#10566 were relevant.

See doctrine#9376 for the bug description; doctrine#9377 is obsoleted by doctrine#10547.
@mpdude
Copy link
Contributor

mpdude commented May 31, 2023

I agree that regarding deletions, we need to find an ordering among the entities scheduled for deletion only. #10547 will do that.

The circular reference you mention in your opening comment is not part of the test case you provided in #9377.

Also without a circular reference, there might be problems with the delete commit order, but only when the order of entities of the same class is relevant.

The test case from #9377 passes on the current 2.16.x branch, so it seems not to be sufficient to show the bug (or the bug has already been fixed?).

/cc @greg0ire

@mpdude
Copy link
Contributor

mpdude commented Jan 3, 2024

@jusephe do you still know how to reproduce the issue and do you think you could give #10913 a try?

@jusephe
Copy link
Author

jusephe commented Jun 19, 2024

@mpdude Hi, sorry for late answer. The problem is fixed either by #10547 or #10913. Thank you very much! I am closing this.

@jusephe jusephe closed this as completed Jun 19, 2024
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 a pull request may close this issue.

3 participants