Skip to content

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Jul 15, 2015

Currently, there is no way to know which Subscribers have been called.

This pull request adds support for logging inside a copy of NotifiesMessageSubscribersMiddleware.

To enable this you'll need to add the following to event_bus_logging.yml:

    simple_bus.event_bus.notifies_message_subscribers_middleware:
        class: SimpleBus\Message\Logging\LoggingNotifiesMessageSubscribersMiddleware
        public: false
        arguments:
            - @simple_bus.event_bus.event_subscribers_resolver
            - @logger
            - %simple_bus.event_bus.logging.level%
        tags:
            - { name: event_bus_middleware, priority: -1000 }

@ruudk
Copy link
Contributor Author

ruudk commented Jul 15, 2015

@matthiasnoback Is this the right way to do this?

@matthiasnoback
Copy link

@ruudk Thanks for suggesting this addition - it's a really useful one. Instead of solving this with an extra class, I would have liked to be able to solve it with decoration. However, that's impossible, since you need to add log messages before and after call_user_func().

Since psr/log is a required dependency of this package already, I'd suggest simply adding the logging functionality to the existing NotifiesMessageSubscribersMiddleware. Just allow null for the extra $logger argument and when null is provided, replace it with an instance of NullLogger.

In the SymfonyBridge we can make the extra argument configurable.

@ruudk
Copy link
Contributor Author

ruudk commented Jul 16, 2015

I'll create a PR soon :)

@ruudk
Copy link
Contributor Author

ruudk commented Jul 17, 2015

I'm wondering. Why is there no subscriber_bus? That would allow me to easily log it using a Middleware. It could also use the WrapsMessageHandlingInTransaction so the handling of a Subscriber works the same as that of a Command.

@matthiasnoback
Copy link

If an event subscriber requires a database transaction, then it's intending to make a persistent change, which is exactly what commands are used for ;) So in that case, just create a new command, and let the command bus handle it.

@ruudk
Copy link
Contributor Author

ruudk commented Jul 24, 2015

@matthiasnoback I changed it a bit so that call_user_func gets called via a proteted method. I think this is cleaner and better. What do you think?

@ruudk
Copy link
Contributor Author

ruudk commented Jul 24, 2015

It looks like this now:
screen shot 2015-07-24 at 09 53 42

@matthiasnoback
Copy link

Ah, well, maybe I wasn't clear: my idea was not to extend the existing class, but just to add logging to the existing NotifiesMessageSubscribersMiddleware class. You should make the last two constructor arguments optional (level should be LogLevel::DEBUG by default), and when no logger was provided, just create an instance of NullLogger inside the constructor.

@ruudk ruudk force-pushed the logging-subscribers branch from b4a41b0 to c0e276e Compare July 25, 2015 11:57
@ruudk
Copy link
Contributor Author

ruudk commented Jul 25, 2015

@matthiasnoback Voila! Ready? :)

@matthiasnoback
Copy link

Nice work!

matthiasnoback added a commit that referenced this pull request Jul 29, 2015
Added Logging for SubscriberNotififierMiddleware
@matthiasnoback matthiasnoback merged commit 757e102 into SimpleBus:master Jul 29, 2015
@matthiasnoback
Copy link

I merged the code, but I know realize that there should be something in the docs about this as well - could you please add a note about it? Would you also like to work on a PR for the bundle?

@ruudk
Copy link
Contributor Author

ruudk commented Jul 29, 2015

I'll take care of it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants