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

[NEW][Features] Added support for field filters and request input values mutating. #376

Merged
merged 6 commits into from
Aug 31, 2017

Conversation

unckleg
Copy link
Contributor

@unckleg unckleg commented Aug 27, 2017

I've implemented something similary to Zend form filters, so now developers can filter data that is passed through Form.

Added:

Kris\LaravelFormBuilder\Fields\FormField:

  1. Property:
    • filtersOverride
  2. Methods:
    • setFilters(array $filters)
    • getFilters()
    • addFilter($filter)
    • removeFilter($name)
    • removeFIlters(array $filterNames)
    • clearFilters()

Kris\Form:

  1. Property:
    • lockFiltering
  2. Methods:
    • filterFields
    • getFilters
    • lockFiltering
    • isFilteringLocked

Kris\LaravelFormBuilder\Events\AfterFormCreation

  1. Methods:
    • filterFields

So now you can bind filters/filter to form field like this:

class SongForm extends Form
{
    public function buildForm()
    {
        $stringTrim = new StringTrim();
        $stripTags  = new StripTags();

        $this
            ->add('name', 'text', [
                'rules' => 'required',
                'filters' => [
                    $stringTrim,
                    $stripTags
                   // Or by Filter aliases/names
                   'StringTrim',
                   'StripTags'
                ],
                // This is used to allow override already assigned filters with same alias/name.
                'filters_override' => true,
            ])
        ;

        $this->getField('name')->removeFilters([
            'StringTrim',
            'StripTags'
        ]);
    }
}

Modify fields:

$this
    ->getField('name')
    ->addFilter(new Integer())
    ->removeFilters([
        'StringTrim',
        'StripTags',
        $filterObject
    ])
    ->removeFilter('StringTrim')
    ->setFilters([
        new SomeFilter(),
        new SecondFilter(),
        new FilterFromPackageCollection()
    ])
;

And if you e.g StringToUpper filter for field name and you passed in post this value: "some string"
the value you'll get in request is mutated. "SOME STRING".

@kristijanhusak
Copy link
Owner

@unckleg This is great! Everything looks really good, just few minor things i want to change, just some code reorganization, nothing special. Will start a review and comment out the parts. Thanks a bunch!

@kristijanhusak
Copy link
Owner

@unckleg This is great! Everything looks really good, just few minor things i want to change, just some code reorganization, nothing special. Will start a review and comment out the parts. Thanks!

@unckleg
Copy link
Contributor Author

unckleg commented Aug 28, 2017

@kristijanhusak Good to hear that!

I know that inital filterFields method calling from onFormCreate event is not place for that, I left it there just for showcase for now and maybe some parts of FilterResolver.

I've planned many times to do PR and I somehow found time to do it yesterday.
I've been using package for a long time and Filters was the thing that was missing for perfect circuit.
Waiting for reviews!

@unckleg
Copy link
Contributor Author

unckleg commented Aug 28, 2017

@kristijanhusak We could also start writing our Collection filters before Merging.
The list is huge, so give the order when to start :)

* @return void
*/
public function __construct(Form $form) {
$this->form = $form;
$this->filterFields();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to move this out of this event to the same place where the event is fired. I would like this event to be clean.

* @throws Exception\UnableToResolveFilterException
* @throws Exception\InvalidInstanceException
*/
public static function instance($filter)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could remove some indentation here by returning early and returning the filter from the validate method. Here's how it could be written:

    public static function instance($filter)
    {
        if (!is_string($filter)) {
            return self::validateFilterInstance($filter);
        }

        if (class_exists($filter)) {
            return self::validateFilterInstance(new $filter());
        }

        if ($filter = FilterResolver::resolveFromCollection($filter)) {
            return self::validateFilterInstance($filter);
        }

        throw new UnableToResolveFilterException();
    }

    /**
     * @param $filter
     *
     * @throws \Exception
     *
     * @return mixed
     */
    private static function validateFilterInstance($filter)
    {
        if (!$filter instanceof FilterInterface) {
            throw new InvalidInstanceException();
        }

        return $filter;
    }

@kristijanhusak
Copy link
Owner

yeah you can do that also :) I finished reviewing, you can start :)

@kristijanhusak
Copy link
Owner

@unckleg also, it would be great to write down some tests for this if you got time :)

@unckleg
Copy link
Contributor Author

unckleg commented Aug 28, 2017

@kristijanhusak I'll write tests first for new functionalities and than starting with Collection filters.

@unckleg
Copy link
Contributor Author

unckleg commented Aug 30, 2017

@kristijanhusak There 'll be small [CS] changes related to Filters, I'm thinking of simplifying the filter class names or aliases (StringToLower -> Lowercase, StringToUpper -> Uppercase, StringTrim -> Trim) maybe..

Also there should be some method & property for storing unmutated/pre-filtered field values.
So we can at some point get the original value from the input. Or also to demutate (all values)/(field value) explicitly.

Some suggestions ?

@kristijanhusak
Copy link
Owner

@unckleg yeah those names and aliases look fine. Both are ok if you ask me, it's up to you to choose.

Storing original value would be great, in case there is a need to get it.

Everything looks ok so far, you can add that original value stuff and choose the final naming. After that, we can merge this in :)

@unckleg
Copy link
Contributor Author

unckleg commented Aug 30, 2017

@kristijanhusak Finished everything planned for this commit i think you can merge it now.
What were you planning for the documentation ?

Todo:

  • Write tests for all filters
  • Write tests for retrieving Raw/Unfiltered field values
  • Increase filter collection
  • Write documentation

@kristijanhusak
Copy link
Owner

@unckleg Thanks, everything looks great!

Yeah some documentation would be great for this. Docs are on the gh-pages branch. Can you add some docs there, if you can figure out how it works? I'm really short on time lately.

Merging this to master. Won't release new version yet, but probably soon.

Thanks a lot!

@kristijanhusak kristijanhusak merged commit 824b2e1 into kristijanhusak:master Aug 31, 2017
@unckleg
Copy link
Contributor Author

unckleg commented Aug 31, 2017

@kristijanhusak Great!
I'm going to vacation in few days, but I will definitely write documentation for new Features very soon.

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

Successfully merging this pull request may close these issues.

None yet

3 participants