InterNACHI / modular

Modularize your Laravel application
MIT License
759 stars 63 forks source link

Laravel 11: Autodiscovery of events is not working anymore #81

Closed Wulfheart closed 6 months ago

Wulfheart commented 6 months ago

The ModularEventServiceProvider calls shouldDiscoverEvents. However, with the new structure it is

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

which will always evaluate to false in an inherited class. This means that https://github.com/InterNACHI/modular/blob/148cfedda25202f086a8fe58014250f860583af2/src/Support/ModularEventServiceProvider.php#L20-L26 will always be false.

Wulfheart commented 6 months ago

Going forward I would like to introduce a fix for it. However, I am not sure what the desired behavior will be as there is no way anymore to determine if autloading has been manually enabled or disabled. I guess, the best way forward would be a config value in the package where it can be enabled or disabled. What do you think?

inxilpro commented 6 months ago

@Wulfheart I'm trying to confirm on my end… but it seems like the ModularEventServiceProvider might break event discovery in general in Laravel 11. Not 100% sure, but it kinda looks like if you register any provider that extends the Laravel EventServiceProvider, that will cause Laravel to not register the default one…

mra-dev commented 6 months ago

@inxilpro Isn't there any workaround yet ?

Wulfheart commented 6 months ago

@mra-dev I have one in a branch but that is not a durable solution.

mra-dev commented 6 months ago

I actually found a nice and easy workaround. Just use Laravel 10's default EventServiceProvider & add it to bootstrap/providers.php's Array & set shouldDiscoverEvents to return true. And suddenly everything's just working fine.

// app/Providers/EventServiceProvider.php

<?php

namespace App\Providers;

use Illuminate\Foundation\Support\Providers\EventServiceProvider as ServiceProvider;

class EventServiceProvider extends ServiceProvider
{
    public function shouldDiscoverEvents(): bool
    {
        return true;
    }
}
// bootstrap/providers.php

<?php

use App\Providers\EventServiceProvider;

return [
    App\Providers\AppServiceProvider::class,
    EventServiceProvider::class,
];

running artisan command: php artisan event:list

before:

...
...
  Illuminate\Queue\Events\JobProcessed .............................................................................................................
  ⇂ Closure at: /vendor/spatie/laravel-ignition/src/IgnitionServiceProvider.php:263
  Illuminate\Queue\Events\JobProcessing ............................................................................................................
  ⇂ Closure at: /vendor/laravel/framework/src/Illuminate/Log/Context/ContextServiceProvider.php:38
  ⇂ Closure at: /vendor/spatie/laravel-ignition/src/IgnitionServiceProvider.php:257

after:

...
...
  Illuminate\Queue\Events\JobProcessed .............................................................................................................
  ⇂ Closure at: /vendor/spatie/laravel-ignition/src/IgnitionServiceProvider.php:263
  Illuminate\Queue\Events\JobProcessing ............................................................................................................
  ⇂ Closure at: /vendor/laravel/framework/src/Illuminate/Log/Context/ContextServiceProvider.php:38
  ⇂ Closure at: /vendor/spatie/laravel-ignition/src/IgnitionServiceProvider.php:257

  MraDev\Product\Events\UploadMedia ................................................................................................................
  ⇂ MraDev\Upload\Listeners\ProcessUploadingMedia@handle
inxilpro commented 6 months ago

Unfortunately this is looking like a harder thing the fix than I hoped. We can’t extend the Laravel service provider any more, so we need to replace more than I expected. I have it partially fixed, but I’m on spring break with my kids, so I have less time to work on it than usual. I’m gonna be on a train for 5 hours today, so hopefully I can get this sorted out then.

inxilpro commented 6 months ago

@mra-dev can you try #88 and see if that works for you? Pretty sure that should fix it.

mra-dev commented 6 months ago

@inxilpro Yeah for sure. How should i do that? Update it from composer.json or ...?

mra-dev commented 6 months ago

@inxilpro I almost forgot. I tested it manually by editing vendor files and dump-autoload composer and then got the event:list & it totally worked fine and listed my newly added Events/Listeners.

Then i thought maybe i should manually turn Autodiscovery ON with Laravel's EventServiceProvider & add it to providers (Taylor said in his conference video that you can already use old providers if you wish to extend). And it worked like a charm. 😍

Like This: Solution

inxilpro commented 6 months ago

@mra-dev you can do composer require internachi/modular:dev-laravel-11-event-support to install it from that branch and try it out.

mra-dev commented 6 months ago

@inxilpro Did you check my Solution yourself & what do you think of it ?

inxilpro commented 6 months ago

@inxilpro Did you check my Solution yourself & what do you think of it ?

That solution is great! I just want to make sure that the package works without any workarounds…

inxilpro commented 6 months ago

(I'm going to close this because I just confirmed that #88 works in Laravel 11.)