Closed onlime closed 5 months ago
So, must AuditingEventServiceProvider
be removed?
It seems to me that it would no longer be used
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.
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,
])
I have not yet upgraded my applications to L11. Is it confirmed that this both works and is backwards compatible?
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.
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.
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.
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
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.
Please, release it on packagist
@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 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 https://github.com/owen-it/laravel-auditing/discussions/925
It is still unknown what will happen in the future.
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 theEventServiceProvider::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 ofAuditingEventServiceProvider
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 infalse
:(
'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:On Illuminate\Foundation\Application#L905-L910 you see, how Laravel just grabs the first serviceProvider, and
AuditingEventServiceProvider
actually is an instanceofIlluminate\Foundation\Support\Providers\EventServiceProvider
(as it extends it):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 theAuditingServiceProvider::boot()
method. By not using Laravel'sEvent
facade, but usingapp('events')->listen()
instead, this should also work for Lumen and eliminates the conditionalclass_alias()
quirk in the previousAuditingEventServiceProvider
.Cheers, Pipo