Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.8] Event Discovery #28064

Merged
merged 14 commits into from
Apr 1, 2019
Merged

[5.8] Event Discovery #28064

merged 14 commits into from
Apr 1, 2019

Conversation

taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Mar 29, 2019

This PR provides automatic event / listener discovery.

Benefits

No longer necessary to manually register events and their listeners in the EventServiceProvider.

How It Works

Events and their listeners are discovered by reflecting into all listeners and inspecting all public handle methods or methods that begin with handle. We then can inspect the methods of those parameters to determine which events the listener is handling, allowing us to build an array of events / listeners that basically looks exactly like the manually built array that is currently configured in EventServiceProvider. We then merge the explicitly defined events on top of the discovered events and register everything with the event dispatcher.

Usage

Since I am adding this to the 5.8 release, automatic discovery is disabled by default. However, it may be enabled by overriding the shouldDiscoverEvents method on the EventServiceProvider. This function should return a boolean.

/**
 * Determine if events and listeners should be automatically discovered.
 *
 * @return bool
 */
public function shouldDiscoverEvents()
{
    return false;
}

If event discovery is enabled, discovered events are loaded in two ways.

  1. Discovered events may be cached during deployment using the new event:cache command. If the framework finds an event cache file, the event / listener configuration will be loaded from that file. Note that any explicitly defined events / listeners in the EventServiceProvider will still be registered as well.

  2. If no event cache file is found, the events / listeners will be discovered on-the-fly during the request. This is not recommended for production since it will be slightly slower; however, for local development it should be satisfactory. Of course, if someone has as unique application where this is too slow even in local development they can simply not use event discovery entirely and register all of their events as they would previously.

@driesvints driesvints changed the title Event Discovery [5.8] Event Discovery Mar 29, 2019
@nunomaduro
Copy link
Member

No plans to merge.

@devcircus
Copy link
Contributor

devcircus commented Mar 29, 2019

I really like this. I implemented cached and auto-discoverable "jobs/handlers" in a previous project. I was never happy with my implementation and removed it. May go back and revisit this now.

@misenhower
Copy link
Contributor

Happy to see caching 👍, would be cool to have an event:list command to show the discovered mappings.

@browner12
Copy link
Contributor

does this jive with the newly approved PSR-14?

https://www.php-fig.org/psr/psr-14/

@halaei
Copy link
Contributor

halaei commented Mar 29, 2019

Why methods that begin with handle? Why not just handle()

@patrickbrouwers
Copy link
Contributor

patrickbrouwers commented Mar 29, 2019

Why methods that begin with handle? Why not just handle()

Will work for event subscribers as well then, I think. And for event listeners with multiple handlers in general (usually registered as Listener@handleSomeEvent)

@taylorotwell
Copy link
Member Author

Why methods that begin with handle? Why not just handle()

I'm fine with removing this if people don't like it; however, I wondered if some people may listen to multiple events in one listener.

@devcircus
Copy link
Contributor

I'm for allowing for methods that begin with "handle". This is a common pattern. I guess as long as that logic can be overridden in the service provider, then it's no big deal either way.

protected function discoverEventsWithin()
{
return [
$this->app->path('Listeners'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be customizable via a config on config/app.php? an also can this be an array of paths?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be customizable via a config on config/app.php? an also can this be an array of paths?

It is now simple to override it in app\Providers\EventsServiceProvider.

@halaei
Copy link
Contributor

halaei commented Mar 29, 2019

This is listener discovery more than event discovery.

*
* @return void
*
* @throws \RuntimeException
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about this @throws \RuntimeException. When can it happen?

@halaei
Copy link
Contributor

halaei commented Mar 29, 2019

To have more configuration control, EventServiceProvider can initiate the discovery itself, just like what console kernel does with Commands directory:

public function listeners()
{
    $this->discover(app_path('Listeners'), 'handle*');
}

static::classFromFile($listener, $basePath)
);

foreach ($listener->getMethods(ReflectionMethod::IS_PUBLIC) as $method) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes more sense to remove the continue control flow and simplify the condition, like so:

foreach ($listener->getMethods(ReflectionMethod::IS_PUBLIC) as $method) {
    if (Str::is('handle*', $method->name) && isset($method->getParameters()[0])) {
        $listenerEvents[$listener->name.'@'.$method->name] = optional($method->getParameters([0]->getClass())->name;
    }
}

@knvpk
Copy link
Contributor

knvpk commented Mar 30, 2019

What will happen for the below case mentioned. I have enabled auto discovery and for an event there are two listeners but I have mentioned one listener explicitly did the other listener will be discovered or ignored.

@taylorotwell
Copy link
Member Author

taylorotwell commented Mar 30, 2019

To have more configuration control, EventServiceProvider can initiate the discovery itself, just like what console kernel does with Commands directory:

public function listeners()
{
    $this->discover(app_path('Listeners'), 'handle*');
}

@halaei What if it is called multiple times? What would the behavior be? How would caching work in this situation?

@halaei
Copy link
Contributor

halaei commented Mar 30, 2019

@halaei What if it is called multiple times? What would the behavior be? How would caching work in this situation?

I haven't think about it. Now that I see discoverEventsWithin() is in EventServiceProvider and it can return an array, I guess it is configurable after all.

return require $this->app->getCachedEventsPath();
}

return $this->shouldDiscoverEvents()
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldDiscoverEvents() is redundant because discoverEventsWithin() can return empty array. The non-empty array $this->app->path('Listeners') can be returned from App\Providers\EventServiceProvider and the feature can be enabled for new Laravel installations by default.

->reduce(function ($discovered, $directory) {
return array_merge(
$discovered,
DiscoverEvents::within($directory, base_path())
Copy link
Member

Choose a reason for hiding this comment

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

You can use $this->app->basePath() here.

@TBlindaruk
Copy link
Contributor

I`m not sure how. But I think w should try to cover this by tests;

@taylorotwell
Copy link
Member Author

I will be writing tests.

@taylorotwell taylorotwell merged commit 8c227e5 into 5.8 Apr 1, 2019
@vicgonvt
Copy link
Contributor

vicgonvt commented Apr 1, 2019

This is awesome. Thanks for that!

@driesvints driesvints deleted the event-discovery branch April 1, 2019 18:10
@tomirons
Copy link
Contributor

tomirons commented Apr 4, 2019

Can someone explain how it knows the order of listeners? I've been trying to figure this out for a little while.

@erikgaal
Copy link
Contributor

erikgaal commented Apr 4, 2019

@tomirons, I don’t think you can specify or influence the order of the listeners. If you need a specific order, you should dispatch a new event from the “parent” listener.

@tomirons
Copy link
Contributor

tomirons commented Apr 4, 2019

@erikgaal I did a quick test by creating an event Foo and two listeners in this order (Second, First). Sure enough, the order of the listeners was Second then First, so it must go by when the file was created?

@MichaelDeBoey
Copy link
Contributor

@tomirons Just like @erikgaal (and @taylorotwell on Twitter) said: if you need to have an order/sequence, the first listener should dispatch a new event that the second listener can act on.

@tomirons
Copy link
Contributor

tomirons commented Apr 4, 2019

@MichaelDeBoey This is what I thought was going to be said, it would be beneficial to just use the jobs at this point and chain them.

@driesvints
Copy link
Member

@tomirons in general you shouldn't rely on the order of your listeners. If you need a specific sequence you should indeed use jobs and chain them.

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.