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

Enhancement Suggestion - Text filters or sanitization rules #14467

Closed
kamranahmedse opened this issue Jul 25, 2016 · 15 comments
Closed

Enhancement Suggestion - Text filters or sanitization rules #14467

kamranahmedse opened this issue Jul 25, 2016 · 15 comments

Comments

@kamranahmedse
Copy link

kamranahmedse commented Jul 25, 2016

Any plans on adding text filters or a similar way of sanitization?

Sometimes you repeatedly have to do some same kind of sanitization or formatting on the received input. For example, trimming the input, making it lowercase etc.

What if we enhance the validator to accept filters to apply i.e. something like

$rules = [
    'email': 'required|filter:lower,trim',
    'name': 'required|filter:strip_tags',
    'attempts': 'int',
    'amount': 'double'
];

Or we may implement something different altogether.

A different service may be e.g. if we pass it the request object, it will mutate the data in the request itself.

$sanitizer = Sanitizer::make($request, [
     'email': 'lower|trim',
     'username': 'strip_tags',
     'attempts': 'int',
     'amount': 'double'
]);
$sanitizer->sanitize();

// `$request` will now have sanitized/filtered data set in place.

If array is passed, items will be mutated and returned i.e.

$sanitizer = Sanitizer::make($input, [
     'email': 'lower|trim',
     'username': 'strip_tags',
     'attempts': 'int',
     'amount': 'double'
]);
$input = $sanitizer->sanitize();

// `$input` will now have sanitized/filtered data set in place.

I am in the last phases of releasing a Laravel package for this (using second method), thought I should ask if I may create a PR with this.

@GrahamCampbell
Copy link
Member

Thanks for getting in touch @kamranahmedse. Ping @taylorotwell.

@mdavis1982
Copy link

mdavis1982 commented Jul 25, 2016

I discussed a similar proposal with @taylorotwell yesterday. My specific example was in a Request class, but it could be added as an argument to a normal Validator if required.

My proposal looks like the following (inside a Request class):

public function filters()
{
    return [

        'pre' => [
            'first_name'       => ['trim', 'strip_spaces'],
            'reference_number' => ['trim'],
        ],

        'post' => [
            'reference_number' => ['uppercase'],
        ]

    ];
}

In the above example, before validation, first_name would be trimmed and have spaces removed, and reference_number would be trimmed.

After validation passes, the data available on the Request would have the reference_number uppercased.

One thing to mention is whether or not the pre rules apply to the data that is set on the Request after validation is successful. Personally, I think I would prefer to only have the result of the post filters applied to the original input. This gives the opportunity for different processing to happen before validation than to the final result which might be useful in a case where you need to trim to do some comparison, but you actually want to store the original data the user entered.

For example, this might look like the following:

public function filters()
{
    return [

        'pre' => [
            'first_name' => ['trim', 'uppercase'],
        ],

        'post' => [
            'first_name' => ['trim', 'titlecase'],
        ]
    ];
}

// User enters: 'mc dougall   '
// Validated value is: 'MC DOUGALL'
// Returned value after validation is: 'Mc Dougall'

I don't mind the repetition of the rules in both the pre and post arrays and think it makes the implicit explicit and avoids confusion.

I think this separation of these filters makes it obvious what they do, and separates them out from the $rules array to keep that clean.

@phroggyy
Copy link
Contributor

Hmm... I like the general idea of filters. However, I don't believe validation should modify the output. Now, the pre concept mentioned by @mdavis1982 is interesting. However, I don't quite see the point, as all that's doing is modifying the input for validation, in which case you might as well just use another validation rule, no? I do however like the idea of post (and especially ensure that the filter is applied after validation).

@rizqidjamaluddin
Copy link

rizqidjamaluddin commented Jul 25, 2016

I like the idea of a laravel-y way of adding sanitizers (probably integrated into Request objects), but I don't think they belong in validators. Sanitizing and validating are different concepts and should be kept apart.

I agree with having the separate Sanitizer class that looks similar to a validator, but returns mutated data instead of validated data, and have it be built-in to form requests.

Arrays should probably be an acceptable input, so we'd need to have the sanitize() function return the sanitized data rather than mutating it by reference.

$sanitizer = Sanitizer::make($request->all(), [
     'email': 'cast:int|trim',
     'attempts': 'cast:int',
     'amount': 'cast:double'
]);
$input = $sanitizer->sanitize();

From a UX perspective, I'm a heavy proponent of sanitizing + sensible defaults over validation, so I'd love to see this pass.

@jlem
Copy link

jlem commented Jul 25, 2016

Agreed with @phroggyy and @rizqidjamaluddin. Validation and sanitation are separate concepts and should have separate APIs. But having a sanitation option for input would be quite awesome (especially if you can extend it like you can the validators so you could write your own re-usable sanitizers).

@mdavis1982
Copy link

I agree, too... I didn't know if there was an API parity requirement between the Validator and the Request classes (like how you specify rules() in the Request class, but pass $rules to the Validator).

I think this is an ideal opportunity to apply filters to data - but I would like to see it 'baked in' to the Request class so that you can just specify a filters() method and return the filters the same way you do validation rules and messages.

@rizqidjamaluddin
Copy link

Funny enough, I'm not usually one to enjoy shortcuts (and I certainly am not a fan of pseudo-magic strings) - but I think a laravel-style sanitizer fits right into many workflows and I do find myself doing a lot of triming and intvaling user input - often a good chunk of a controller is just this. Having a sanitizing shortcut would mean I can more comfortably do $request->only() without having a bunch of pre-processing scripts.

One thing to carefully note is that this practice mutates data before it gets validated, which means validation error messages may not match the provided input from the user.

@kamranahmedse
Copy link
Author

kamranahmedse commented Jul 25, 2016

@rizqidjamaluddin ^^What I had in my mind was passing the request object. And then it will automatically mutate the data in the request object.

But yeah, it could be made as to mutate both ways i.e. if the passed argument is a request object, items in request will be mutated and set in the request. Or if the passed parameter is an array, the given array will be mutated and returned. (Updated the original description)

@rizqidjamaluddin
Copy link

I think binding sanitizing to the request mechanism is too far interlocked - it's not uncommon to want to sanitize other data, like incoming information from an API request, parsing an inbound email webhook or just people who still do $request->all. I'd suggest that having a $sanitize property in a FormRequest should suffice for people who want the automagicky parts.

@franzose
Copy link

@rizqidjamaluddin What do you think about using a middleware in case of filtering?

@franzose
Copy link

At first view, middleware might be a thing.

@themsaid
Copy link
Member

Hey everyone,

Anyone willing to make a PR for this?

@phroggyy
Copy link
Contributor

@themsaid I'll look into it this weekend!

@themsaid
Copy link
Member

@phroggyy I'm going to close this issue since it's a feature request, but looking forward to your PR when you have time :)

@phroggyy
Copy link
Contributor

@themsaid yep, started on it this morning!
On Tue, 27 Sep 2016 at 16:56, Mohamed Said [email protected] wrote:

@phroggyy https://github.com/phroggyy I'm going to close this issue
since it's a feature request, but looking forward to your PR when you have
time :)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#14467 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AG65g7IJpNwMFHt5mPO1hsPZno-xrdpKks5quSrwgaJpZM4JT2Lz
.

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

No branches or pull requests

8 participants