Fix Octane Event using handle instead of asListener

[dmason30]

Fixes #136

This PR seems to fix the issue when using listeners with an application running on Octane where it calls handle instead of the asListener method.

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

We encountered this issue on listeners that were not queued (i.e. not implementing ShouldQueue).

Cause

After some lengthy debugging, it seemed that the app()->extend(.. callback in ActionManager::extend method was not actually getting called once the listener was resolved when running on Octane. So I tried to figure out which container instance it should be using.

Fix

Basically reverted the changes made originally here a2e42f0 as it seems in this case it is correct to use the Application instance passed to the beforeResolving callback to extend the found action classes.

This seemed to fix the issue for me when running Octane locally. We deployed this fork to our prod environments too (on Vapor) and it works without issue.

Possible Breaking Changes

I have had to change the method signature of ActionManager::extend to accept an Application $app argument.
Let me know if you can come up with a change that would not require changing this method signature.

Anyone who has done a workaround to this issue by defining their event listeners like:

UserEvent::class => [
    [UserAction::class, 'asListener']
]

They will get an error after this fix of Unknown method ListenerDecorator::asListener and will need to change these back to:

UserEvent::class => [
    UserAction::class,
]

[lorisleiva]

Hi there, thank you for that! 🍺

I'm happy to merge this as a minor bump despite the niche breaking change.

However, it looks like some tests are failing in the PR for some environments. Would you mind investigating?

[dmason30]

@lorisleiva these failing tests are not caused by this change but rather by a fix made in a newer version of laravel 9.x/10.x.

I have created a separate PR with the test fix here: #237

[dmason30]

Whilst running this change in production, it works 99% of the time, yet we have seen this error still occur 4 times in 8 days all on the same AsListener action out of 1000s of successful executions in that time. Which is concerning, I have made another change which adds the following:

• ActionManager is now registered as a scoped singleton instance. This is to ensure that the class is reset between requests, mainly to ensure the ActionManager::$extended property is reset.

  • The scoped method binds a class or interface into the container that should only be resolved one time within a given Laravel request / job lifecycle. While this method is similar to the singleton method, instances registered using the scoped method will be flushed whenever the Laravel application starts a new "lifecycle", such as when a Laravel Octane worker processes a new request or when a Laravel queue worker processes a new job. Source: https://laravel.com/docs/10.x/container#binding-scoped

  • The beforeResolving callback used to extend actions now uses the container to get the instance of ActionManager.

• Bumped the illuminate/contracts minimum dependency to 8.67:

  • Scoped bindings was originally added in 8.47 so I was going to bump the minimum to that but then I noticed that your Github Actions run-tests.yml was already running on a minimum of 8.67 so I went with that instead.

I will deploy this updated change to our test environments in the next week, and production will probably be a couple weeks. If you prefer, I can split this specific part of the fix into a seperate PR, I look forward to your feedback.

[lorisleiva]

Awesome, thanks for that!

Could you please pull the latest changes from main? I have fixed the tests caused by the latest version of Laravel and also dropped support for Laravel 8 altogether since that's now 2 major versions behind.

[dmason30]

@lorisleiva I have rebased the PR from the latest main branch. 👍

[dmason30]

You should probably add PHP 8.2 to the github actions test matrix.

[lorisleiva]

Thanks! Would you be able to tell me if that PR now fixes your production app 100% of the time so we know for sure it works before releasing?

You should probably add PHP 8.2 to the github actions test matrix.

It doesn't look like the laravel/framework repo supports 8.2 yet in the composer.json so I prefer waiting for Laravel to support it first.

[dmason30]

Thanks! Would you be able to tell me if that PR now fixes your production app 100% of the time so we know for sure it works before releasing?

I won't have this newest version of this fix on production for a couple weeks once it has gone through testing. Feel free to hold off merging until then.

It doesn't look like the laravel/framework repo supports 8.2 yet in the composer.json so I prefer waiting for Laravel to support it first.

Laravel does support 8.2 all our laravel apps are running on PHP 8.2 and their composer.json does support it 😕 ?

[lorisleiva]

My bad, I misread the composer.json haha. I merged a PR that adds PHP 8.2 support.

Yeah let's do that, just ping me whenever it's working in production and I'll merge/release. Thank you! 🍺

[dmason30]

@lorisleiva I'm sorry I completely forgot to come back and let you know if the issue was resolved. 🤣

I am happy to report that this PR is working perfectly in production for a couple weeks and we have had no occurrences of this error since.

What I will say is that we are only using AsListener and AsJob in our projects so i can't say for certain if this resolved any issues with AsController.

[lorisleiva]

Thanks! Let's merge that in.