2.16.0 - Issue with persisting entities #10869
Comments
|
I did some more digging and this seems to be caused by some event listener that does stuff on |
|
Can you create a reproducer for this particular issue? |
|
Similar report here: #10547 (comment) Not sure if it is allowed/supported to call |
yeah thats the question 😄 Nothing is mentioned in the docs |
|
Well actually it says
|
|
Since |
|
b42cf99 migth be related, it makes a change to
|
|
I will rewrite this listener logic on our end. So feel free to close this issue. I think its very tricky to support all those weird edge cases 😛 |
|
Should we prevent I am really afraid what might happen when the UoW has computed its lists of insertions, field updates etc. and then, during the course of the flush, some event listener calls |
|
So I had a look at rewriting the listener logic and its not easy 😢 I would need to hook into the Currently I don't see any way of achieving this then if I cannot |
|
Assuming the |
Indeed I just tested it: that would work 😊 |
|
@mpdude I will try to check it, but as I see it from my mobile phone your reproducer is for single key reference, I am using composite keys. Product offer is partitioned by $shop and $id. This might be the difference we are looking for. |
|
@tasselchof maybe you meant to comment on #10868? |
Yes, sorry once more, didn’t saw it on mobile. |
|
I tried to create a simple reproducer but currently failing with it, the thing I did not yet tried is when the entity is a Atleast the flush on postPersist didn't have any issues here: This test currently runs fine: So maybe really more related to ResolvedTargetListener but would need to test that, if somebody got time before me try to reproduce it on a simpler installation maybe can try it to create on top. |
|
hello everyone, I have the same problem since the update of symfony 6.3.3..It worked very well before, I used postPersist to retrieve the generated ID and trace by creating a log with a flush .. public function postPersist(PostPersistEventArgs $args): void with the following result (incomplete)=> An exception occurred while executing a query: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '?, ?, ?, ?, ?, ?, ?, ?, ?)' at line 1 Doctrine\DBAL\Exception\SyntaxErrorException {#5000 ▼ the error only exists for entities with associations !! I tried to change the logic using onflush and postflush, but I can't get the newly created entity iD. |
This PR prevents reentrant calls to `UnitOfWork::commit()` with a clear exception message. Reentrance happens, for example, when users call `EntityManager::flush()` from within `postPersist` or similar event listeners. Since doctrine#10547, it causes strange-looking, non-helpful error messages (doctrine#10869). Instead of treating this as a bug and trying to fix it, my suggestion is to fail with a clear exception message instead. Reasons: I assume the UoW was never designed to support reentrant `flush()` calls in the first place. So, trying to make (or keep) this working might be a rabbit hole. The assumption is based on the following: * Not a single test in the ORM test suite covers or even triggers `flush()` reentrance – otherwise, such a test would have failed with the changes suggested here. * Documentation for e. g. [`preFlush`](https://www.doctrine-project.org/projects/doctrine-orm/en/2.16/reference/events.html#preflush) or [`postFlush`](https://www.doctrine-project.org/projects/doctrine-orm/en/2.16/reference/events.html#preflush) explicitly mentions that `flush()` must not be called again. I don't know why this is not also stated for e. g. `postPersist`. But why would it be safe to call `flush()` during `postPersist`, in the middle of a transaction, when there are reasons against calling it in `postFlush`, just before final cleanups are made? * Last but not least, entity insertions, computed changesets etc. are kept in fields like `UnitOfWork::$entityChangeSets` or `UnitOfWork::$originalEntityData`. It's all to easy to mess up these states when we re-compute changesets mid-way through a `flush()` – and again, if that were anticipated, I'd assume to find any kind of test coverage.
|
@dmaicher this code linked in this comment uses a Is your use case the same? Or are you changing/adding other objects than the one the |
|
@alexander-schranz I wanted to suggest you could try using But then I realized you need the database-provided ID to fill in the missing object value, so you have to |
workaroundversion 2.16.1cause flush as often as possible |
|
i see a double insert |
|
The problem arises when postPersist event is caused and in it there is a flash |
|
Hello, I have the same issue but only when I'm trying to persist a I don't have this issue with my others listeners which using My controller My entity My Listener Hope it helps |
This PR addresses the issue brought up in doctrine#10869. It happens when users use event listeners – `postPersist` in particular – to make changes to entities and/or persist new entities and finally call `EntityManager::flush()`, while the `UnitOfWork` is currently within the commit phase. There is a discussion in doctrine#10900 whether this kind of reentrance should be deprecated in 2.x and prevented in 3.x. But, in order to prevent complete breakage with the 2.16.0 update, this PR tries to apply a band-aid 🩹. A few things changed inside the UoW commit implementation in 2.16, and for sure this PR does not restore all details of the previous behavior. Take it with a grain of salt. Here's the details. The issue appears when `UoW::commit()` is performing entity insertions, and `postPersist` listener causes `commit()` reentrance when there are pending insertions left. In that situation, the "inner" (reentrant) call will start working through all changesets. Eventually, it finishes with all insertions being performed and `UoW::$entityInsertions` being empty. The entity insertion order, an array computed at the beginning of `UnitOfWork::executeInserts()`, still contains entities that now have been processed already. This leads to the reported failure down the road. The mitigation is to check for this condition and skip such entities. Before doctrine#10547 (pre-2.16), things worked a bit differently: The UoW did not build a list of entities (objects) as the commit order, but a sequence of _classes_ to process. For every entity _class_, it would find all entity instances in `UoW::$entityInsertions` and pass them to the persister in a batch. The persister, in turn, would execute the `INSERT` statements for _all_ those entities _before_ it dispatched the `postInsert` events. That means when a `postInsert` listener was notified pre-2.16, the UoW was in a state where 1) all insertions for a particular class had been written to the database already and 2) the UoW had not yet gathered the next round of entities to process.
|
Hello, I have the same issue when upgrading to |
|
I confirm the same issue, on 2.16.x persisting of secondary entities doesnt seems to work. Which works on 2.15.x. On 2.16.x no changes made on postPersist (other entities on which persist is called) doesnt seems to be flushed without the flush. And with the flush there is error regarding missing parameters. Maybe this is related to? |
|
I run into this problem when using an ApiPlatform state processor. public function process($data, Operation $operation, array $uriVariables = [], array $context = []): Transaction
{
$entityB = $this->entityBRepository->findOneBy(['refId' => $data->refId]);
if ($entityB == null) {
$entityB = (new EntityB())->setRefId($data->refId);
$this->entityManager->persist($entityB);
}
$entity = new EntityA();
$entity
->setEntityB($entityB)
->setReference($data->reference)
->setDate(new DateTimeImmutable($data->tDate))
;
$this->entityManager->persist($entity);
$this->entityManager->flush();
return $entity;
} |
|
Same issue here. In the symfony profiler I can see the INSERT query properly executed the first time with all |
|
Reading previous comments, this may be related to listeners? |
….0 (Take 2) The changes from doctrine#10547, which landed in 2.16.0, cause problems for users that call `EntityManager::flush()` from within `postPersist` event listeners. * When `UnitOfWork::commit()` is re-entered, the "inner" (reentrant) call will start working through all changesets. Eventually, it finishes with all insertions being performed and UoW::$entityInsertions being empty. After return, the entity insertion order, an array computed at the beginning of `UnitOfWork::executeInserts()`, still contains entities that now have been processed already. This leads to a strange-looking SQL error where the number of parameters used does not match the number of parameters bound. This has been reported as doctrine#10869. * The fixes made to the commit order computation may lead to a different entity insertion order than previously. `postPersist` listener code may be affected by this when accessing generated IDs for other entities than the one the event has been dispatched for. This ID may not yet be available when the insertion order is different from the one that was used before 2.16. This has been mentioned in doctrine#10906 (comment). This PR suggests to address both issues by dispatching the `postPersist` event only after _all_ new entities have their rows inserted into the database. Likewise, dispatch `postRemove` only after _all_ deletions have been executed. This solves the first issue because the sequence of insertions or deletions has been processed completely _before_ we start calling event listeners. This way, potential changes made by listeners will no longer be relevant. Regarding the second issue, I think deferring `postPersist` a bit until _all_ entities have been inserted does not violate any promises given, hence is not a BC break. In 2.15, this event was raised after all insertions _for a particular class_ had been processed - so, it was never an "immediate" event for every single entity. doctrine#10547 moved the event handling to directly after every single insertion. Now, this PR moves it back a bit to after _all_ insertions.
….0 (Take 2) The changes from doctrine#10547, which landed in 2.16.0, cause problems for users calling `EntityManager::flush()` from within `postPersist` event listeners. * When `UnitOfWork::commit()` is re-entered, the "inner" (reentrant) call will start working through all changesets. Eventually, it finishes with all insertions being performed and `UoW::$entityInsertions` being empty. After return, the entity insertion order, an array computed at the beginning of `UnitOfWork::executeInserts()`, still contains entities that now have been processed already. This leads to a strange-looking SQL error where the number of parameters used does not match the number of parameters bound. This has been reported as doctrine#10869. * The fixes made to the commit order computation may lead to a different entity insertion order than previously. `postPersist` listener code may be affected by this when accessing generated IDs for other entities than the one the event has been dispatched for. This ID may not yet be available when the insertion order is different from the one that was used before 2.16. This has been mentioned in doctrine#10906 (comment). This PR suggests to address both issues by dispatching the `postPersist` event only after _all_ new entities have their rows inserted into the database. Likewise, dispatch `postRemove` only after _all_ deletions have been executed. This solves the first issue because the sequence of insertions or deletions has been processed completely _before_ we start calling event listeners. This way, potential changes made by listeners will no longer be relevant. Regarding the second issue, I think deferring `postPersist` a bit until _all_ entities have been inserted does not violate any promises given, hence is not a BC break. In 2.15, this event was raised after all insertions _for a particular class_ had been processed - so, it was never an "immediate" event for every single entity. doctrine#10547 moved the event handling to directly after every single insertion. Now, this PR moves it back a bit to after _all_ insertions.
|
Everyone affected by this please try #10915 and report whether it helps |
|
Same problem for me.
I tried it and it works for me ! |
….0 (Take 2) The changes from doctrine#10547, which landed in 2.16.0, cause problems for users calling `EntityManager::flush()` from within `postPersist` event listeners. * When `UnitOfWork::commit()` is re-entered, the "inner" (reentrant) call will start working through all changesets. Eventually, it finishes with all insertions being performed and `UoW::$entityInsertions` being empty. After return, the entity insertion order, an array computed at the beginning of `UnitOfWork::executeInserts()`, still contains entities that now have been processed already. This leads to a strange-looking SQL error where the number of parameters used does not match the number of parameters bound. This has been reported as doctrine#10869. * The fixes made to the commit order computation may lead to a different entity insertion order than previously. `postPersist` listener code may be affected by this when accessing generated IDs for other entities than the one the event has been dispatched for. This ID may not yet be available when the insertion order is different from the one that was used before 2.16. This has been mentioned in doctrine#10906 (comment). This PR suggests to address both issues by dispatching the `postPersist` event only after _all_ new entities have their rows inserted into the database. Likewise, dispatch `postRemove` only after _all_ deletions have been executed. This solves the first issue because the sequence of insertions or deletions has been processed completely _before_ we start calling event listeners. This way, potential changes made by listeners will no longer be relevant. Regarding the second issue, I think deferring `postPersist` a bit until _all_ entities have been inserted does not violate any promises given, hence is not a BC break. In 2.15, this event was raised after all insertions _for a particular class_ had been processed - so, it was never an "immediate" event for every single entity. doctrine#10547 moved the event handling to directly after every single insertion. Now, this PR moves it back a bit to after _all_ insertions.
….0 (Take 2) The changes from doctrine#10547, which landed in 2.16.0, cause problems for users calling `EntityManager::flush()` from within `postPersist` event listeners. * When `UnitOfWork::commit()` is re-entered, the "inner" (reentrant) call will start working through all changesets. Eventually, it finishes with all insertions being performed and `UoW::$entityInsertions` being empty. After return, the entity insertion order, an array computed at the beginning of `UnitOfWork::executeInserts()`, still contains entities that now have been processed already. This leads to a strange-looking SQL error where the number of parameters used does not match the number of parameters bound. This has been reported as doctrine#10869. * The fixes made to the commit order computation may lead to a different entity insertion order than previously. `postPersist` listener code may be affected by this when accessing generated IDs for other entities than the one the event has been dispatched for. This ID may not yet be available when the insertion order is different from the one that was used before 2.16. This has been mentioned in doctrine#10906 (comment). This PR suggests to address both issues by dispatching the `postPersist` event only after _all_ new entities have their rows inserted into the database. Likewise, dispatch `postRemove` only after _all_ deletions have been executed. This solves the first issue because the sequence of insertions or deletions has been processed completely _before_ we start calling event listeners. This way, potential changes made by listeners will no longer be relevant. Regarding the second issue, I think deferring `postPersist` a bit until _all_ entities have been inserted does not violate any promises given, hence is not a BC break. In 2.15, this event was raised after all insertions _for a particular class_ had been processed - so, it was never an "immediate" event for every single entity. doctrine#10547 moved the event handling to directly after every single insertion. Now, this PR moves it back a bit to after _all_ insertions.
….0 (Take 2) The changes from doctrine#10547, which landed in 2.16.0, cause problems for users calling `EntityManager::flush()` from within `postPersist` event listeners. * When `UnitOfWork::commit()` is re-entered, the "inner" (reentrant) call will start working through all changesets. Eventually, it finishes with all insertions being performed and `UoW::$entityInsertions` being empty. After return, the entity insertion order, an array computed at the beginning of `UnitOfWork::executeInserts()`, still contains entities that now have been processed already. This leads to a strange-looking SQL error where the number of parameters used does not match the number of parameters bound. This has been reported as doctrine#10869. * The fixes made to the commit order computation may lead to a different entity insertion order than previously. `postPersist` listener code may be affected by this when accessing generated IDs for other entities than the one the event has been dispatched for. This ID may not yet be available when the insertion order is different from the one that was used before 2.16. This has been mentioned in doctrine#10906 (comment). This PR suggests to address both issues by dispatching the `postPersist` event only after _all_ new entities have their rows inserted into the database. Likewise, dispatch `postRemove` only after _all_ deletions have been executed. This solves the first issue because the sequence of insertions or deletions has been processed completely _before_ we start calling event listeners. This way, potential changes made by listeners will no longer be relevant. Regarding the second issue, I think deferring `postPersist` a bit until _all_ entities have been inserted does not violate any promises given, hence is not a BC break. In 2.15, this event was raised after all insertions _for a particular class_ had been processed - so, it was never an "immediate" event for every single entity. doctrine#10547 moved the event handling to directly after every single insertion. Now, this PR moves it back a bit to after _all_ insertions.
|
I'm using gedmo tree and after persisting a tree, it is persisted in reverse. I had to revert to 2.15.5 for the bug to disappear. 2.17.2 didn't work for me. |
|
Even upgrading to 2.18.1 doesn't fix the problem of reverse persist. |
|
@Herz3h can you provide a reproduce case? |
|
Having this issue also with the latest stable version 2.19.4. The case is similar to this that might help to replicate. Now in case that I do flush() before setting the relation |
BC Break Report
Summary
I have a case in our test suite where entities are not persisted properly anymore. This worked fine on 2.15.
The insert statement does not bind any parameters and fails:
The logs (see
parameters: []):How to reproduce
I cannot provide a small reproducer yet. I had a look and somehow inside the
BasicEntityPersisterthe call toprepareInsertData($entity)with that entity returns an empty array.The setup part of the test code that triggers the error looks like this:
The issue seems to come from the second
$this->em->persist($this->fixtures->createWidgetSettings($account, false));line 🤔@mpdude does this information help at all to pinpoint the issue? I can try to provide a small reproducer
The text was updated successfully, but these errors were encountered: