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

Fix Laravel 11 event auto-discovery by registering listeners in AuditingServiceProvider #916

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

onlime
Copy link
Contributor

@onlime onlime commented Mar 21, 2024

Laravel 11 ships with default Event Discovery, so it automatically finds and registers your event listeners by scanning your app/Listeners directory. So, you no longer need to extend the EventServiceProvider::shouldDiscoverEvents() method.

But currently, Laravel Auditing's AuditingEventServiceProvider breaks the auto-discovery.

L11 picks the first service provider and calls the shouldDiscoverEvents() on that one to decide whether auto-discovery should be enabled or not. And because of AuditingEventServiceProvider extending the default \Illuminate\Foundation\Support\Providers\EventServiceProvider, this packages event service provider is usually the first one in the app container (Illuminate\Foundation\Application::$serviceProviders), so that method call will always result in false:

    public function shouldDiscoverEvents()
    {
        return get_class($this) === __CLASS__;
    }

('OwenIt\Auditing\AuditingEventServiceProvider' !== 'Illuminate\Foundation\Support\Providers\EventServiceProvider')

So, basically, this package hijacks Laravel's event service auto-discovery altogether, and in a L11 app, we could only fix/override this, by registering our own EventServiceProvider that overrides above method:

// bootstrap/app.php
return Application::configure(basePath: dirname(__DIR__))
    ->withProviders([
        // nasty workaround to do without this PR
        \App\Providers\EventServiceProvider::class, // ... where shouldDiscoverEvents() always returns true

On Illuminate\Foundation\Application#L905-L910 you see, how Laravel just grabs the first serviceProvider, and AuditingEventServiceProvider actually is an instanceof Illuminate\Foundation\Support\Providers\EventServiceProvider (as it extends it):

    public function getProvider($provider)
    {
        return array_values($this->getProviders($provider))[0] ?? null;
    }

    public function getProviders($provider)
    {
        $name = is_string($provider) ? $provider : get_class($provider);

        return Arr::where($this->serviceProviders, fn ($value) => $value instanceof $name);
    }

I found no other package that directly extends Laravel's EventServiceProvider, and I assume this is "bad practice" (which I cannot prove as I didn't find any documentation about this) with such potential negative side-effects.

This PR fixes it by ditching AuditingEventServiceProvider and moving the package's event listener registration to the AuditingServiceProvider::boot() method. By not using Laravel's Event facade, but using app('events')->listen() instead, this should also work for Lumen and eliminates the conditional class_alias() quirk in the previous AuditingEventServiceProvider.

Cheers, Pipo

…compatibility

Eliminate AuditingEventServiceProvider which interfered with L11 event auto-discovery, EventServiceProvider::shouldDiscoverEvents()
@cesarreyes3
Copy link

So, must AuditingEventServiceProvider be removed?

It seems to me that it would no longer be used

@onlime
Copy link
Contributor Author

onlime commented Apr 17, 2024

So, must AuditingEventServiceProvider be removed?

It seems to me that it would no longer be used

exactly! Look at the diff of this PR, it's already removed.

@willpower232
Copy link
Contributor

Just stumbled into this myself, hope the PR gets merged soon.

Following your suggestion, I worked around it without creating a new service provider and just calling laravels directly, forcing the use of the real class

    ->withProviders([
        Illuminate\Foundation\Support\Providers\EventServiceProvider::class,
    ])

@MortenDHansen
Copy link
Contributor

I have not yet upgraded my applications to L11. Is it confirmed that this both works and is backwards compatible?

@onlime
Copy link
Contributor Author

onlime commented Apr 22, 2024

I have not yet upgraded my applications to L11. Is it confirmed that this both works and is backwards compatible?

I'm only running on L11. But yes, I have run tests on both L10 (v10.48.8) and L11 (v11.4.0) on PHP 8.3, both successfully.

Afaik, extending from EventServiceProvider was never supposed to be used in packages, and it's removed completely from L11 Events docs. So, directly registering the listeners with $events->listen() in the ServiceProvider's boot() method was always the recommended way.

This PR was done together with @pascalbaljet and reviewed by him as well.

@ManuelLeiner
Copy link

This works for me.

@onlime Out of curiosity, is there a reason, you did not use the Event facade? Compatibility with L7-9? I am not familiar with package development.

@onlime
Copy link
Contributor Author

onlime commented Apr 22, 2024

This works for me.

@onlime Out of curiosity, is there a reason, you did not use the Event facade? Compatibility with L7-9? I am not familiar with package development.

nope, no specific reason. The Event facade already existed back in L7. I have changed it back to use the facade in 18b17b4.
Or, wait, I think it was for Lumen support, as Lumen does not seem to support facades out of the box. But is Lumen still a thing? It has been archived recently (April 9th) and I think we should no longer support it.

@willpower232
Copy link
Contributor

Interestingly they've archived the lumen template repository but the framework one is still going.

Judging by https://github.com/laravel/lumen-framework/blob/11.x/src/Providers/EventServiceProvider.php, I'd say app('events') is the most agnostic way of doing it however the docs reference using the Event facade so its probably fine either way https://lumen.laravel.com/docs/11.x/events

@driesvints
Copy link

driesvints commented Apr 23, 2024

The solution in this PR is twtg 👍 We no longer recommend to extend the default event service provider. In fact we never recommended packages to do that in the first place.

Interestingly they've archived the lumen template repository but the framework one is still going.

Yeah since we no longer recommend to starting new Lumen projects. We're still updating the framework one for current apps. But our general recommendation is to move away from Lumen if you can.

@MortenDHansen MortenDHansen merged commit 2be6a9c into owen-it:master Apr 29, 2024
15 checks passed
@cesarreyes3
Copy link

Please, release it on packagist

@onlime
Copy link
Contributor Author

onlime commented May 15, 2024

@parallels999 Are you planning to release this soon? Thanks for merging. I can totally live with some more weeks/months running on dev-master, but it looks like others waste their time with this bug and duplicate issues pile up (#923, #924, #926)

Oh, just read the sad news on #925 – is there anybody out there who could take over @MortenDHansen's wonderful project?

@parallels999
Copy link

@parallels999 Are you planning to release this soon?

I'm not the maintainer, I do not have the necessary permissions

Oh, just read the sad news on #925

It is still unknown what will happen in the future.

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.

None yet

7 participants