Skip to content

Conversation

holtkamp
Copy link

When trying to configure an EventBus, the part of registering event subscribers was not totally clear to me. Some questions I had:

  • why would I need a ServiceLocator / why is it mandatory?
  • why was my 'notify' method invoked statically?

The it struck my that my approach was using:

 $callable1 = [My\Class\Name::class, 'notify']

Which is a valid callable, passing the is_callable() check, and therefore the method was invoked statically. Eventually I went for the $fqcn approach, which then uses the ServiceLocator to instantiate the classname.

Not sure whether the suggested approach improves readability, just want to underline that it can be clarified a bit... I also think the ServiceLocator overcomplicates the example, maybe introduce a simpler plain CallableResolver that does not require / use a ServiceLocator? When handing over callables, this seems a more obvious approach opposed to requiring a ServiceLocator that is never used.

@TomasVotruba
Copy link

@holtkamp Awesome. I remember I came across the same question.

I would add space after comment start: //Will => // Will

Otherwise fine 👍

doc/event_bus.md Outdated

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is true

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that is how I got it configured now:

What part is not true do you think?. Maybe the text should be

.. and invoke $instance->notify() if the notify() function exists

@matthiasnoback
Copy link

@holtkamp Thanks for adding this useful piece of information to the docs! If you could fix my comment please and also add a space after the "//" as @TomasVotruba mentioned, then I can merge and publish this.

@holtkamp
Copy link
Author

@matthiasnoback, just added the spaces and commented on your note.

Additionally, what do you think of the suggestion to leave out the mandatory ServiceLocator. Only 2 out of the 5 identified ways to configure the EventHandlers depend on it. For beginners on this topic a ServiceLocator might confuse users... Just my 2 cents 😉

@matthiasnoback
Copy link

@holtkamp Thanks. It turns out I was wrong - the service locator accepts a string argument. I just didn't think about using it like this (i.e. instantiating the class in the service locator callable) :)

Could you please rebase this branch on the latest master? It seems that this branch is outdated.

Leaving out the service locator was indeed an option in one of the earlier designs. However, I feel that every application using a command bus should use lazy-loading for command handlers, since otherwise the application would have to instantiate all handlers and all dependencies upon boot time. So, I'd like to just leave it as it currently is.

@holtkamp
Copy link
Author

Still learning about git, seems my GitHub Desktop automatically pushed an incomplete merge. Hope it is ok now...

@matthiasnoback
Copy link

Hi there, I decided to dive into the code myself and make the description a little bit more complete and to also copy the same description of these "callables" to the command bus section.
Thanks for reporting this lack of documentation! And thanks for using SimpleBus of course!

@holtkamp
Copy link
Author

Great! Thank you for sharing this flexible tool, your knowledge and
experience on the web ;)
On Nov 27, 2015 8:08 PM, "Matthias Noback" notifications@github.com wrote:

Hi there, I decided to dive into the code myself and make the description
a little bit more complete and to also copy the same description of these
"callables" to the command bus section.
Thanks for reporting this lack of documentation! And thanks for using
SimpleBus of course!


Reply to this email directly or view it on GitHub
#46 (comment).

@matthiasnoback
Copy link

@holtkamp :)

@holtkamp holtkamp deleted the patch-4 branch December 2, 2015 07:42
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.

3 participants