doctrine/archived-orm
1
0
Fork 0
mirror of https://github.com/doctrine/orm.git synced 2026-03-23 22:42:18 +01:00
Code Issues 5.3k Packages Projects Releases 100 Wiki Activity

Order of referenced child deletions not correct when referencing to itself #7085

New Issue
Closed
opened 2026-01-22 15:44:15 +01:00 by admin · 15 comments
admin commented 2026-01-22 15:44:15 +01:00
Owner
Copy Link

Originally created by @Toflar on GitHub (Dec 29, 2022).

Bug Report

Q A
BC Break no
Version 2.14.0

Summary

The order in which the UoW wants to delete children is somehow missing a dependency when there is an association to the same class even if you specified cascade: ['remove'].
Given you have

  • Parent -> has an association to children with cascade: ['remove']
  • Child 1
  • Child 2 -> has an association to Child 1 with cascade: ['remove'].

Now you delete the parent.
Doctrine tries to remove Child 1 first instead of Child 2 causing a foreign constraint violation:

Uncaught PDOException: SQLSTATE[23000]: Integrity constraint violation: 1451 Cannot delete or update a parent row: a foreign key constraint fails (test.child_entities, CONSTRAINT FK_DAEB194156A273CC FOREIGN KEY (origin_id) REFERENCES child_entities (id))

How to reproduce

I've created a test setup because I'm not familiar with the internals of ORM at all. So here we go:

  1. Clone https://github.com/Toflar/doctrine-orm-bug-reproducer
  2. Run composer install
  3. Adjust dummy mysql connection example in bootstrap.php.
  4. Run bin/doctrine orm:schema-tool:drop --force && bin/doctrine orm:schema-tool:create
  5. Run php test.php.

Expected behavior

It should not throw an exception. Doctrine should remove Child 2 first.

Originally created by @Toflar on GitHub (Dec 29, 2022). ### Bug Report | Q | A |------------ | ------ | BC Break | no | Version | 2.14.0 #### Summary The order in which the UoW wants to delete children is somehow missing a dependency when there is an association to the same class even if you specified `cascade: ['remove']`. Given you have - Parent -> has an association to children with `cascade: ['remove']` - Child 1 - Child 2 -> has an association to Child 1 with `cascade: ['remove']`. Now you delete the parent. Doctrine tries to remove Child 1 first instead of Child 2 causing a foreign constraint violation: > Uncaught PDOException: SQLSTATE[23000]: Integrity constraint violation: 1451 Cannot delete or update a parent row: a foreign key constraint fails (`test`.`child_entities`, CONSTRAINT `FK_DAEB194156A273CC` FOREIGN KEY (`origin_id`) REFERENCES `child_entities` (`id`)) #### How to reproduce I've created a test setup because I'm not familiar with the internals of ORM at all. So here we go: 1. Clone https://github.com/Toflar/doctrine-orm-bug-reproducer 2. Run `composer install` 3. Adjust dummy mysql connection example in `bootstrap.php`. 4. Run `bin/doctrine orm:schema-tool:drop --force && bin/doctrine orm:schema-tool:create` 5. Run `php test.php`. #### Expected behavior It should not throw an exception. Doctrine should remove Child 2 first.
admin added the Bug label 2026-01-22 15:44:15 +01:00
admin closed this issue 2026-01-22 15:44:15 +01:00
admin commented 2026-01-22 15:44:16 +01:00
Author
Owner
Copy Link

@simonberger commented on GitHub (Jan 2, 2023):

This test is based on @Toflar ' s reproducer and can be picked up:
2abff4cce5
I could also create a PR if requested. It fails with mysql but works with sqlite. I didn't test further.
It is no new bug and happened also in 2.11.

@simonberger commented on GitHub (Jan 2, 2023): This test is based on @Toflar ' s reproducer and can be picked up: https://github.com/simonberger/orm/commit/2abff4cce5fd8959574a9b0dc115df9aab509425 I could also create a PR if requested. It fails with mysql but works with sqlite. I didn't test further. It is no new bug and happened also in 2.11.
admin commented 2026-01-22 15:44:17 +01:00
Author
Owner
Copy Link

@Toflar commented on GitHub (Jan 2, 2023):

Thanks <3 Maybe sqlite is configured without foreign key support in the CI? 🤔

@Toflar commented on GitHub (Jan 2, 2023): Thanks <3 Maybe sqlite is configured without foreign key support in the CI? 🤔
admin commented 2026-01-22 15:44:17 +01:00
Author
Owner
Copy Link

