Tucker-Eric / EloquentFilter

An Eloquent Way To Filter Laravel Models And Their Relationships
http://tucker-eric.github.io/EloquentFilter
MIT License
1.72k stars 120 forks source link

Feature request: give us an option to disable camel_case replacement in getFilterMethod #83

Closed kanidjar closed 5 years ago

kanidjar commented 5 years ago

Hi,

First of all, thank you very much for this great package.

However, I noticed something very annoying in the way the getFilterMethod() transforms a key.

    public function getFilterMethod($key)
    {
        return camel_case(str_replace('.', '', $this->drop_id ? preg_replace('/^(.*)_id$/', '$1', $key) : $key));
    }

Actually, using your package with a FormRequest causes the following issue:

<?php

namespace App\Http\Requests;

use Urameshibr\Requests\FormRequest;

class ExampleRequest extends FormRequest
{
    /**
     * Determine if the user is authorized to make this request.
     *
     * @return bool
     */
    public function authorize()
    {
        return true;
    }

    /**
     * Get the validation rules that apply to the request.
     *
     * @return array
     */
    public function rules()
    {
        return [
            'publish_on' => 'date',
        ];
    }
}
namespace App\ModelFilters;

use EloquentFilter\ModelFilter;
use Carbon\Carbon;

class ExampleFilter extends ModelFilter
{
    public function publishOn($publishOn) {
        $this->whereDate('publish_on', Carbon::parse($publishOn));
    }
}

If someone tries to pass "publishOn" instead of "publish_on" in the query string with a bad value, the parameter will not go through the FormRequest validation and will reach the "publishOn" method, causing an exception in the above example.

We should be able to disable the camel_case transformation so only "publish_on" parameter would reach the "publish_on" method.

PS: we've adopted snake_case params for the whole application.

Thank you

kanidjar commented 5 years ago

Also: the solution today is to override the getFilterMethod()

Tucker-Eric commented 5 years ago

Hhhhmmm, you bring up a valid point. What would you suggest in terms of disable camel casing? Initially I would think adding a protected $camelCasedMethods = true property to the model filter but then setting that on every model filter created wouldn't be to far off from adding:

public function getFilterMethod($key)
{
    return $key
}

This may be better solved creating an abstract class that extends ModelFilter with the appropriate logic replaced and extending that class instead of making the same config update per filter.

I'm open to suggestions though.

kanidjar commented 5 years ago

In my opinion, the $camelCasedMethods property seems to be a good idea since it would allow everyone to benefit from the improvements you would make to the method (like the '.' fix).

For instance, in our case, we've only dropped the 'camel_case' instruction so our filters can stick to your documentation as much as possible.

The most annoying part of the current situation is that it can create breaches in applications.

Tucker-Eric commented 5 years ago

@kanidjar I'm open to the update if you wanted to submit a pr?

kanidjar commented 5 years ago

@Tucker-Eric Here it is :)