[RFR] Added "endFlush" event #481
Conversation
|
how does this relate to postFlush? is it maybe just that postFlush is raised a bit too early to genuinely happen after the flush is terminated? at which point is the orm doing the postFlush? |
|
ORM does postFlush here: https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/UnitOfWork.php#L398 i.e. before resetting the state of the UOW - it says explicitly in the documentation to not call "flush" inside |
|
weird. is there something else we could do? like listen on a request terminate event and autogenerate routes only at that point? |
|
Yeah .. I guess that could work (kernel.terminate) - but could we imagine that causing unexpected behavior? Obviously its hacky - I would prefer this option if there was a genuine use case :) So we would persist the autoroutes on |
|
Maybe you should have a look at #476 and rebase on that later with the new event service mechanism. |
|
regarding the endless loop fear: we could say that if a flush is done within the endFlush event, no new event is triggered. this will limit generic cases with cascading effects but would prevent the loop. not sure if its a good idea or creating more confusion and unexpected behaviour. |
|
well, in the RoutingAutoBundle I handle this in the Subscriber, when the event is called I set a flag, and if the flag is set we do not invoke it again. But I like the idea you had about only invoking the event if there are things scheduled in the UOW. |
|
I think that event can be nice and usefull. Just for my understanding: with Order of operation you mean the position of events in an object's lifecycle? It is not the Point of that PR, but here i have a feeling as those positions arn't equal. |
|
@ElectricMaxxx no, the order-of-operations problem is the fact that operations (remove, move, insert, update) are executed in a fixed order in the UOW instead of the order in which they were comitted, which makes certain things (e.g. replacing a document) impossible. |
|
@dantleech you are right this is an other thing, so the scheduled and mapped stuff should need a position. But this causes equal issues on the order events (mostly post*) that where thrown, as they are bound on the order of those operations. |
There was a problem hiding this comment.
is this change needed? seems like $id is not used afterwards. and we prefer to do assignments outside method calls.
There was a problem hiding this comment.
No, I guess I was just debugging. Will fix this and add some tests.
|
i am ok with this to go in here, even though it was refused in the orm. @lsmith77 do you have any objections or concerns? |
|
so in other words +1 for adding this event |
|
I do not really see a better alternative for our very real needs. one of these days there will be ORM 3.x anyway and at that point we will all likely move over to a new UoW structure. |
|
alright. waiting for @dantleech to clarify the question i have on the code, then we can merge. @dantleech can you please also add a note to the changelog and do a PR on the phpcr-odm-documentation repo? |
There was a problem hiding this comment.
Have reimplemented this early exit from the ORM. Unfortunately it doesn't seem to avoid the infinite loop in the EventManagerTest. Will have to come back to it.
There was a problem hiding this comment.
event listeners should also take care to not go into endless loops. but this should work so if you can investigate why it does not, that would probably fix some other issues. btw, is there not some public method to ask the dm / uow if there is anything to flush?
There was a problem hiding this comment.
Couldn't find anything in the PHPCR-ODM code, and the ORM does the same as I have done here.
There was a problem hiding this comment.
one thing: if somebody was calling flush to trigger a phpcr session save after doing raw phpcr operations, this would be a BC break for him. its an edge case but i would still call session save, it should not hurt.
|
hmm, seems that there is always an update computed by computeChangeset which causes the recursive flush. |
|
even on a completely empty uow, or only once there is some document registered? either way, that sounds like a bug. are you investigating? |
- Update the "originalData" to the "actualData" after computing the CS. This means the additional flushes do not register the difference in the actual data (which had previously had fields removed) as a change.
|
ok, so there was a bug - the updates were always executed again when calling flush multiple times. The Doctrine2 seems to update the |
There was a problem hiding this comment.
I would vote for removing this in the next major version.
There was a problem hiding this comment.
agreed. can you open an issue to remind us of that please?
There was a problem hiding this comment.
In accordance with what I said earlier, I don't think we should be calling flush() if we are modifying the PHCPR session directly. The ODM has no knowledge of the inner state of the PHPCR session.
|
thanks a lot. this seems sane to me now. it could also fix issues when people batch update data as the changeset would have grown and grown if they did not call $dm->clear(). can you squash the commits please? then lets merge. btw, this reminds me that i am rather unhappy with the whole UoW codebase - i am pretty sure at least 90% of the logic in there is the same as for the orm, but its all copied and diverging from the orm... |
|
squashed and merged in 92e0c42 thanks a lot dan! |
Just testing the water -- should we have an end flush event?
I am using this event with success in RoutingAutoBundle to persist additional documents (the redirect routes) but this is a workaround for the fact that the order of persistance is wrong (and refactoring the entire UOW is a big task).
Are there any genuine use cases for this event?