@simonberger commented on GitHub (Jan 2, 2023):

I just tested local on mac. This could be the reason. Maybe it even fails in CI. :)
I am interested to know and created the PR. We can close it again after seeing the results.

EDIT:
Sqlite succeeds also in the CI. MariaDB, PostgreSQL and MySQL all fail.

@simonberger commented on GitHub (Jan 2, 2023): I just tested local on mac. This could be the reason. Maybe it even fails in CI. :) I am interested to know and created the PR. We can close it again after seeing the results. EDIT: Sqlite succeeds also in the CI. MariaDB, PostgreSQL and MySQL all fail.
admin commented 2026-01-22 15:44:18 +01:00
Author
Owner
Copy Link

@mpdude commented on GitHub (Feb 27, 2023):

@Toflar could you please have a look at #10547?

I am not too confident it might fix this issue here, since I do not know if all the entities that need to be cascade-deleted are put into the \Doctrine\ORM\UnitOfWork::$entityDeletions list before the commit order is computed.

But anyway, you might want to give it a shot.

@mpdude commented on GitHub (Feb 27, 2023): @Toflar could you please have a look at #10547? I am not too confident it might fix this issue here, since I do not know if all the entities that need to be cascade-deleted are put into the `\Doctrine\ORM\UnitOfWork::$entityDeletions` list _before_ the commit order is computed. But anyway, you might want to give it a shot.
admin commented 2026-01-22 15:44:18 +01:00
Author
Owner
Copy Link

@mpdude commented on GitHub (Feb 28, 2023):

Ah, maybe this is #10548?

@mpdude commented on GitHub (Feb 28, 2023): Ah, maybe this is #10548?
admin commented 2026-01-22 15:44:19 +01:00
Author
Owner
Copy Link

@Toflar commented on GitHub (Feb 28, 2023):

Hey @mpdude wow, the other PR is some impressive work! Maybe we can pick up @simonberger's reproducer (2abff4cce5) in the CI as well, then we know if your PR fixes the problem :)

@Toflar commented on GitHub (Feb 28, 2023): Hey @mpdude wow, the other PR is some impressive work! Maybe we can pick up @simonberger's reproducer (https://github.com/simonberger/orm/commit/2abff4cce5fd8959574a9b0dc115df9aab509425) in the CI as well, then we know if your PR fixes the problem :)
admin commented 2026-01-22 15:44:20 +01:00
Author
Owner
Copy Link

@simonberger commented on GitHub (Mar 5, 2023):

@mpdude Looks good that this issue is covered by your fix. I ran the tests on your branch and it succeeded.
I created a PR on your branch or just add it manually if you like.
https://github.com/mpdude/doctrine2/pull/2

@simonberger commented on GitHub (Mar 5, 2023): @mpdude Looks good that this issue is covered by your fix. I ran the tests on your branch and it succeeded. I created a PR on your branch or just add it manually if you like. https://github.com/mpdude/doctrine2/pull/2
admin commented 2026-01-22 15:44:20 +01:00
Author
Owner
Copy Link

@mpdude commented on GitHub (Mar 5, 2023):

That's great news, although I am not sure if that's by incident. But I have this issue on my shortlist and think it can be fixed as well.

For the delete case, we need not look out for nullable associations (like we do at insert time), but find the associations that use SET NULL as candidates for breaking cycles.

@mpdude commented on GitHub (Mar 5, 2023): That's great news, although I am not sure if that's by incident. But I have this issue on my shortlist and think it can be fixed as well. For the delete case, we need not look out for nullable associations (like we do at insert time), but find the associations that use `SET NULL` as candidates for breaking cycles.
admin commented 2026-01-22 15:44:20 +01:00
Author
Owner
Copy Link

@simonberger commented on GitHub (Mar 5, 2023):

While I don't have much knowledge about the implementation, the test at least looked senseful to me and should have some validity.

@simonberger commented on GitHub (Mar 5, 2023): While I don't have much knowledge about the implementation, the test at least looked senseful to me and should have some validity.
admin commented 2026-01-22 15:44:20 +01:00
Author
Owner
Copy Link

@mpdude commented on GitHub (Mar 5, 2023):

Definitely, it is! I did not want to put that into question, sorry if it sounded like that.

My thinking was that I see another problematic case pointed out by this issue here, and I think #10547 does not address this precisely enough. I’ll see if I can fine-tune the test a little to make it fail until that situation is addressed as well.

