Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Constructor middleware broken by #589 #594

Closed
aaronhuisinga opened this issue Oct 4, 2022 · 8 comments
Closed

Constructor middleware broken by #589 #594

aaronhuisinga opened this issue Oct 4, 2022 · 8 comments
Assignees
Labels

Comments

@aaronhuisinga
Copy link

  • Octane Version: 1.3.2+
  • Laravel Version: 9.32.0
  • PHP Version: 8.1.x
  • Server & Version: Laravel Vapor (whichever the default is)
  • Database Driver & Version: MySQL 8

Description:

After #589 was merged, we've had an issue causing us to revert to v1.3.1. In several of our applications running Octane, we create a BaseController that allows us to reference the current user via $this->user in all of our Controllers that extend it. The code we use is the following:

<?php

namespace App\Http\Controllers;

use Illuminate\Foundation\Auth\Access\AuthorizesRequests;
use Illuminate\Foundation\Bus\DispatchesJobs;
use Illuminate\Foundation\Validation\ValidatesRequests;
use Illuminate\Routing\Controller as BaseController;

class Controller extends BaseController
{
    use AuthorizesRequests;
    use DispatchesJobs;
    use ValidatesRequests;

    /**
     * The authenticated User for this request.
     *
     * @var \App\Models\User
     */
    protected $user;

    public function __construct()
    {
        $this->middleware(function ($request, $next) {
            $this->user = auth()->user();

            return $next($request);
        });
    }
}

For some reason, #589 is causing $this->user to always be empty. The value is set in the constructor, but appears not to persist so that it can be referenced by methods in Controllers extending this base Controller.

For example, the following will throw an exception saying that we cannot access the $user property until it has been initialized:

<?php

namespace App\Http\Controllers\API;

use App\Http\Controllers\Controller;
use Illuminate\Http\Request;

class ShowDashboardStats extends Controller
{
    /**
     * Get stats to be displayed on the Dashboard.
     *
     * @param Request $request
     *
     * @return \Illuminate\Http\JsonResponse
     */
    public function __invoke(Request $request)
    {
        return response()->json([
            'data' => [
                'user' => $this->user->id,
            ],
        ]);
    }
}

I'm not sure that this is an intended consequence of flushing the controller, but this is a nice little helper we've been using in Octane.& non-Octane Laravel applications for years, and we've never had an issue with it until now.

Is this the intended behavior now and something that we should approach differently? Is this a bug caused by flushing the Controller? It seems to me that this is something that should work correctly.

@nunomaduro nunomaduro self-assigned this Oct 4, 2022
@driesvints driesvints added the bug label Oct 6, 2022
@rndb8418
Copy link

We're also encountering this issue when trying to set up state on the controller for the request via middleware in the constructor (i.e. loading something common to all methods of a constructor), the data is correctly set but when it invokes the actual controller method the data is no longer set.

As far as I'm aware, that's how we're intended to set state since the introduction of middleware.

I'm wary of reverting to a prior version of Octane, as issue that PR #589 fixed implied that there was a bug that could cause a rather serious issue:

This issue seems to not be limited to SocialiteManager, but is something we also experienced with AuthManager, resulting in people being logged in on others accounts.

I'm unsure if that was specific to how they were working with Octane or not though.

@rndb8418
Copy link

Further to my previous comment, it looks like the middleware might being ran on the old controller instance or something peculiar like that. This is some rudimentary logging to show a similar flow (middleware in a base controller's constructor, then a method within a controller that extends it).

[2022-10-25 14:40:45] local.INFO: event: RequestReceived
[2022-10-25 14:40:45] local.INFO: BaseController: __construct
[2022-10-25 14:40:45] local.INFO: BaseController: middleware
[2022-10-25 14:40:45] local.INFO: ControllerWithMethod: create
[2022-10-25 14:40:46] local.INFO: event: RequestTerminated
[2022-10-25 14:40:46] local.INFO: ApplicationGateway: flushing
[2022-10-25 14:40:48] local.INFO: event: RequestReceived
[2022-10-25 14:40:48] local.INFO: BaseController: __construct
[2022-10-25 14:40:48] local.INFO: BaseController: middleware
[2022-10-25 14:40:48] local.INFO: ControllerWithMethod: store (validation fails on purpose, redirect to create)
[2022-10-25 14:40:48] local.INFO: event: RequestTerminated
[2022-10-25 14:40:48] local.INFO: ApplicationGateway: flushing
[2022-10-25 14:40:50] local.INFO: event: RequestReceived
[2022-10-25 14:40:50] local.INFO: BaseController: __construct
[2022-10-25 14:40:50] local.INFO: BaseController: middleware
[2022-10-25 14:40:50] local.INFO: ControllerWithMethod: create
[2022-10-25 14:40:50] local.INFO: event: RequestTerminated
[2022-10-25 14:40:50] local.INFO: ApplicationGateway: flushing
[2022-10-25 14:40:53] local.INFO: event: RequestReceived
[2022-10-25 14:40:53] local.INFO: BaseController: __construct
[2022-10-25 14:40:53] local.INFO: BaseController: middleware
[2022-10-25 14:40:53] local.INFO: ControllerWithMethod: store (validation fails on purpose, redirect to create)
[2022-10-25 14:40:53] local.INFO: event: RequestTerminated
[2022-10-25 14:40:53] local.INFO: ApplicationGateway: flushing
[2022-10-25 14:40:53] local.INFO: event: RequestReceived
[2022-10-25 14:40:53] local.INFO: BaseController: middleware
[2022-10-25 14:40:53] local.INFO: BaseController: __construct
[2022-10-25 14:40:53] local.INFO: ControllerWithMethod: create
[2022-10-25 14:40:53] local.ERROR: errors due to data not being set via middleware
[2022-10-25 14:40:55] local.INFO: event: RequestTerminated
[2022-10-25 14:40:55] local.INFO: ApplicationGateway: flushing

At the end (the ERROR line), is where it encounters the data not set (it seems to consistently fail after a page load, form submission, redirect, form submission, fail). The __construct log is occuring after the log entry from the middleware on that failed request, whereas the others it always occurs first.

<?php

namespace App\Http\Controllers;

use Illuminate\Routing\Controller;

abstract class BaseController extends Controller
{
    public function __construct()
    {
        info('BaseController: __construct');
        $this->middleware(function ($request, $next) {
            info('BaseController: middleware');
            
            // Set data here

            return $next($request);
        });
    }
}
<?php

namespace App\Http\Controllers;

use Illuminate\Http\Request;

class RegisterController extends BaseController
{
    public function create()
    {
        info('ControllerWithMethod: create');
        
        // access data

        return view('form');
    }
    
    public function store(Request $request)
    {
        info('ControllerWithMethod: store (validation fails on purpose, redirect to create)');
        
        // validate data - fail & return to previous page
    }
}

@nunomaduro
Copy link
Member

@aaronhuisinga @rndb8418 Can you update to the latest version of Laravel Framework and Octane, and tell us if you are still experience any issues?

@rndb8418
Copy link

@nunomaduro Yes on latest versions (laravel/framework: v9.37.0 and laravel/octane: v1.3.5) the issue is still present, downgrading to laravel/octane: v1.3.1 fixes the issue.

@rndb8418
Copy link

I've done a completely fresh install like:

  1. laravel new [project]
  2. composer require laravel/octane
    1. roadrunner & install binary
  3. php artisan make:controller HomeController
  4. Update web.php to be:
<?php

use App\Http\Controllers\HomeController;
use Illuminate\Support\Facades\Route;

Route::get('/', HomeController::class);
  1. Update app/Http/Controllers/Controller.php to be:
<?php

namespace App\Http\Controllers;

class Controller extends \Illuminate\Routing\Controller
{
    protected array $example;

    public function __construct()
    {
        $this->middleware(function ($request, $next) {
            $this->example = ['foo' => 'bar'];

            return $next($request);
        });
    }
}
  1. Update app/Http/Controllers/HomeController.php to be:
<?php

namespace App\Http\Controllers;

class HomeController extends Controller
{
    public function __invoke()
    {
        var_dump($this->example);
    }
}
  1. Run php artisan octane:start
  2. Refresh the page in your browser a few times, it'll eventually error saying that Typed property App\Http\Controllers\Controller::$example must not be accessed before initialization

Screenshot of octane request log:
image

@driesvints
Copy link
Member

We've sent in some fixes for this. These will be released on Tuesday.

@nunomaduro
Copy link
Member

Fixed on the most recent version of Laravel.

@adampatterson
Copy link

adampatterson commented Sep 9, 2024

If anyone is trying to use Octane with a fresh install of Laravel 11, you need to modify your controllers.

From abstract class Controller to class Controller extends \Illuminate\Routing\Controller

The error was something like "Target [App\Http\Controllers\Controller] is not instantiable.".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants