Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Added new Events: preMove and postMove #246

Merged
merged 9 commits into from

5 participants

@laupiFrpar

Some events are missing when a document is moved.

So I added new events:

  • preMove
  • postMove
@laupiFrpar laupiFrpar referenced this pull request
Merged

Added missing events. #245

lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -1926,7 +1927,7 @@ private function executeUpdates($documents, $dispatchEvents = true)
*
* @param array $documents array of all to be moved documents
*/
- private function executeMoves($documents)
+ private function executeMoves($documents, $dispatchEvents = true)
@dbu Collaborator
dbu added a note

do we ever call this method with dispatchEvents false? if not i think we should not have that parameter. we can refactor later if we actually need it.

@lsmith77 Owner

agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -1935,6 +1936,15 @@ private function executeMoves($documents)
list($document, $targetPath) = $value;
+ if ($dispatchEvents) {
+ if (isset($class->lifecycleCallbacks[Event::preMove])) {
@dbu Collaborator
dbu added a note

i wonder if we should not factor out those 4 lines into a custom method. #245 is adding new classes for the event - but i think we should only do that if we need specific event arguments. @dantleech what do you think about that, would it make sense to refactor that code?

just having

$this->triggerEvent(Event::preMove, $document);

would be more compact and less distracting.

@dbu Collaborator
dbu added a note

if we need specific event classes for each event, the value of a triggerEvent method has less value. though we could keep a method needsEvent just for the double if statement, could still compact the code a little bit. wdyt?

@dantleech Collaborator

Yeah, I don't see any particular value in having classes for each event - I am just trying to be make things as similar as possible with the ORM.

Interestingly I also found the event classes in commons (https://github.com/doctrine/common/tree/master/lib/Doctrine/Common/Persistence/Event) but I thought it would have been strange to use them as they talk about Entities and use ->getObjectManager(). I don't know if at somepoint in the future all of the Doctrine projects will use ObjectManager as a standard.

Actually, I just had a quick look at the ORM events and there is some differentiating logic between the various non-lifecycle events, whereas currently we do not have any differentiating logic, onClear, onFlush, postFlush and preFlush are the same. So +1 for dropping them. We can subclass the replacement (hmm - UnitOfWorkEvent?, DocumentManagerEvent?)

@stof
stof added a note

@dantleech ObjectManager is the name of the interface implemented by the ORM EntityManager and the ODM DocumentManager. And IIRC, there is getObject as alias for getEntity in Common master

@dbu Collaborator
dbu added a note

i created http://www.doctrine-project.org/jira/browse/PHPCR-96 for this. @stof can you confirm there if it makes sense to use the classes from common where applicable and otherwise extend those?

So, I remove the test of lifecycleCallbacks or I leave as is ?

If preMove and postMove events has a specific event object, it is not make sense to test the lifecycleCallbacks of the $class.

@dantleech Collaborator

@laupiFrpar I think we could refactor the lifecycle callbacks test later, as your code works the same way as all the other event invocations., just my opinion.
@stof, @dbu The "problem" I see with using the Common events is that the methods are hardcoded with getObjectManager and getEntity, whereas in the ODM we talk about getDocumentManager and getDocument.

@dbu Collaborator
dbu added a note

lets not do anything in this PR but discuss in http://www.doctrine-project.org/jira/browse/PHPCR-96 if we can improve something. using the common event classes would indeed mean a BC break so if we want to do it at all, we should do it soon. i think lifecycleCallbacks are all events the document might listen to, also things like a move.

@stof
stof added a note

@dantleech The ORM 2.4 is extending the common classes providing deprecated BC methods.
and as said previously, you have getObject, not only getEntity (the other ODM teams already saw this).

As the PHPCR ODM is not stable yet, I would suggest keeping the BC layer only for a short time (removing it before going stable).

@dantleech Collaborator
@dbu Collaborator
dbu added a note
@dantleech Collaborator

I have created a WIP PR #248 which is probably a better place to continue this discussion :)

@stof
stof added a note

@dbu I'm not sure

for the BC break, you could keep a child class with getDocumentManager temporarily (triggering E_USER_DEPRECATED) to give some time to update

@dantleech The ORM 2.4 is using Common 2.4 so these getters are available. But as the ORM is stable since a long time now, it also keeps getEntity and getEntityManager for BC (they will be removed only in 3.0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu
Collaborator
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -1935,6 +1936,15 @@ private function executeMoves($documents)
list($document, $targetPath) = $value;
+ if ($dispatchEvents) {
+ if (isset($class->lifecycleCallbacks[Event::preMove])) {
+ $class->invokeLifecycleCallbacks(Event::preMove, $document);
+ }
+ if ($this->evm->hasListeners(Event::preMove)) {
+ $this->evm->dispatchEvent(Event::preMove, new LifecycleEventArgs($document, $this->dm));
@dbu Collaborator
dbu added a note

actually we probably want a specific move event object that contains the source and target paths in addition to the document. that could be handy information for the event handler.

@lsmith77 Owner

agreed

You mean I create a new object like $this->evm->dispatchEvent(Event::preMove, new MoveEventArgs($document, $targetPath, $this->dm)); ?

@dbu Collaborator
dbu added a note

almost. i think we also want the $sourcePath, not only the $targetPath. i guess in the post move at least the document does not have the sourePath as id anymore.

ok

$this->evm->dispatchEvent(Event::preMove, new MoveEventArgs($document, $sourcePath, $targetPath, $this->dm)); is ok ?

@dbu Collaborator
dbu added a note

perfect! and the same with postMove of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@laupiFrpar

I fix following your recommandations

@dbu
Collaborator

this looks fine now, thanks.

can you please add tests, to EventComputingTest and maybe EventManagerTest? then we can merge.

and it would be cool if you can do a PR against the doc as well, will be similar to doctrine/phpcr-odm-documentation#18

@laupiFrpar

ok, I will make a new PR tomorrow :)

@laupiFrpar laupiFrpar referenced this pull request in doctrine/phpcr-odm-documentation
Merged

Added documentation for preMove and postMove events #20

@laupiFrpar

I added some tests but I cannot test on my computer because I never launch the test of doctrine with succcess.

Regards,

@dantleech
Collaborator
@laupiFrpar

After install with composer.phar, I launch ./tests/travis_jackrabbit.sh and I get the following error:

./tests/travis_jackrabbit.sh: line 3: composer: command not found
./tests/travis_jackrabbit.sh: line 18: ./vendor/jackalope/jackalope-jackrabbit/bin/jackrabbit.sh: No such file or directory
PHP Fatal error: Class 'Jackalope\Tools\Console\Command\Jackrabbitcommand' in /[...]cli-config.php on line 11
@dantleech
Collaborator
@laupiFrpar

Thanks.

I'm working on mac and I do not have wget command. (/vendor/jackalope/jackalope-jackrabbit/bin/jackrabbit.sh line 13)

So I am actually launching the cURL command manually :)

@laupiFrpar

It is ok

@dbu
Collaborator

either the test or even the code has some bug:

Doctrine\Tests\ODM\PHPCR\Functional\EventComputingTest::testComputingBetweenEvents

    PHPCR\PathNotFoundException: HTTP 409: /.getAncestor(1)

this looks like somewhere there is a string instead of a method being called. i don't see getAncestor in your PR so i am confused as to what happens here. can you investigate?

the test fails (2 error, 3 fails) about multilang are normal, they will be fixed in the pull request about cascading locale information.

@laupiFrpar

Ok I investigate now. I thought this is normal since some somedays...

I'm sorry...

@laupiFrpar

It is OK now :)

...ine/Tests/ODM/PHPCR/Functional/EventComputingTest.php
@@ -60,6 +60,20 @@ public function testComputingBetweenEvents()
$user = $this->dm->find('Doctrine\Tests\Models\CMS\CmsUser', $user->id);
$this->assertTrue($user->name=='preupdate');
+ // Move test
+ $targetPath = '/' . $user->name;
+ $this->dm->move($user, $targetPath);
@dbu Collaborator
dbu added a note

i am confused a bit: is this move from /preupdate to /preupdate, a noop? and why is the preMove listener changing the name of the document? would that not mean the move target should change? but then its like with onFlush events, altering the document is not taken into account anymore?

No, before the move action, $user have the ID /functional/preupdate and I move to /preupdate.

I change the name of the document and I can test if this name is not changed after moved. That prove the move action do not change the document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff
...ine/Tests/ODM/PHPCR/Functional/EventComputingTest.php
@@ -100,4 +116,16 @@ public function postUpdate(EventArgs $e)
$document->username = 'postupdate';
}
+ public function preMove(EventArgs $e)
+ {
+ $this->preMove = true;
+ $document = $e->getDocument();
+ $document->name = 'premove';
@dbu Collaborator
dbu added a note

you could set username to 'premove' and in postmove do $document->username .= '-postmove' and check if username became premove-postmove. that would check the proper order of events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu merged commit efe8417 into from
@dbu
Collaborator

thank you for the PR. i cleaned up the tests a bit in a19a5cf

@dbu
Collaborator

i start wondering if we can't trigger preMove much earlier, so that listeners could still change fields or even add other documents as reactions to the move (for example a redirectroute for the location from which the route document is being moved away...)

/cc @lsmith77

@lsmith77
Owner

right now we trigger the events in the beginning of the relevant execute..() methods. i think trying to move this earlier is however imho tricky .. especially if we trigger move earlier, then we might trigger other events which in turn also wish to do more moves etc .. guess we simply need to accept that in some cases we need to use a post flush to trigger another flush ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
2  lib/Doctrine/ODM/PHPCR/Event.php
@@ -26,9 +26,11 @@
const prePersist = 'prePersist';
const preRemove = 'preRemove';
const preUpdate = 'preUpdate';
+ const preMove = 'preMove';
const postRemove = 'postRemove';
const postPersist = 'postPersist';
const postUpdate = 'postUpdate';
+ const postMove = 'postMove';
const postLoad = 'postLoad';
const postFlush = 'postFlush';
const preFlush = 'preFlush';
View
103 lib/Doctrine/ODM/PHPCR/Event/MoveEventArgs.php
@@ -0,0 +1,103 @@
+<?php
+
+/*
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\ODM\PHPCR\Event;
+
+/**
+ * MoveEventArgs
+ */
+class MoveEventArgs extends \Doctrine\Common\EventArgs
+{
+ /**
+ * @var object
+ */
+ private $document;
+
+ /**
+ * @var \Doctrine\ODM\PHPCR\DocumentManager
+ */
+ private $dm;
+
+ /**
+ * @var string
+ */
+ private $sourcePath;
+
+ /**
+ * @var string
+ */
+ private $targetPath;
+
+ /**
+ * Constructor
+ *
+ * @param object $document The object
+ * @param \Doctrine\ODM\PHPCR\DocumentManager $dm The document manager
+ * @param string $sourcePath The source path
+ * @param string $targetPath The target path
+ */
+ public function __construct($document, $dm, $sourcePath, $targetPath)
+ {
+ $this->document = $document;
+ $this->dm = $dm;
+ $this->sourcePath = $sourcePath;
+ $this->targetPath = $targetPath;
+ }
+
+ /**
+ * Get the document
+ *
+ * @return object
+ */
+ public function getDocument()
+ {
+ return $this->document;
+ }
+
+ /**
+ * Get the document manager
+ *
+ * @return \Doctrine\ODM\PHPCR\DocumentManager
+ */
+ public function getDocumentManager()
+ {
+ return $this->dm;
+ }
+
+ /**
+ * Get the source path
+ *
+ * @return string
+ */
+ public function getSourcePath()
+ {
+ return $this->sourcePath;
+ }
+
+ /**
+ * Get the target path
+ *
+ * @return string
+ */
+ public function getTargetPath()
+ {
+ return $this->targetPath;
+ }
+}
View
28 lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -30,6 +30,7 @@
use Doctrine\ODM\PHPCR\Event\PreFlushEventArgs;
use Doctrine\ODM\PHPCR\Event\PostFlushEventArgs;
use Doctrine\ODM\PHPCR\Event\OnClearEventArgs;
+use Doctrine\ODM\PHPCR\Event\MoveEventArgs;
use Doctrine\ODM\PHPCR\Proxy\Proxy;
use Jackalope\Session as JackalopeSession;
@@ -640,6 +641,7 @@ public function scheduleMove($document, $targetPath)
$oid = spl_object_hash($document);
$state = $this->getDocumentState($document);
+
switch ($state) {
case self::STATE_NEW:
unset($this->scheduledInserts[$oid]);
@@ -1947,12 +1949,20 @@ private function executeMoves($documents)
list($document, $targetPath) = $value;
- $path = $this->getDocumentId($document);
- if ($path === $targetPath) {
+ $sourcePath = $this->getDocumentId($document);
+ if ($sourcePath === $targetPath) {
continue;
}
- $this->session->move($path, $targetPath);
+ if (isset($class->lifecycleCallbacks[Event::preMove])) {
+ $class->invokeLifecycleCallbacks(Event::preMove, $document);
+ }
+
+ if ($this->evm->hasListeners(Event::preMove)) {
+ $this->evm->dispatchEvent(Event::preMove, new MoveEventArgs($document, $this->dm, $sourcePath, $targetPath));
+ }
+
+ $this->session->move($sourcePath, $targetPath);
// update fields nodename and parentMapping if they exist in this type
$class = $this->dm->getClassMetadata(get_class($document));
@@ -1966,11 +1976,11 @@ private function executeMoves($documents)
// update all cached children of the document to reflect the move (path id changes)
foreach ($this->documentIds as $oid => $id) {
- if (0 !== strpos($id, $path)) {
+ if (0 !== strpos($id, $sourcePath)) {
continue;
}
- $newId = $targetPath.substr($id, strlen($path));
+ $newId = $targetPath.substr($id, strlen($sourcePath));
$this->documentIds[$oid] = $newId;
$document = $this->getDocumentById($id);
@@ -1991,6 +2001,14 @@ private function executeMoves($documents)
}
}
}
+
+ if (isset($class->lifecycleCallbacks[Event::postMove])) {
+ $class->invokeLifecycleCallbacks(Event::postMove, $document);
+ }
+
+ if ($this->evm->hasListeners(Event::postMove)) {
+ $this->evm->dispatchEvent(Event::postMove, new MoveEventArgs($document, $this->dm, $sourcePath, $targetPath));
+ }
}
}
View
31 tests/Doctrine/Tests/ODM/PHPCR/Functional/EventComputingTest.php
@@ -21,7 +21,7 @@ public function setUp()
$this->listener = new TestEventDocumentChanger();
$this->dm = $this->createDocumentManager();
$this->node = $this->resetFunctionalNode($this->dm);
- $this->dm->getEventManager()->addEventListener(array('prePersist', 'postPersist', 'preUpdate', 'postUpdate'), $this->listener);
+ $this->dm->getEventManager()->addEventListener(array('prePersist', 'postPersist', 'preUpdate', 'postUpdate', 'preMove', 'postMove'), $this->listener);
}
public function testComputingBetweenEvents()
@@ -60,6 +60,20 @@ public function testComputingBetweenEvents()
$user = $this->dm->find('Doctrine\Tests\Models\CMS\CmsUser', $user->id);
$this->assertTrue($user->name=='preupdate');
+ // Move test, Before move the path is /functional/preudpate and I move to /preupdate
+ $targetPath = '/' . $user->name;
+ $this->dm->move($user, $targetPath);
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $this->assertTrue($user->username == 'premove-postmove');
+
+ $user = $this->dm->find('Doctrine\Tests\Models\CMS\CmsUser', $targetPath);
+
+ // The document is moved and do not be modified
+ $this->assertTrue($user->name == 'preupdate');
+ $this->assertTrue($this->listener->preMove);
+
// Clean up
$this->dm->remove($user);
$this->dm->flush();
@@ -74,6 +88,8 @@ class TestEventDocumentChanger
public $postUpdate = false;
public $preRemove = false;
public $postRemove = false;
+ public $preMove = false;
+ public $postMove = false;
public $onFlush = false;
public function prePersist(EventArgs $e)
@@ -100,4 +116,17 @@ public function postUpdate(EventArgs $e)
$document->username = 'postupdate';
}
+ public function preMove(EventArgs $e)
+ {
+ $this->preMove = true;
+ $document = $e->getDocument();
+ $document->name = 'premove'; // I try to update the name of the document but after move, the document should never be modified
@dbu Collaborator
dbu added a note

you could set username to 'premove' and in postmove do $document->username .= '-postmove' and check if username became premove-postmove. that would check the proper order of events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ $document->username = 'premove';
+ }
+
+ public function postMove(EventArgs $e)
+ {
+ $document = $e->getDocument();
+ $document->username .= '-postmove';
+ }
}
View
27 tests/Doctrine/Tests/ODM/PHPCR/Functional/EventManagerTest.php
@@ -28,8 +28,9 @@ public function setUp()
$this->dm = $this->createDocumentManager();
$this->node = $this->resetFunctionalNode($this->dm);
$this->dm->getEventManager()->addEventListener(array(
- 'prePersist', 'postPersist', 'preUpdate', 'postUpdate',
- 'preRemove', 'postRemove', 'onFlush', 'postFlush', 'preFlush'
+ 'prePersist', 'postPersist', 'preUpdate', 'postUpdate',
+ 'preRemove', 'postRemove', 'onFlush', 'postFlush', 'preFlush',
+ 'preMove', 'postMove'
), $this->listener);
}
@@ -59,7 +60,13 @@ public function testTriggerEvents()
$this->assertFalse($this->listener->pagePostRemove);
$this->assertFalse($this->listener->itemPreRemove);
$this->assertFalse($this->listener->itemPostRemove);
-
+
+ $this->dm->move($page, '/' . $page->title);
+ $this->dm->flush();
+
+ $this->assertTrue($this->listener->preMove);
+ $this->assertTrue($this->listener->postMove);
+
$item = new CmsItem();
$item->name = "my-item";
$item->documentTarget = $page;
@@ -108,7 +115,9 @@ class TestPersistenceListener
public $onFlush = false;
public $postFlush = false;
public $preFlush = false;
-
+ public $preMove = false;
+ public $postMove = false;
+
public function prePersist(EventArgs $e)
{
$document = $e->getDocument();
@@ -168,6 +177,16 @@ public function postRemove(EventArgs $e)
}
}
+ public function preMove(EventArgs $e)
+ {
+ $this->preMove = true;
+ }
+
+ public function postMove(EventArgs $e)
+ {
+ $this->postMove = true;
+ }
+
public function onFlush(EventArgs $e)
{
$this->onFlush = true;
Something went wrong with that request. Please try again.