laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 28 forks source link

[Proposal] add casts to FormRequests #846

Open NoelDeMartin opened 7 years ago

NoelDeMartin commented 7 years ago

I have been working on slimming down my controllers and something I found that could be improved is input request casting.

Let me illustrate with an example. I have a form where one field is a datetime, this form is submitted through ajax and in order to prevent any issues with the timezone, I submit this field as a unix timestamp. As a consequence, I need to do the following on my controller:

$dueDate = Carbon::createFromTimestamp(request('due_date'));

I think a good solution would be to have a $casts property in the form request (using the same concept as $casts in eloquent models). An example implementation would be the following:

class CreateTask extends FormRequest {

    protected $casts = [
        'due_date' => 'date'
    ];

    public function authorize()
    {
        return true;
    }

    public function rules()
    {
        return [
            'name'     => 'required|string',
            'due_date' => 'required'
        ];
    }

}

I have started working on this as a proof of concept, I'll be using this in my projects but I think it'd be great if something like this was supported in the platform. You can see my work on this branch: https://github.com/NoelDeMartin/framework/tree/proposal-requests-input-casting

I am aware of some existing solutions / proposals that can solve the same problem:

sisve commented 7 years ago

Are you sure that request('due_date') queries your FormRequest object, and not the Request object?

This can currently be easily implemented as getter methods.

class CreateTask extends FormRequest {
    public function giefMyDueDate() {
        return Carbon::createFromTimestamp($this->input('due_date'));
    }
}

function myAction(CreateTask $request) {
    dd($request->giefMyDueDate());
}
NoelDeMartin commented 7 years ago

@sisve In the proof of concept I have implemented (you can see it on the branch I linked), what I do is that I replace the value in the request using the merge method on the Request object, so it'll be casted inside the request.

I have seen how eloquent casts attributes and it's also doing the same, for example if you dd() an eloquent model, you will see the values inside the $attributes property are casted, so it isn't something that happens when using getters.

I also think it's a better approach since it prevents the controller from knowing any specifics (which input should I retrieve with getters and which using the request() helper?). And this was the whole purpose of this approach, slimming down controllers and having this kind on logic inside the request class, which is also where the validation rules are defined so it's everything nicely encapsulated and once the controller method is called, it only needs to consume the form data.

NoelDeMartin commented 7 years ago

After looking further into this, I found out that using request() helper only works in the scenario of having a json request.

Let me explain. Requests have different input sources (query parameters, request body, json, etc.) and depending on the type of request, one of those is used to retrieve input:

https://github.com/laravel/framework/blob/5.5/src/Illuminate/Http/Request.php#L336-L343

When having a json request, this is working because FormRequests always use the same ParameterBag instance for the json input source, as seen here:

https://github.com/laravel/framework/blob/5.5/src/Illuminate/Foundation/Providers/FormRequestServiceProvider.php#L59

However, when the request is not json, the ParameterBag instance is unique to each Request. In that case, the request obtained from the container will not have access to the same ParameterBag as the FormRequest.

In the end, then, the casted values can only be retrieved with security from the FormRequest instance who performed the validation / casting. This may not be a problem but it certainly complicates things. I'm not sure if there'd be any way to propagate this casts to all the instances, but I'm not sure if it would make sense.

scorgn commented 4 years ago

I want to tag onto this idea. Another cool idea would be to be able to use the casts in the same way you use the rules in, with nested arrays. For example "users.*.created_at" => "date",.

gvanto commented 4 years ago

Any developments on this? I have an boolean input which is coming through as '1' (since I can't send a boolean with PostMan) and a prevalidation cast as NoelDeMartin is suggesting would be super handy here - unless there's another way of casting prior to validation?

yob-yob commented 3 years ago

this Idea is quite good...

I wanted to cast my File-Input to be casted directly as an Uploaded File Class.. Since I'm using Filepond as my File uploader.. I only need to send the Temporary File path of the file... and every time I sent my request I need to initialize it as an UploadFile object..

So, Any updates on this? or maybe a package I could use to help me achieve this task? I could even help in developing one.. is your proof of concept only for that purpose? or are you trying to make it a package?

NoelDeMartin commented 3 years ago

I was using this in production when I opened the issue, but I haven't used it recently so I have no plans to make a package.

Anyone is welcome to grab my code and make a package, not need to give attribution or anything :).