Made all DoctrineExtensions listeners lazy-loaded.

[bobthecow]

• Removed ListenerManager classes.
• Tagged listeners as Doctrine (DBAL/ODM) listeners.

This change has two benefits:

1. Listeners are no longer instantiated on every core.request event. Before, every listener was instantiated on every request, and passed to the ListenerManager. Now, listeners are lazy-loaded as needed when their document or entity manager service is requested. On requests that don't use document managers or entity managers, listeners aren't even instantiated :)
2. Because listeners are loaded as needed, they will be present whenever their associated EM/DM is loaded. In other words, they now work as expected in integration and functional tests. Which is, incidentally, is exactly why this change was necessary.

This has two side effects:

1. There is no longer a manual ListenerManager to attach/remove listeners. This is now done the same way as manually attaching and removing all Doctrine listeners.
2. Currently, listener services are marked as "private", meaning a different listener instance will be instantiated for each document or entity manager. In the worst case scenario, this will generate at most four listeners per EM or DM used in a given request. In practice, the number of duplicated listeners will be much smaller, as not all listeners will be enabled, and not all managers will be used in every request. This side effect could be mitigated entirely by making the listeners public services, but I think the current implementation is perfectly acceptable.

[l3pp4rd]

Hey, good change, if I'm not mistaken these extensions do not have DBAL listeners, only ORM and ODM :)
everything else seems nice

[stof]

I was thinking to a refactoring to make them lazy-loaded without having the good implementation yet.
I don't like the removing of the manual attach/remove as it forces to always use the listeners. But the user can want to disable them in some cases (my example is a migration command needing to keep the old timestamps). Your change removes the flexibility for the application developper.

So I will not merge it at the moment and keep searching a better way to do it.
Of course if you have an idea to combine the lazy-loading and the flexibility feel free to provide another pull request.

[l3pp4rd]

I think the migration case with timestamp changes are my responsibility in extensions. On timestampable behavior I should check the changeset to see if user is not forcing the value by $entity->SetCreated(/his datetime/) that should by fixed on my side. I also suggest not to rely on core.request and use the lazy load

[bobthecow]

It seems like maintaining the current implementation is quite a bit of overhead and inefficiency for an edge case, right? All event listeners can be removed from the EventManager the good old fashioned way:

$em = $dm->getEventManager();
foreach ($em->getEventListeners() as $event => $listeners) {
    foreach ($listeners as $hash => $listener) {
        if ($listener instanceof TimestampableListener) {
            $em->removeEventListener($listener);
        }
    }
}

[bobthecow]

Removing public=false from the listener service declarations would clean up that block of code a bit:

$timestampableListener = $this->container->get('stof_doctrine_extensions.odm.mongodb.listener.timestampable');
$em = $dm->getEventManager();
if ($em->hasEventListener($timestampableListener)) {
    $em->removeEventListener($timestampableListener);
}

But, as I said, this seems like an edge case, and I'm not sure it should impact the public or private-ness of the services. If side effect 2 listed above should be resolved as "make 'em public", a slightly simpler listener removal process will be a happy side effect.

[l3pp4rd]

I agree with bobthecow. I will soon update the timestampable to handle user manual(forced) values as it should.

[bobthecow]

Also, this resolves issue #2 (which is why I wrote it, incidentally).

[stof]

If the timestampable behaviour handle the case of forced values correctly your patch will be fine. Removing the other listeners at runtime makes no sense anyway.

I will merge it and adapt it to my latest commit with the ODM listeners.

[l3pp4rd]

Timestampable is updated to handle manual values, I think that solves the issues with migration

[stof]

@bobthecow: btw, are you sure the listeners are instanciated several times with the current implementation ? they are passed to the ListenerManager which use this instance for all entity managers so I don't see how it could be possible.

[bobthecow]

No, current implementation only instantiates one of each listener. Caveat 2 could have been worded better as:

Currently, listener services are marked as "private". Unless this is changed, a different listener instance will be instantiated for each document or entity manager...

[stof]

The other issue is setting the locale from the session. I did not find how to pass it directly last week so it was made in the ListenerManager. So I will push the change in a separate branch waiting to find the solution for this.

[bobthecow]

I've got a fix for that. I'll update the pull request :)

[stof]

I just pushed my work to a lazy-load branch (whitout using your commit because of the conflict with my changes and removing the useless DIC parameters doctrine.orm.entity_managers and doctrine.odm.mongodb.document_managers)

Btw you should work in topic branches instead of master to make it easier to keep your master branch in sync

[stof]

I find your solution good.

[stof]

This is now merged into master. Thanks.

Btw you should use git branch master upstream/master (assuming my repo is named upstream in your remotes) to have the same history as mine instead of merging. This is the reason why working in a topic branch is the good solution (the master branch stay clean)

[bobthecow]

I know. But the only reason there's a bobthecow/master branch was to make this update, so i'll just git push -f bobthecow upstream/master:master :)