[9.x] Handle precognition requests by default

[driesvints]

Requires a framework release for laravel/framework#44339

This PR adds the HandlePrecognitiveRequests middleware to the default middleware stack. The goal of this is to make it even more frictionless for people to get started with implementing precognition requests.

Precognition requests are invoked with a Precognition: true header meaning apps that don't explicitly pass this aren't affected. The only thing that's left to do for users is to implement the isPrecognitive checks in their apps. Adding the middleware on a route-per-route basis isn't needed anymore.

People can still turn off this global middleware and implement on a route-per-route basis but I could not see a problem by adding this by default.

[habibalkhabbaz]

Hello @driesvints

By adding HandlePrecognitiveRequests to the default middleware stack, the middleware will try to add the Vary header on the web routes too, which means if we have a response with a binary file such as the following:

return response()->file(Storage::path($payment->attachment));

... the middleware will try to to add the Vary header here too but it will throw an exception because the response here is an instance of \Symfony\Component\HttpFoundation\BinaryFileResponse and not \Illuminate\Http\Response that is expected by appendVaryHeader function

Call to undefined method Symfony\Component\HttpFoundation\BinaryFileResponse::header()

Reference

appendVaryHeader function

    /**
     * Append the appropriate "Vary" header to the given response.
     *
     * @param  \Illuminate\Http\Request  $request
     * @param  \Illuminate\Http\Response  $response
     * @return \Illuminate\Http\Response $response
     */
    protected function appendVaryHeader($request, $response)
    {
        return $response->header('Vary', implode(', ', array_filter([
            $response->headers->get('Vary'),
            'Precognition',
        ])));
    }

response()->file function

    /**
     * Return the raw contents of a binary file.
     *
     * @param  \SplFileInfo|string  $file
     * @param  array  $headers
     * @return \Symfony\Component\HttpFoundation\BinaryFileResponse
     */
    public function file($file, array $headers = []);

The Fix

We can fix this by checking the instance of response parameter in the appendVaryHeader function and just return the response without adding the header because it's not required here.
If you'd like me to submit a PR, I will be happy to do it.

[vini123]

虚幻之物对应着冥冥之路！：《羽》

[timacdonald]

In principle I'm okay with this being applied by default, however some things we should consider:

• This does leak implementation on all routes, even when the feature is not used i.e. someone could look at response headers and see the Vary header contains Precognition and they now know this is a Laravel application powered backend. Of course there are existing ways to determine this, such as the structure of a validation response.
• It allows apps to get into weird states that might not be expected if Precognition is not in the developers mind.

Take this convoluted scenario:

class Middleware
{
    public function handle($request, $next)
    {
        Counter::increment($request->user());

        return $next($request)
    }
}

class Controller
{
    public function show($request)
    {
        AnotherCounter::incrememt($request->user());

        return response();
    }
}

Route::get('foo', [Controller::class, 'show'])->middleware(Middleware::class);

If the middleware is applied globally, then if it isn't considered on this route, I can open up Postman and get these counters out of sync, even if they should not be allowed to within the application i.e. this isn't a route that the front-end uses Precognition features.

Perhaps for a Precognition request the intended functionality of this middleware is to not perform a side-effect:

class Middleware
{
    public function handle($request, $next)
    {
        if (! $request->isPrecognitive()) {
            Counter::increment($request->user());
        }

        return $next($request)
    }
}

My point being that it is a little more developer overhead to consider on every single route / middleware. If it is opt-in it is still a very intentional thing.

In principle I do support this PR, just wanna make sure we consider everything.

[timacdonald]

@habibalkhabbaz I've already addressed the issue you raised: laravel/framework#44424

[habibalkhabbaz]

@habibalkhabbaz I've already addressed the issue you raised: laravel/framework#44424

Thanks!
By the way, you made something awesome to Laravel 💯

[driesvints]

@timacdonald what if we comment out the middleware by default? Like I've done now.

[taylorotwell]

I think it's probably mostly intended that you apply this on the routes you want to be precognitive.