PHP-DI / Silex-Bridge

PHP-DI integration in Silex
http://php-di.org/doc/frameworks/silex.html
MIT License
24 stars 8 forks source link

Container aware Event Dispatcher #12

Closed praswicaksono closed 8 years ago

praswicaksono commented 8 years ago

I dont know if this project appropriate with this feature. Long time ago I have proposed this feature in silex but it was closed, so I decide to open it in here.

The point is override the current EventDispatcher with ContainerAwareEventDispatcher. This will allow listener loaded from container and it will load listener lazily only when related event triggered.

here is related article for this feature : http://symfony.com/doc/current/components/event_dispatcher/container_aware_dispatcher.html

I would love to send PR if this feature accepted :)

jdreesen commented 8 years ago

I like the idea of lazy event dispatchers (I'm currently thinking about extending our own small event dispatcher for my current paid work project to be lazy, too) and I'm curious what @mnapoli thinks about them.

What I don't really like is the name ContainerAwareEventDispatcher though. Maybe LazyEventDispatcher would be better?

mnapoli commented 8 years ago

I'd rather stay close to the base Silex package. Is that really hard to replace the event dispatcher in Silex (or Silex + PHP-DI)? I'm not sure why this package should make too many things different, unless there's a good reason? If replacing the event dispatcher is just a config line then I think it's fine leaving it to users.

@jdreesen for event dispatchers in general I definitely agree that lazy loading listeners make sense. Also that's not directly the topic but I find it very interesting that dispatching to event listeners sounds a lot like dispatching to controllers, CLI commands, etc. Sounds something like this would make sense: InvokerInterface. And that's funny because something similar is being discussed right this moment in container-interop: https://github.com/container-interop/container-interop/issues/43

jdreesen commented 8 years ago

You are right. As this is a brige it should probably not add many new features besides integrating PHP-DI with Silex.

Besides that it should be fairly easy to use a custom event dispatcher implementation by just overwriting the dispatcher_class param in the container (Silex 1.3) or the dispatcher service in the HttpKernelServiceProvider (Silex 2). As long as it implements Symfony's EventDispatcherInterface, of course ;)

So this should probably be done in a separate provider package which implements a lazy event dispatcher that integrates with any container that implements container-interop's ContainerInterface and thus can be used with other containers, too.

jdreesen commented 8 years ago

@mnapoli you are absolutely right, that InvokerInterface makes sense in this regard. I experimented a bit with our project's event dispatcher and stumbled over something similar along the way. And I'm following the mentioned discussion in container-interop of course ;)

mnapoli commented 8 years ago

@jdreesen Great!

(that's not directly related but since container-interop is mentioned feedback on this repo would be great if you have any: https://github.com/container-interop/service-provider - feel free to open issues)

So this should probably be done in a separate provider package which implements a lazy event dispatcher that integrates with any container that implements container-interop's ContainerInterface and thus can be used with other containers, too.

That sounds good, I didn't think that would be needed indeed. The lazy event dispatcher is not compatible with Pimple I guess?

praswicaksono commented 8 years ago

@mnapoli I see your point, and yes it is not compatible with Pimple. Actually its very easy to replace it and it compatible non-lazy listener. The only benefit by using lazy event dispatcher only in userland code since all listener in silex registered in non-lazy way.

Also the idea of symfony event dispatcher that aware for any container interop is good and I agree if this feature should be made in different package also this is good time for me to experience container-interop service provider

mnapoli commented 8 years ago

@Atriedes that's great! If you do it, you can also send a PR to add your repository to the list: https://github.com/container-interop/container-interop#projects-using-containerinterface

I'll be closing this issue then.