-
-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preserve order of operations #620
base: 2.x
Are you sure you want to change the base?
Conversation
@@ -813,7 +800,7 @@ private function doScheduleInsert($document, &$visited, $overrideIdGenerator = n | |||
// TODO: Change Tracking Deferred Explicit | |||
break; | |||
case self::STATE_REMOVED: | |||
unset($this->scheduledRemovals[$oid]); | |||
$this->treeOpQueue->unqueue(TreeOperation::OP_REMOVE, $oid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure about the validity of this. If the Document was previously removed, and then re-inserted we remove all REMOVE operations from the stack. Not sure if that could conflict somehow with MOVE or REMOVE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want to record operations, i think you would need to queue another add operation instead. i am sure it would be possible to find some hairbrained scenario where this makes a difference. and probably some very real usecase with listeners doing many things could actually result in the same scenario and actually make sense...
@dbu any idea why this is not working on Jackrabbit? Something on our end? |
Jackalope-jackrabbit sends the following POST:
Resulting in:
If I catch the |
@@ -927,16 +914,22 @@ public function scheduleMove($document, $targetPath) | |||
|
|||
switch ($state) { | |||
case self::STATE_NEW: | |||
unset($this->scheduledInserts[$oid]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a behaviour change i think, if we previously auto-persisted documents upon move operations.
interesting! about the jackrabbit issue: can you try to do that in a phpcr-api-test (CombinedOperationsTest i guess) to see if we have a bug there? i think there is a philosophical question we need to decide. as i understood, doctrine calculates the optimal path to transform an object model into database operations, and does not record changes procedurally. but if we don't manage to calculate this transition, maybe explicit ordered operations make more sense? |
This PR is aiming to preserve the order of operations, it tries to do this whilst preserving the current behavior as much as possible.
When flush is called:
execute*
methods are called with batches of operations, e.g. INSERT, INSERT, INSERT would be a single batch, however INSERT, INSERT, MOVE, INSERT would be three batches, so that the move is executed after the first 2 inserts.