Added new Events: preMove and postMove #246

Merged
merged 9 commits into from Feb 23, 2013

Projects

None yet

5 participants

@laupiFrpar
Contributor

Some events are missing when a document is moved.

So I added new events:

  • preMove
  • postMove
Pierre-Louis... added some commits Feb 19, 2013
@laupiFrpar laupiFrpar referenced this pull request Feb 19, 2013
Merged

Added missing events. #245

@dbu dbu and 1 other commented on an outdated diff Feb 19, 2013
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
dbu Feb 19, 2013 Member

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.

@dbu dbu and 3 others commented on an outdated diff Feb 19, 2013
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
dbu Feb 19, 2013 Member

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
dbu Feb 19, 2013 Member

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
dantleech Feb 19, 2013 Contributor

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 Feb 19, 2013 Member

@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
dbu Feb 20, 2013 Member

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?

laupiFrpar
laupiFrpar Feb 20, 2013 Contributor

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
dantleech Feb 20, 2013 Contributor

@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
dbu Feb 20, 2013 Member

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 Feb 20, 2013 Member

@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
dantleech Feb 21, 2013 Contributor

@stof ok seems like I wasn't looking at 2.4 commons :) So my question
would then be should we be thinking about replacing all instances of
"getDocument" with "getObject" and "getDocumentManager" with
"getObjectManager"? Will the ORM make this change?

On Wed, Feb 20, 2013 at 03:30:10PM -0800, Christophe Coevoet wrote:

In 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])) {
    

@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).


Reply to this email directly or view it on GitHub.

dbu
dbu Feb 21, 2013 Member

yes exactly, this is a major bc break for our event consumers. good we
did not release 1.0 yet, so we can tell people "told you" :-)
but we still should start the changelog when we do that.

@stof: will doctrine orm 2.3 be compatible with commons 2.4? if not
requiring commons 2.4 would make us incompatible for use in a project
with orm 2.3

dantleech
dantleech Feb 21, 2013 Contributor

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

stof
stof Feb 21, 2013 Member

@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)

Member
dbu commented Feb 19, 2013

i think it makes sense to have those events. they do not exist in orm as
there is no similar operation there.

@dbu dbu and 2 others commented on an outdated diff Feb 19, 2013
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
dbu Feb 19, 2013 Member

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.

laupiFrpar
laupiFrpar Feb 20, 2013 Contributor

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

dbu
dbu Feb 20, 2013 Member

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.

laupiFrpar
laupiFrpar Feb 20, 2013 Contributor

ok

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

dbu
dbu Feb 20, 2013 Member

perfect! and the same with postMove of course.

Contributor

I fix following your recommandations

Pierre-Louis LAUNAY Fix typo 2f72dcb
Member
dbu commented Feb 20, 2013

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

Contributor

ok, I will make a new PR tomorrow :)

Pierre-Louis LAUNAY Added tests d0cc0f7
@laupiFrpar laupiFrpar referenced this pull request in doctrine/phpcr-odm-documentation Feb 21, 2013
Merged

Added documentation for preMove and postMove events #20

Contributor

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

Regards,

Contributor

What problems do you have running the tests?
On Thu, Feb 21, 2013 at 06:13:57AM -0800, Pierre-Louis LAUNAY wrote:

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

Regards,


Reply to this email directly or view it on GitHub.

Pierre-Louis LAUNAY Fix fatal error on test 426d746
Contributor

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
Contributor

Yeah, you will need to install composer (which you probably already have):

http://getcomposer.org/download/

and have it in your path - if your using linux, try copying composer.phar to
/usr/bin/composer

On Thu, Feb 21, 2013 at 08:48:40AM -0800, Pierre-Louis LAUNAY wrote:

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


Reply to this email directly or view it on GitHub.

Contributor

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 :)

Contributor

It is ok

Member
dbu commented Feb 22, 2013

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.

Contributor

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

I'm sorry...

Pierre-Louis LAUNAY Fix tests 7e33650
Contributor

It is OK now :)

@dbu dbu and 1 other commented on an outdated diff Feb 22, 2013
...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
dbu Feb 22, 2013 Member

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?

laupiFrpar
laupiFrpar Feb 22, 2013 Contributor

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.

@dbu dbu commented on the diff Feb 22, 2013
...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
dbu Feb 22, 2013 Member

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.

@dbu dbu merged commit efe8417 into doctrine:master Feb 23, 2013

1 check failed

default The Travis build failed
Details
Member
dbu commented Feb 23, 2013

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

Member
dbu commented Feb 27, 2013

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

Member

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