Do not throw exception on checkState if item is deleted / set node clean after save/update. #309
Conversation
| @@ -636,10 +636,6 @@ protected function checkState() | |||
| $this->postDirtyState = -1; | |||
| } | |||
|
|
|||
| if ($this->state === self::STATE_DELETED) { | |||
| throw new InvalidItemStateException('Item '.$this->path.' is deleted'); | |||
| } | |||
There was a problem hiding this comment.
So I guess this implies that DELETED is a valid state until you try and manipulate the node.
There was a problem hiding this comment.
the problem with this is that when you manipulate the node, it calls checkState to make sure it can be manipulated.
looking at the stack trace in https://travis-ci.org/jackalope/jackalope-doctrine-dbal/jobs/119277489 i think the problem is something else. https://github.com/jackalope/jackalope/blob/check_state_delete_no_exception/src/Jackalope/ObjectManager.php#L1124 assumes that a node that is dirty is not deleted (because both are states). but then ->getIdentifier will trigger checkState which detects the node is deleted. i think before the deleted check in the object manager, we should check for dirty and if the node is dirty call refresh($keepChanges). then the state will not be dirty anymore but deleted or new or modified and things work as expected i'd hope.
There was a problem hiding this comment.
if you call refresh repeatedly, things will become very expensive as everything that is dirty is reloaded.
it smells fishy when a node is still dirty after a save() operations, btw. if that is the case there might be some other bug too.
dantleech
force-pushed
the
check_state_delete_no_exception
branch
from
418a145
to
a5b08cf
Compare
| @@ -869,6 +869,7 @@ public function save() | |||
| if ($item->isModified() || $item->isMoved()) { | |||
| $item->confirmSaved(); | |||
| } | |||
| $item->setClean(); | |||
There was a problem hiding this comment.
This is the change.
There was a problem hiding this comment.
i think we did not do this so that auto generated things would be found when reloading (jackrabbit might autocreate some things)
There was a problem hiding this comment.
Not sure I understand: do you mean if the node contains some auto-created properties? i.e. if it is dirty then the state will be reloaded somehow?
Otherwise do you think this is the correct behavior? If not, any ideas as to what we can do to fix the issue?
There was a problem hiding this comment.
i think we on purpose mark the node dirty when we confirm it its saved. there are autocreated properties in the backend, e.g. the uuid if you did not force creation already.
i wrote about the solution that i expect to be correct in a comment that is now hidden:
looking at the stack trace in https://travis-ci.org/jackalope/jackalope-doctrine-dbal/jobs/119277489 i think the problem is something else. https://github.com/jackalope/jackalope/blob/check_state_delete_no_exception/src/Jackalope/ObjectManager.php#L1124 assumes that a node that is dirty is not deleted (because both are states). but then ->getIdentifier will trigger checkState which detects the node is deleted. i think before the deleted check in the object manager, we should check for dirty and if the node is dirty call refresh($keepChanges). then the state will not be dirty anymore but deleted or new or modified and things work as expected i'd hope.
There was a problem hiding this comment.
@dantleech can we do the solution i propose in my last comment, or does that lead to some other problem?
|
@dbu have updated - now the only change is setting the node to clean after updating it with the transport, seems to fix the issue. When the user later tries to access a property of the (deleted) node they will receive the usual InvalidItemStateException. |
dantleech
changed the title
Set the node as clean after updating the node via. transport during the
save()operation.See related: