lorisleiva / laravel-actions

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

Actions don't work with Octane #202

Closed jedjones-uk closed 8 months ago

jedjones-uk commented 2 years ago

Hi,

I've been testing actions for use with octane and it seems they don't work properly. The first run of an action goes to the html output, and the second and all subsequent requests to the same endpoint goes to the json output.

I think it has to do with the fact that ControllerDecorator@__invoke is not called subsequent requests but I've not really been able to trace it down to a specific set of code.

The subsequent requests also seem to go direct to the action itself.

Spice-King commented 2 years ago

Going to tag in with the same problem.

As far as I can see, ActionManager's $extended property is not getting cleared between runs, thus on the second run is not calling app()->extend(). Going to see if I find a solution and see about making a PR.

Spice-King commented 2 years ago

Looks like I was right. Debating on how to make a PR, but this is the hack job of a listener does work when added to config/octane.php for the RequestReceived event if you're interested @jedjones-uk.

<?php

namespace App\Listeners;

use Laravel\Octane\Events\RequestReceived;
use Lorisleiva\Actions\ActionManager;

class FixLaravelActionsOnOctane
{
    public function handle(RequestReceived $event)
    {
        $manager = $event->app->get(ActionManager::class);
        $r = new \ReflectionObject($manager);
        $p = $r->getProperty("extended");
        $p->setAccessible(true);
        $p->setValue($manager, []);
    }
}

Octane really seems to lack documentation about best practices for making packages with it, outside of this bit on dependacy injection. Just no mention of the events or the included listeners for them that reset the pins for Laravel and it's first party packages.

jedjones-uk commented 2 years ago

@Spice-King

This is great work for tracing this down to a repeatable solution, I spent a while losing hair trying to figure out just where the key issue was. You fix works fine as a hack but a long term quality solution I think requires 2 changes.

1: Define some helper functions to determine if octane is in use like these: https://github.com/laravel/octane/issues/508#issuecomment-1096937174

2: when octane is in use, skip the $this->isExtending($abstract) call in the extending function for ActionManager

This would avoid the need for reflection and keeps everything in house.

Thanks again.

Spice-King commented 2 years ago

Looped back to this to try and find reasons for the extended property to exist. Unfortunately, the commit with adds everything for it (813570006a063d1b1db4b555e8221f912a1228d9) just has the message of wip with no details around it. Without knowing the thought behind adding it, I don't want to just bypass or remove it in a near unconfitional way.

That said, the only patterns that would use this code code path is commands, controllers and listeners and so an educated guess is probably only there to stop DI issues with those types as the starting point. Going to PR something based on the idea of my external hack in a bit.

ClaraLeigh commented 1 year ago

Been having weird routing issues lately. Dependancy and route binding working 99% of the time but occasionally failing. Turning off octane seems to have cleaned it up, wonder if its related to this

Wulfheart commented 1 year ago

Do you have a reproduction repository?

ClaraLeigh commented 1 year ago

@Wulfheart if you're asking me, I did all the notes and comments I had in the PR #221 Ran out of time to work on it further for the moment

binaryfire commented 1 year ago

Hi all. Should I be avoiding using this package in a complex app that's using Octane? I'm new to Laravel so it'd be hard for me to debug the kind of issues described here. Our app is headless and proxying several external APIs so Octane is essential...

I really like the way this package could simplify our architecture, but this issue has been open since October and another Octane-related issue (https://github.com/lorisleiva/laravel-actions/issues/136) has been open since 2021. So I'm beginning to think it might not play nicely with Octane in general?

dmason30 commented 1 year ago

Hi all, This may have been fixed indirectly in https://github.com/lorisleiva/laravel-actions/releases/tag/v2.6.0 from my PR attempting to fix issues with octane and synchronous listeners/jobs. If someone can check and confirm that would be great.