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

Event cache used even though event auto discovery is off #28872

Closed
jkudish opened this issue Jun 17, 2019 · 4 comments
Closed

Event cache used even though event auto discovery is off #28872

jkudish opened this issue Jun 17, 2019 · 4 comments

Comments

@jkudish
Copy link

jkudish commented Jun 17, 2019

  • Laravel Version: 5.8.23
  • PHP Version: 7.2.x
  • Database Driver & Version: MariaDB 10.1.x && Redis 4.0.6 (cache, etc.)

Description:

In preparation for adding Event Auto-Discovery to our application, I added the php events:cache command to our Envoyer deployment hooks. The PR which added event auto-discovery to our app was however not ready yet, but we still deployed other code, which caused the php events:cache command to fire.

Following this, every event that was type hinted in our app started to fire twice. Shouldn't the cache not fire at all if shouldDiscoverEvents is set to false? Or at least, shouldn't the cache not contain discover events if shouldDiscoverEvents is set to false?

Steps To Reproduce:

  • Created a few event + event listeners (with type hinted events in the handle method)
  • Setup a cache driver for the app
  • Run php events:cache
  • Events will start firing twice even though shouldDiscoverEvents is set to false
@jf-m
Copy link
Contributor

jf-m commented Jun 20, 2019

I am facing the same issue.

However, the reason why the events are fired twice is not only linked to shouldDiscoverEvents.

Basically, this issue uncovers three bad behavior regarding the new event discovery system :

1. Events duplication

Once the events are cached, every EventServiceProvider registered inside the app will listen the whole events.php cache file, because of this ($this->discoveredEvents):

$events = array_merge_recursive(
$this->discoveredEvents(),
$this->listens()

protected function discoveredEvents()
{
if ($this->app->eventsAreCached()) {
return require $this->app->getCachedEventsPath();
}

Therefore, there will be as may duplicated of every fired events as there is custom EventProviders registered in the app.

Example:
You have you own EventServiceProvider, and you also have registered another one from a composer dependency project.

Once the events are cached:
Both EventServiceProvided will have their boot methods executed, both will return and listen to the whole content of the events.php cache file, and all of you events will be duplicated in your application.

2. Events cached regardless of shouldDiscoverEvents

In the EventCacheCommand, there is no check on shouldDiscoverEvents, therefore all the EventServiceProvider will cache their own discoverEventsWithin folders.

Obviously, the default EventServiceProvider class discovers the Listeners folder.
It has its shouldDiscoverEvents to false, but because event:cache does not check on this setting, it will save the Listeners folder anyway in the events.php cache.

This behavior will produce duplication inside the events.php.

3. Duplication is possible on the cached events.php file

Overall, it is currently possible to have duplicated listeners in the events.php file, while I believe, it should not be possible.
For instance, the follow can happen at this time:

  'App\\Events\\MyFooEvent' => 
  array (
    0 => 'App\\Listeners\\MyBarListener',
    1 => 'App\\Listeners\\MyBarListener',
    2 => 'App\\Listeners\\MyBarListener',
  ),

@jf-m
Copy link
Contributor

jf-m commented Jun 20, 2019

Workaround

If you have multiple EventServiceProviders, and you still want to use the event:cache command to speed up your event discovery, here is the workaround that will solve all of the issues mentioned above:

This workaround is not ideal, but it works and maybe it will interest some people.

1. Override the default EventCacheCommand

Create your own implementation of EventCacheCommand and use the following

use Illuminate\Foundation\Support\Providers\EventServiceProvider;

class EventCacheCommand extends \Illuminate\Foundation\Console\EventCacheCommand
{

    /**
     * Get all of the events and listeners configured for the application.
     *
     * @return array
     */
    protected function getEvents()
    {
        $events = [];

        foreach ($this->laravel->getProviders(EventServiceProvider::class) as $provider) {
            if ($provider->shouldDiscoverEvents()) {
                $providerEvents = array_merge($provider->discoverEvents(), $provider->listens());
            } else {
                $providerEvents = $provider->listens();
            }

            $events = array_merge_recursive($events, $providerEvents);
        }

        return $events;
    }
}

Note that, I've only added the check on shouldDiscoverEvents.

Pay attention that amongst all of your EventProviders, the discoverEventsWithin must never return twice the same folder.

2. Override the boot of your custom EventProviders

The parent::boot() method of your EventServiceProvider should be called only by one of all of your EventServiceProviders.
The other ones should have only a single part of the parent's boot :

    public function boot()
    {
        foreach ($this->subscribe as $subscriber) {
            Event::subscribe($subscriber);
        }
    }

@jf-m
Copy link
Contributor

jf-m commented Jun 20, 2019

I'll try to propose a PR 🍻

@jkudish
Copy link
Author

jkudish commented Jun 21, 2019

I think this issue is solved by #28904. Thanks for the PR @jf-m !

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

No branches or pull requests

2 participants