Thank you for the PR against my branch, that helps.

@mpdude commented on GitHub (Mar 5, 2023): Definitely, it is! I did not want to put that into question, sorry if it sounded like that. My thinking was that I see another problematic case pointed out by this issue here, and I think #10547 does not address this precisely enough. I’ll see if I can fine-tune the test a little to make it fail until that situation is addressed as well. Thank you for the PR against my branch, that helps.
admin commented 2026-01-22 15:44:21 +01:00
Author
Owner
Copy Link

@simonberger commented on GitHub (Mar 5, 2023):

I also didn't read it like this and the tests are from toflar, I just converted them to phpunit.

@simonberger commented on GitHub (Mar 5, 2023): I also didn't read it like this and the tests are from toflar, I just converted them to phpunit.
admin commented 2026-01-22 15:44:22 +01:00
Author
Owner
Copy Link

@mpdude commented on GitHub (Mar 6, 2023):

I've taken a closer look at this and in fact, what you've found here is a variant of the deletion case from #10531:

graph LR;
    Child1 --> Parent;
    Child2 --> Parent;
    Child2 --> Child1;

Both associations (from Child to Parent as well as from Child to Child) are cascade: delete, so trying to delete the Parent instance will setup the entire graph for deletion.

All associations are nullable in the database, but that's of no relevance for the delete case, since the ORM does not currently use an "extra update" before DELETE to break cycles.

There is no database-level ON DELETE SET NULL, so the ORM will have to find a valid deletion order by itself.

This is, in fact, possible by working through Child2, Child1, Parent. But, the current 2.14.x code only looks at associations at the table level, deleting child rows before parent. It does not honor that a particular order must be followed between the entities of the child table itself. This is the #10531 issue, and it will be fixed by #10547.

@mpdude commented on GitHub (Mar 6, 2023): I've taken a closer look at this and in fact, what you've found here is a variant of the deletion case from #10531: ```mermaid graph LR; Child1 --> Parent; Child2 --> Parent; Child2 --> Child1; ``` Both associations (from Child to Parent as well as from Child to Child) are `cascade: delete`, so trying to delete the `Parent` instance will setup the entire graph for deletion. All associations are `null`able in the database, but that's of no relevance for the delete case, since the ORM does not currently use an "extra update" before `DELETE` to break cycles. There is no database-level `ON DELETE SET NULL`, so the ORM will have to find a valid deletion order by itself. This is, in fact, possible by working through `Child2`, `Child1`, `Parent`. But, the current `2.14.x` code only looks at associations at the table level, deleting `child` rows before `parent`. It does not honor that a particular order must be followed between the entities of the `child` table itself. This is the #10531 issue, and it will be fixed by #10547.
admin commented 2026-01-22 15:44:22 +01:00
Author
Owner
Copy Link

@Toflar commented on GitHub (Mar 6, 2023):

Wohoo! Thanks so much for the investigation and the awesome work - that PR seems to solve a lot of issues! Kudos @mpdude! Let's hope your work can make it to a release soon!

@Toflar commented on GitHub (Mar 6, 2023): Wohoo! Thanks so much for the investigation and the awesome work - that PR seems to solve a lot of issues! Kudos @mpdude! Let's hope your work can make it to a release soon!
admin commented 2026-01-22 15:44:22 +01:00
Author
Owner
Copy Link

@mpdude commented on GitHub (Mar 6, 2023):

Test case included at 3917692ff8

@mpdude commented on GitHub (Mar 6, 2023): Test case included at https://github.com/doctrine/orm/pull/10547/commits/3917692ff86a4f61c3a10a12b1ee65c6e59b123b
admin commented 2026-01-22 15:44:22 +01:00
Author
Owner
Copy Link

@mpdude commented on GitHub (Mar 6, 2023):

Indeed there is another bug regarding the delete order, which I have filed as #10566

@mpdude commented on GitHub (Mar 6, 2023): Indeed there is another bug regarding the delete order, which I have filed as #10566
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
BC Break
Bug
Can't Fix
Deprecation
Documentation
DQL
Duplicate
Failing Test
Hacktoberfest
Improvement
Improvement
Incomplete
Invalid
Missing Tests
New Feature
PR Created
pull-request
Mirrored from GitHub Pull Request
 Question
Regression
Status: Waiting feedback
Waiting feedback
WIP
Won't Fix
No Label Bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
admin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: doctrine/archived-orm#7085
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?