laravel / framework

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

Can not use request() to get manual set input parameter #44812

Closed lvqingan closed 2 years ago

lvqingan commented 2 years ago

Description:

Can not use request() to get manual set input parameter from a post json request.

Steps To Reproduce:

Custom request class and manual set a extra parameter

<?php

namespace App\Http\Requests;

use Illuminate\Foundation\Http\FormRequest;

class ManualRequest extends FormRequest
{
    public function rules(): array
    {
        return [];
    }

    protected function prepareForValidation()
    {
        $this->getInputSource()->set('foo', 1);
    }
}
Route::post('manual/change', function (\App\Http\Requests\ManualRequest $request) {
    //return request()->input('foo'); // not work
    return $request->input('foo'); // it works
});

Is it related with #44671 ?

driesvints commented 2 years ago

I don't think changing request params at runtime is a sensible thing to do, sorry.

florianoversmake commented 2 years ago

We got the same problem as the original author.

Our application is relying on input manipulation for API Migrations ( renaming attributes etc. ).

I agree that manipulating input data at runtime is hacky and we wouldn't chose the same approach again but this seems to be a big change for a minor version change.

The laravel documentation itself mentions "input transformations" here: https://laravel.com/docs/9.x/requests#input-trimming-and-normalization

@lvqingan

We will use the same approach as the ConvertEmptyStringsToNull middleware until we can get rid of our "API Migrations" entirely. I hope it helps.

meyer59 commented 1 year ago

Hello We found out our application is broken because of this change. We were relying on prepareForValidation() for mergingIfMissing() attributes. We were then using the request() helper to get those attributes.
If you use the injected object request $request this is working fine. A few questions:

To replicate this issue: in your Request file:

   protected function prepareForValidation()
    {
        $this->mergeIfMissing([
            'foo' => 'bar',
        ]);
    }

In your Controller:

 $request->dump();// [ "foo" => "bar"]
 request()->dump();// []

I think this will be a huge breaking change for everyone. We are not using Octane. Thank you

meyer59 commented 1 year ago

@driesvints any consideration to fixing this ? As it seems to be an unwanted breaking change

driesvints commented 1 year ago

@kohlerdominik what are your thoughts here?

kohlerdominik commented 1 year ago

Hi all

I made a bugfix, and I can see now that it might be a breaking change if you relied on the behaviour of the bug. To write back parameters to the request only works for the $json ParameterBag. If you would have a $query ParameterBag or a $request ParameterBag, it wouldn't work. That's why I proposed to make the behaviour consistent.


@lvqingan, request() returns a \Illuminate\Http\Request, while YourFormRequest $request wraps the request in a YourFormRequest. Further, by default Laravel provides a new Instance with every use of the form request. This is why @driesvints said in his first replay:

I don't think changing request params at runtime is a sensible thing to do, sorry.

And he is right. However, I had the same use case as you, therefore I wrote https://packagist.org/packages/lextira/laravel-formrequest-singleton. If you pull this package and follow the installation instructions, you can use a single FormRequest instance. But remember to typehint the correct FormRequest type in every place, request() does not return a FormRequest-instance. but app(YourFormRequest::class) does.


@meyer59

Should we consider in the future that request() is not equal to $request

I think you do not inject \Illuminate\Http\Request for your $request variable. If this is the case, then indeed $request and request() are not the same. Try this code, which should work the way you want: https://laravelplayground.com/#/snippets/6be0d795-785a-43ca-bc63-bee3c615ef2f

Route::get('/', function (\Illuminate\Http\Request $request){
  dump(request()->get('foo'));   // null

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

  dump(request()->get('foo'));   // "bar"
});

Where as this obviously doesn't work: https://laravelplayground.com/#/snippets/5c387fd8-3087-4534-b49b-465455450889

Route::get('/', function (MyFormRequest $request){
  dump(request()->get('foo'));   // null

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

  dump(request()->get('foo'));   // null

  dump($request->get('foo'));   // "bar"
});
taylorotwell commented 1 year ago

I'm really torn with this one. I do think the original PR fixed a genuine bug - but I think probably people were relying on this behavior. I have reverted it for now and we can maybe revisit this on Laravel 10.x.

driesvints commented 1 year ago

@kohlerdominik maybe attempt a PR at 10.x? Thanks everyone for your feedback on this.

lk77 commented 1 year ago

@kohlerdominik so in the end, when using custom requests, $request should be prefered over request() ?

kohlerdominik commented 1 year ago

I'm really torn with this one. I do think the original PR fixed a genuine bug - but I think probably people were relying on this behavior. I have reverted it for now and we can maybe revisit this on Laravel 10.x.

Hi @taylorotwell and @driesvints

This bug is a terrible edge case to rely on, IMO. It works !only! for JSON-requests. But as it is there now, fixing it should not break current behavior without breaking changes release. So, I agree, that this is better off in the next major release.

Right now, I can’t redirect it, as 10.x is not yet synced with 9.x with the revert. But I will definitively suggest it again for 10.x

kohlerdominik commented 1 year ago

@kohlerdominik so in the end, when using custom requests, $request should be prefered over request() ?

Hi

What do you mean with custom requests? If you mean a form request, then you need to understand how the mechanic works:

/// Like this, we will have a instance of Illuminate\Http\Request for $request, because we asked for this
Route::get('/', function (Illuminate\Http\Request $request){
    $request;   // instanceOf 'Illuminate\Http\Request'
});

// but like this, $request will be an instance of MyFormRequest, because you type-hinted that you want an instance of it
Route::get('/', function (MyFormRequest $request){
    $request;   // instanceOf 'MyFormRequest'
});

// While, anywhere you use request(), you will ALWAYS get an instance of Illuminate\Http\Request, you can't change that
Route::get('/', function (){
    $request = request();   // instanceOf 'Illuminate\Http\Request'
});

// However, you can ask your DI container for an instance of MyFormRequest by asking it explicitly:
Route::get('/', function (){
    $request = app(MyFormRequest::class)   // instanceOf 'MyFormRequest'
});
driesvints commented 1 year ago

@kohlerdominik I've just merged 9.x into master for ya 👍