Throw deprecation warning on nested flushes

[alcaeus]

As indicated in #1051, calling flush in an event subscriber can have unwanted side effects and shouldn't be done. To prepare people for this change, ODM 1.2 will throw deprecation warnings to give users a chance to fix offending event subscribers before 2.0 starts throwing exceptions.

[malarzm]

We should have a test checking that commitsInProgress is decreased in case of exception or any error.

[alcaeus]

Test added.

[alcaeus]

Looking at #1584, the suggestion is to also deprecate calls to other persistence methods (persist, remove, etc. during event subscriber calls. These calls could be allowed in preFlush and onFlush event listeners, but considered unsafe afterwards because any new operations scheduled might not be executed. What do you think @malarzm?

[malarzm]

I think it will require a lot of ifology as for instance it is safe to call remove on postPersist as removals are last. But lemme think about it a bit more :)

[alcaeus]

I think it will require a lot of ifology as for instance it is safe to call remove on postPersist as removals are last. But lemme think about it a bit more :)

Which is exactly why I wouldn't allow any events at all after onFlush. There are just too many variables for us to figure out whether the event call is allowed, and it gets even worse for users that don't know the inner workings of UnitOfWork. I see it as an opportunity to prevent users from shooting themselves in the foot 😉

[malarzm]

I see it as an opportunity to prevent users from shooting themselves in the foot 😉

While this is a noble thing to do, this also takes away feature that is quite powerful and working without side effects (to some degree of course) :P I mean I can see people wanting to remove a document after successful update of another one, and if somebody is caring enough to go look into UoW's internals to figure out it's OK to do so, I'm happy for them.

[alcaeus]

I can see people wanting to remove a document after successful update of another one

That's a valid use case, although I think people should be handling this in postFlush. However, when postFlush has been called, changeSets have not been cleared, so it isn't save to make another call to flush just yet.

Furthermore, even by looking into UnitOfWork, scheduling a new operation while the flush is in progress is a big no-go as you can't be sure it will be executed (for one, order of operations may forbid it or we may ignore it because we already handled insertions for that type, etc.).

[alcaeus]

Merging without additional deprecations - we can always re-visit this. Ideally we'll find a different solution to the problem at hand.