laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.32k stars 10.95k forks source link

there is a difference between $request and request() #42765

Closed Ham3D closed 2 years ago

Ham3D commented 2 years ago

Description:

when you attach something to $request , you dont have access to it when you call request()->get('something');

Steps To Reproduce:

There is a wired bug in the request. I fetch something inside a policy class (which is loaded inside a FormRequest class) imagine i have to:

$chat = Chat:first();

and then inside my services i need to again get this $chat, but because i have it in the first place, i do this inside policy [here we dont have access to $request, so i used request() helper]:

request()->request->add(['chat' => $chat]);

or

request()->merge(['chat' => $chat]);

the final result is the same (both buggy)

and inside my services, or in controller, etc, i do this:

$chat = $request->get('chat');    // this will be null
$chat = request()->get('chat') // this one has the actual chat model

Test case :

https://github.com/Ham3D/bug-report

there is a test, controller, policy, and request. also I attached an SQLite db just in case.

nielsdos commented 2 years ago

I believe the issue is that you're using $request() as a function call instead of $request as a variable, in the first (buggy) example.

Ham3D commented 2 years ago

I believe the issue is that you're using $request() as a function call instead of $request as a variable, in the first (buggy) example.

no, the bug is there, give it a try!

i fixed the typo.

igorgaming commented 2 years ago

+1. I also have this bug. A workaround its a passing of the current $request to the service.

Upd: i am not sure this is a bug or not, but documentation says: 'The request function returns the current request instance or obtains an input field's value from the current request'.

rodrigopedra commented 2 years ago

Can you please provide a GitHub repository to demonstrate your issue.

You can use this command, if you have the Laravel Installer installed locally:

$ laravel new bug-report --github="--public"

Right now, I could not reproduce. Steps I did:

$ laravel new issue-42765
$ cd issue-42765
$ php artisan make:policy UserPolicy --model=User
$ php artisan serve

Then edit ./routes/web.php to:

<?php

use App\Models\User;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\Auth;
use Illuminate\Support\Facades\Route;

// avoid setting up a database
Auth::login(User::factory()->makeOne()->forceFill(['id' => 1]));

Route::get('/', function (Request $request) {
    return [
        $request->get('chat'),
        request()->get('chat'),
        request('chat'),
    ];
})->can('viewAny', User::class);

And change UserPolicy@viewAny method to:

public function viewAny(User $user)
{
    request()->merge(['chat' => now()->timestamp]);

    return true;
}

Lastly, access http://127.0.0.1:8000 , this is the output:

[1654956237,1654956237,1654956237]

As you can see, all three access methods have the same output.

I tried with request()->request->add(['chat' => now()->timestamp]); inside the Policy and it also output the expected results.

One note: avoid accessing the Request instance outside of routes, Controllers, and Middleware, basically avoid accessing it outside your ./app/Http or ./routes folders. If, for example, you want to check that policy in a job, after login in a user manually as I did above, trying to access the request object might yield an unexptected result.

igorgaming commented 2 years ago

@rodrigopedra Idk how the OP got this bug, but personally I get it when i use custom form request class. All you need its replace the base Request class to the custom form request. E.x: php artisan make:request CustomRequest, after that replace Request in your route to the CustomRequest. You will get input [1654956237,null,null].

Ham3D commented 2 years ago

Can you please provide a GitHub repository to demonstrate your issue.

You can use this command, if you have the Laravel Installer installed locally:

$ laravel new bug-report --github="--public"

Right now, I could not reproduce. Steps I did:

$ laravel new issue-42765
$ cd issue-42765
$ php artisan make:policy UserPolicy --model=User
$ php artisan serve

Then edit ./routes/web.php to:

<?php

use App\Models\User;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\Auth;
use Illuminate\Support\Facades\Route;

// avoid setting up a database
Auth::login(User::factory()->makeOne()->forceFill(['id' => 1]));

Route::get('/', function (Request $request) {
    return [
        $request->get('chat'),
        request()->get('chat'),
        request('chat'),
    ];
})->can('viewAny', User::class);

And change UserPolicy@viewAny method to:

public function viewAny(User $user)
{
    request()->merge(['chat' => now()->timestamp]);

    return true;
}

Lastly, access http://127.0.0.1:8000 , this is the output:

[1654956237,1654956237,1654956237]

As you can see, all three access methods have the same output.

I tried with request()->request->add(['chat' => now()->timestamp]); inside the Policy and it also output the expected results.

One note: avoid accessing the Request instance outside of routes, Controllers, and Middleware, basically avoid accessing it outside your ./app/Http or ./routes folders. If, for example, you want to check that policy in a job, after login in a user manually as I did above, trying to access the request object might yield an unexptected result.

attached a git hub repo with the mentioned bug in the main post

rodrigopedra commented 2 years ago

Thanks for the clarification @Ham3D and @igorgaming . It wasn't clear at first this only happened when using a form request.

I sent PR #42770 to propose a fix to this issue.

Ham3D commented 2 years ago

so the problem still exists, @taylorotwell Closed #42770 , is there any work around this problem ?

rodrigopedra commented 2 years ago

@Ham3D

You can try adding this to your FormRequest's prepareForValidation method:

protected function prepareForValidation()
{
    $this->container->instance('request', $this);
}

Unfortunately this should only work for a single FormRequest.

Adding the following to your AppServiceProvider's boot method would work for all Form Requests, only if you use the can middleware instead of checking the policy on the FormRequest's authorization method.

public function boot()
{
    $this->app->afterResolving(FormRequest::class, function ($request, $app) {
        $app->instance('request', $request);
    });
}

Reason is that the FormRequest's authorize method is called before the ->afterResolving(...) callbacks.

driesvints commented 2 years ago

$request and request() are not the same and this is expected. See the answer by Taylor here: https://github.com/laravel/framework/pull/42770#issuecomment-1156818638