Allow Event Listeners the ability to modify context information in the event

[blockjon]

I am trying to automatically add a useful MongoDB $comment (http://docs.mongodb.org/manual/reference/operator/meta/comment/) to my project whenever a Mongo update takes place. In order to do this, it appears that I will need to slightly modify the query before it is executed against the mongo driver.

It appears that the most elegant way for me to do this would be to register a listener on the collectionPreUpdate event within my project with code that looks roughly like this:

+    public function collectionPreUpdate(UpdateEventArgs $args)
+    {
+        $query = $args->getQuery();
+        $comment = $this->createUsefulComment();
+        $query['$comment'] = $comment;
+        $args->setQuery($query);
+    }

Doctrine/mongodb was not designed to allow the event hooks to modify the query as part of the events system. However, I submit to you all that this is a situation in where allowing me to customize the query before it's executed is extremely useful. I'm submitting this PR as an example of something I'm trying to accomplish and asking for your advise on if there is a better way to do this currently, or, if perhaps we should have something like this built into the repo.

[malarzm]

Personally I'm 👍

[blockjon]

Any chance we can prioritize this? I've been on a private fork for a while now and this has been an extremely valuable technique for us. I'd like to get back on the main line.

[alcaeus]

Sorry, I was looking at this the other day but didn't get around with it. While I'm not sure what (besides "we're too slow to implement new MongoDB features" ;)) it could be used for, I don't see a problem adding this. My only question is, why only for update? Why not add it for all other query events as well?

[blockjon]

It may be useful for the other query types. In my use case, it seemed this technique was useful for setting a $comment on my write queries.

[blockjon]

Shall I take a stab at completing this feature? We'd like to see this make it into the next tag that Doctrine ships.

[alcaeus]

Sure, it's a good idea! I don't have time to do this myself, but

[malarzm]

[jmikola]

[beberlei]

don't want to spoil all the fun, but this requires tests :-)

[blockjon]

I just submitted a revision to the PR. I tried to implement every single Event that receives context information which could be manipulated before processing in Doctrine\MongoDB\Collection.

[alcaeus]

After a first look, I think this definitely needs tests to ensure the changed values are used for queries, not the originals.

[blockjon]

@alcaeus My strategy in the PR was to find any place where there's a "do" command such as doGroup, doFindOne() etc were the listener received variables that are subsequently passed into the "do" function.

For tests, each event class that I changed now has tests (such as in FindEventArgsTest.php) to ensure that if a change is made (via my new set functions) that it is correctly returned when the getter is called.

The only thing I'm not specifically testing for (and I don't think the current codebase is testing for either) is if the final integration works between the Collection.php class and the Event Listener calls I modified.

I suppose I could setup a test where I register a listener on every one of the functions I modified where my listener changes each of the variables and then I mock out the "do" function in order to ensure that the change is detected. If you think that is necessary, please say so and I'll do it.

[alcaeus]

I'm not sure you'd need a mock; I think functional tests should do:

• register event subscriber designed to change query (e.g.: ['foo' => 'bar'] is changed to ['foo' => 'foo'])
• run query against basic data (e.g.: ['foo' => 'bar'])
• ensure result matches the changed query, not the original

Since this is the center of your logic, there should be tests for it as @beberlei mentioned :)

[blockjon]

Sorry if this sounds pedantic but I think the proper test here would not need to fire a real database query. I think the test should be able to prove that "If an event handler tries to change the $query, $newObj, $options, or whatever that was changed via my new event setters, that by the time the do() function fires, we receive the changed value."

[blockjon]

Just pushed the rest of the tests to this PR: 3ea9c65

[alcaeus]

Even though the rest of the methods don't have docblocks, I think we can improve that one and add docblocks to the event args classes. Would you mind doing that?

[blockjon]

Done.

[alcaeus]

Not to be pedantic, but you're missing a @since tag here :)

[blockjon]

Should I add @SInCE on all of the functions that I added?

[alcaeus]

Yes, please. That helps people identify when stuff was added.

[blockjon]

ok all set

[alcaeus]

Since we add new public methods in a backwards compatible way, Semantic Versioning requires this change to be in a minor version increment. That makes it @since 1.3.

[blockjon]

Done

[blockjon]

I just confirmed that this change set works with my app when I try to use it to set a mongodb $comment.

[alcaeus]

👍

FWIW, once this is released you might as well use the comment method in query builder:
https://github.com/doctrine/mongodb/blob/master/lib/Doctrine/MongoDB/Query/Builder.php#L307

[blockjon]

Seeking confirming from the maintainers that there are no additional changes requested.

[alcaeus]

I was waiting for @jmikola to take a look at this before merging it. Due to conference travels this might take another few days.

[blockjon]

Kindly seeking review by @jmikola when he's got some time. Thank you! ;)

[blockjon]

Kindly seeking review by @jmikola .. thank you

