Skip to content

fix: calling scheduleForInsert twice#11835

Merged
greg0ire merged 4 commits intodoctrine:2.20.xfrom
gseidel:fix-pre-persist-call-persist
Aug 7, 2025
Merged

fix: calling scheduleForInsert twice#11835
greg0ire merged 4 commits intodoctrine:2.20.xfrom
gseidel:fix-pre-persist-call-persist

Conversation

@gseidel
Copy link
Copy Markdown
Contributor

@gseidel gseidel commented Feb 14, 2025

If scheduleForInsert was called in prePersist hook already, then persistNew need to check this case first, otherwise a ORMInvalidArgumentException will be thrown.

Describing my use case:

Trying to implement some "two column polymorphism", see discusson #9928.
So I want to call a persist on an unmapped property. This works fine for most cases. But if the entity inside the unmapped property has some cascade persist to the entity carrying the unmapped property, then the scheduleForInsert will be called twice. Once for the cascading inside the hook and later on the persistNew which called the hook at first. You may see the use case on the provided functional test.

If scheduleForInsert was called in prePersist hook already, then persistNew need to check this case first, otherwise a ORMInvalidArgumentException will be thrown
@gseidel
Copy link
Copy Markdown
Contributor Author

gseidel commented Feb 14, 2025

I originally started at branch 3.3.x, but the bug exists also in 2.x. I just saw now, that the tests not working at 2.x. Before I fix the test I will wait for some review to fix all at once.

@gseidel
Copy link
Copy Markdown
Contributor Author

gseidel commented Mar 25, 2025

Tests are already fixed, maybe someone has time to review.

@github-actions
Copy link
Copy Markdown
Contributor

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label Jun 23, 2025
@gseidel
Copy link
Copy Markdown
Contributor Author

gseidel commented Jun 23, 2025

PR still need review

@github-actions github-actions bot removed the Stale label Jun 24, 2025
@gseidel
Copy link
Copy Markdown
Contributor Author

gseidel commented Jul 15, 2025

@greg0ire May you have a look at this PR please.

ostrolucky
ostrolucky previously approved these changes Jul 25, 2025
@greg0ire
Copy link
Copy Markdown
Member

otherwise a ORMInvalidArgumentException will be thrown.

From here I suppose:

orm/src/UnitOfWork.php

Lines 1392 to 1394 in 0f32569

if (isset($this->entityInsertions[$oid])) {
throw ORMInvalidArgumentException::scheduleInsertTwice($entity);
}

Copy link
Copy Markdown
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few nits

@greg0ire greg0ire added this to the 2.20.6 milestone Aug 7, 2025
@greg0ire greg0ire merged commit 04694a9 into doctrine:2.20.x Aug 7, 2025
74 checks passed
@greg0ire
Copy link
Copy Markdown
Member

greg0ire commented Aug 7, 2025

Thanks @gseidel !

@beberlei
Copy link
Copy Markdown
Member

@greg0ire I dont understand why we need this, the fix here is to configure the relationship to cascade persist or not? The listener in the test re-implements cascade persist it seems to me.

@greg0ire
Copy link
Copy Markdown
Member

@beberlei you can't configure the relationship to cascade persist, because there isn't a relationship, since the property is unmapped (by design). That's my understanding at least, but maybe @gseidel can explain it better.

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.

5 participants