lorisleiva / laravel-actions

⚡️ Laravel components that take care of one specific task
https://laravelactions.com
MIT License
2.52k stars 124 forks source link

[Octane] Event use handle instead of asListener #136

Closed exodusanto closed 1 year ago

exodusanto commented 3 years ago

Using Laravel Octane seems that actions are run as object instead of listener.

The problem is similar to #87, the workaround on EventServiceProvider event/listener registration works.

UserUpdated::class => NotifyUserUpdated::class.'@asListener'

lorisleiva commented 3 years ago

Hi there 👋

Thanks for raising this! The issue with #87 was that Laravel Actions was not able to recognise the action as being a listener due to a lack of recognised frames.

Since I've not used Octane yet, would you be able to tell me how Laravel dispatches listeners when used with Octane? It could be using a different method or perhaps even a different class than the ones listed above which would cause Laravel Actions to default to returning an object.

exodusanto commented 3 years ago

This is a trace of the exception.

#0 /.../vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php(424): App\\Actions\\User\\SetupCompleted->handle(Object(App\\Events\\UserUpdated))
#1 /.../vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php(249): Illuminate\\Events\\Dispatcher->Illuminate\\Events\\{closure}('App\\\\Events\\\\User...', Array)
#2 /.../vendor/nuwave/lighthouse/src/Schema/Directives/EventDirective.php(48): Illuminate\\Events\\Dispatcher->dispatch('App\\\\Events\\\\User...')

Note: I'm using lighthouse graphql plugin, but this problem is only with octane serve

lorisleiva commented 3 years ago

According to your stacktrace, Laravel Actions should recognise it as a Listener (see code below).

https://github.com/lorisleiva/laravel-actions/blob/6e0aa6ed7cf379c3dd0e8d842a1ce754394152f6/src/DesignPatterns/ListenerDesignPattern.php#L20

Is there anything unusual about how you dispatch these events?

exodusanto commented 3 years ago

I made some tests and I think that this part

https://github.com/lorisleiva/laravel-actions/blob/6e0aa6ed7cf379c3dd0e8d842a1ce754394152f6/src/ActionServiceProvider.php#L41-L51

extends only some classes, maybe octane create a new application instance like there

Edit: I added an var_dump before the $manager->extend($abstract); and only Actions registred as command are logged

lorisleiva commented 3 years ago

Hey 👋 Sorry for the late reply. I finally got around to checking it out. Could you try to use dev-main and let me know if that fixes your issue?

exodusanto commented 3 years ago

Hi, thank you for your fix, but it doesn't work.

I have the same exception:

App\Actions\User\SetupCompleted::handle(): Argument #1 ($user) must be of type App\Models\User, App\Events\UserUpdated given, called in /.../vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php on line 424

I also made some tests but I'm worried that beforeResolving is not supported from Octane, or for integrate it this package need to register some logic into octane.php file

 RequestReceived::class => [
            ...Octane::prepareApplicationForNextOperation(),
            ...Octane::prepareApplicationForNextRequest(),
            //
        ],
lorisleiva commented 3 years ago

No worries, thanks for trying!

If anyone using Octane would like to open a PR for this, that would be most appreciated. ❤️

exodusanto commented 3 years ago

Maybe @nunomaduro can give us a hit if nowadays Octane support beforeResolving in some way

nunomaduro commented 2 years ago

Are you able to make a pull request in Octane with a failing test? Or create an issue, with a repository example, so I can reproduce the issue locally?

exodusanto commented 2 years ago

Hi Nuno!

This is the repo.

To setup the reproduction DB

php artisan migrate:fresh
php artisan db:seed # this will create an user

There are two commits:

  1. Base reproduction
  2. Octane installed

Base reproduction

Route / send an event UserUpdated and the listener action UserUpdateTask writes on logs a message.

Octane installed

Starting the server with Octane the same flow throws the error

Wulfheart commented 2 years ago

@exodusanto I think he meant that you should create an issue in the Octane repo itself.

exodusanto commented 2 years ago

Not yet 😔, I'll find some time next weeks