[jmikola]

Since EventArgs doesn't take its arguments by reference, I wanted to confirm that this would work with batchInsert()'s by-reference parameter. According to https://3v4l.org/4O6VL, re-assigning $a seems to have the desired effect of passing the changes back on to the calling context (I'm admittedly a bit rusty on the ins and outs of PHP references :)

I'll assume this is fine for insert() and save() as well.

[blockjon]

I also believe this will work as desired.

[jmikola]

Left some notes above for small fixes, but nothing major. One separate concern (not tied to a particular line of code) is that we already have MutableEventArgs to allow listeners for change results from the Collection methods. Additionally, that class has logic to check if some event data has actually changed (calculated in setData()). The class name also made sense at the time because EventArgs and its various sub-classes were all immutable (much like other events in DBAL, ORM, and ODM).

I wasn't able to find any events in DBAL and ORM for modifying database operations (i.e. queries), so there's probably not a similar API to compare this to -- I'm also not sure if/how they allow services to hook in and modify queries.

In any event, I wonder if we should leave EventArgs as-is and just start using MutableEventArgs, since the class already exists. Then the immutable EventArgs would remain in place for its other use cases, such as connection events. Beyond that, is there value in having isXXXChanged() methods for the various other event classes we're modifying here?

[blockjon]

@jmikola As for the MutableEventArgs vs. EventArgs, that's your call.

In my case, I have already done the full regression test with this branch against my project and I can see everything works as expected so I'm just excited to see this ship so we can get back on the main line and off of our fork.

[blockjon]

@jmikola Just wanted to triple check there's no other actions you are requesting of me on this ticket. We are very excited to see this hit the main line asap.

[jmikola]

@blockjon: Sorry, I missed the notifications. Nothing to do on your end here. I'll quickly discuss with @alcaeus and then we can resolve this. Thanks for the contribution 👍

[jmikola]

I'm alright with adding setters to the named event classes (e.g. AggregateEventArgs), but I'd like to bring up one last point about EventArgs and MutableEventArgs. For the record, MutableEventArgs dates back to 161be2d and #94.

On a side note, while it'd be nice if we had an interface to denote whether an event class was mutable or immutable, nothing of the sort exists at the moment and extending MutableEventArgs doesn't make sense since it only concerns itself with tracking changes to the $data property.

---

Let's start by reviewing places where EventArgs is used:

• Collection.php
  • preBatchInsert
  • preDropCollection, postDropCollection
  • preFindAndRemove
  • preGetDBRef
  • preInsert
  • preRemove, postRemove
  • preSave
  • postUpdate
• Connection.php
  • preDropDatabase, postDropDatabase
  • preConnect, postConnect
  • preSelectDatabase, postSelectDatabase
• Database.php
  • postCreateCollection
  • preDropDatabase, postDropDatabase
  • preGetDBRef
  • preGetGridFS, postGetGridFS
  • preSelectCollection, postSelectCollection

I'd prefer to leave EventArgs immutable and change some of these events to use MutableEventArgs (with the addition of a setOptions() method you require). Of these, I think only you're only requesting mutability for the following events:

• Collection.php
  • preBatchInsert
  • preFindAndRemove
  • preInsert
  • preRemove, postRemove
  • preSave
  • postUpdate

There is a potential caveat with preBatchInsert, preInsert, and preSave, since ext-mongo potentially modifies the PHP zval and injects an _id field into the array/object. Currently, the arguments passed to these Collection methods make their way directly to the driver methods, which means the user can still access the field on whatever PHP value they supplied as an argument. If we start using MutableEventArgs and allow that argument to be changed, this could break that behavior. The driver would instead inject _id into the changed $data argument, leaving the original untouched. Users would then only be able to access that _id by hooking into the post event.

For that reason, I'd prefer to see preBatchInsert, preInsert, and preSave left as-is and continue to use immutable EventArgs and change only the following to use MutableEventArgs:

• Collection.php
  • preFindAndRemove
  • preRemove, postRemove
  • postUpdate

This would still allow you flexibility to modify the queries used in deletes and updates, which may be all that your original use case required (of the methods dispatching EventArgs).

[jmikola]

Opened lyft#1 with suggested changes. /cc @blockjon

[jmikola]

@alcaeus: I think we're good to merge this with additional changes from lyft#1.

[blockjon]

As I stated on lyft#1, this change is fine so long as I can modify the query object within the collectionPreUpdate event.

[jmikola]

@blockjon: I don't expect any conflict with your needs. We're only using MutableEventArgs for preFindAndRemove, preRemove, postRemove, and postUpdate. The preBatchInsert, preInsert, and preSave events stick with immutable events (for reasons I cited in my previous post). All of the other events objects (including preUpdate) are using their respective event objects, which are now mutable per your own patch. 👍

[alcaeus]

Merged manually. Thank you @blockjon for your contribution!