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

Form handling/validating #135

Closed
barryvdh opened this issue Jul 28, 2015 · 14 comments
Closed

Form handling/validating #135

barryvdh opened this issue Jul 28, 2015 · 14 comments

Comments

@barryvdh
Copy link
Contributor

I was looking at this package as a replacement for Symfony Forms in Laravel.
I kind of like Symfony Forms, but it's a pretty much tied to Twig + Symfony conventions.

Something I do like in Symfony is the handling of forms (http://symfony.com/doc/current/book/forms.html#handling-form-submissions)

// just setup a fresh $task object (remove the dummy data)
$task = new Task();

$form = $this->createFormBuilder($task)
    ->add('task', 'text')
    ->add('dueDate', 'date')
    ->add('save', 'submit', array('label' => 'Create Task'))
    ->getForm();

$form->handleRequest($request);

if ($form->isValid()) {
    // perform some action, such as saving the task to the database

    return $this->redirectToRoute('task_success');
}

So this takes the POST data, calls the setters on the Task instance and checks the constraints etc.

In Laravel, I would think of something like this:

$task = new Task();

$form = \FormBuilder::plain([
        'model' => $task,
    ])
    ->add('task', 'text', [
        'rules' => 'required|min:10'
    ]);

$form->handle();

if ($form->isValid()) {
    $task->save();

    return redirect()->back();
}

This would have a few benefits (above the FormRequests)

  • Rules coupled to the Form (so you don't miss a form or add a rule for something that doesn't exist or has a different name)
  • Could possibly use the Label from the field. (Robust way to pass labels to validator #56)
  • Might be safer, because you only set the fields you have defined in your form (vs Input::all() or fillable/guarded)

And would open the door for something I really like from Former (https://github.com/formers/former); Client-side HTML5 validation. Eg. a required rule -> required attribute, max rule -> max-length attribute etc. This would be automatic, so no need to require twice.

What do you think about this? I would be open for helping with this, when you think it's a good idea (and I'm going too switch to this)

@kristijanhusak
Copy link
Owner

Hi @barryvdh ,

That is a great feature, and I also like it. I wanted to implement it at some point, but binding the values from the form to the model is a bit tricky because of the nested child forms/collection types. I just didn't have enough time to find best solution to that.
I will probably have some time this weekend to look into it.

@barryvdh
Copy link
Contributor Author

Okay cool, let me know if you need any help!

About child forms: They would get their own Model/data object, so could be handled similarly by the ChildForm handle method?

And collections are a bit trickier indeed, because Laravel can't really 'prepare' a collection. So I would guess you could just skip it, and let the developer handle that manually?

@kristijanhusak
Copy link
Owner

Will do, thanks!

Yes, that was the initial idea, to provide property that is in Symfony's form called Entity. Probably will use same thing for collection, so it will allow using saveMany http://laravel.com/docs/5.1/eloquent-relationships#inserting-related-models . Really needs some good analysis. When i get to some point, I will probably need help with testing.

@sadekd
Copy link

sadekd commented Jul 31, 2015

look at https://github.com/nette/forms
has great live validation(js) and some nice features, best part of nette ;-)

@barryvdh
Copy link
Contributor Author

Interesting, but live validation shouldn't be very hard if we follow html5 validation specs, that work with any proper is lib.

@kristijanhusak
Copy link
Owner

@barryvdh
I added validation to the package. It works similar as you suggested.

class MyForm extends Form {

    public function buildForm()
    {
        $this
            ->add('name', 'text', [
                'rules' => 'required|min:5'
            ])
            ->add('email', 'email', [
                'rules' => 'required|email'
            ])
            ->add('description', 'textarea', [
                'rules' => 'max:255'
            ]);

    }
}
class Controller {
    public function create()
    {
        $form = \FormBuilder::create('MyForm');

        // This does automatic validation by pulling rules from field options
        // and checking if it passes
        if (!$form->isValid()) {
            return redirect()->back()->withErrors($form->getErrors());
        }

        // If rules and/or messages needs to be modified, it can be done with
        // validate method like this:
        $rules = [
            'name' => 'required'
        ];
        $messages = [
            'name' => 'Name field needs to be populated.'
        ];

        // Returns validator object, can be saved to variable and be used later
        $form->validate($rules, $messages);

        if ($form->isValid()) {
            // Do saving...
        } else {
            return redirect()->back()->withErrors($form->getErrors());
        }
    }
}

Also names in messages are used from labels. For example if you have field:

$this->add('first_name', 'text', [
     'label' => 'First name'
    'rules' => 'required'
]);

Error message for that field will be:

The First name field is required.

Everything is up on dev-master. I would like You to test it out if it works fine, so i draft a release.
Thanks for the idea.

@barryvdh
Copy link
Contributor Author

barryvdh commented Aug 4, 2015

Looks cool!

Perhaps couple suggestions (without actually checking the code)

  • validate() returns the validator? Perhaps name is validator() or createValidator or something (see next point).
    • You could look at the ValidatesRequest Trait. You could use $form->validate() to throw that Exception to make it even easier.
public function create(Request $request)
{
    $form = \FormBuilder::create('MyForm');

    // Validate or throw exception to return the previous page with errors
    $form->validate($request);

    // Save the input
}

Only thing I'm not sure about is if the Request should be handled automatically. Not sure if there would be a use-case where this wouldn't be preferred? Because Laravel doesn't do it automatically, but requires you to inject the request object.

@kristijanhusak
Copy link
Owner

I'm not sure if I want to couple that type of validation to the form object. Many times regular validation is enough, especially on smaller projects. With that trait it should be simple to add that type of validation to the controller with small helper method that is basically a copy of validate from trait:

class Controller {

    use ValidatesRequest;

    protected function validate(Request $request, Form $form, array $rules = [], array $messages = [])
    {
        $validator = $form->validate($rules, $messages);

        if ($validator->fails()) {
            $this->throwValidationException($request, $validator);
        }
    }
}

I was trying to figure out best solution to allow passing the request object on validation, but to use default one if nothing is passed. If i do that in validate method, in some situations could be kind of ugly, for example, when you want only to override some rule without passing a request:

$form->validate(null, ['name' => 'required']);

But I will probably add it that way, so if something needs to be override, request can be passed as first argument anyway.

@barryvdh
Copy link
Contributor Author

barryvdh commented Aug 4, 2015

Just brainstorming some thoughts but:

  • If you expose the rules ($form->getRules()) we can just use the existing trait like before:
    $this->validate($request, $form->getRules().

About the request, we could also give the possibility to let the user change the Request object. $form->setRequest($request). I guess it wouldn't be a very common issue anyways.

@kristijanhusak
Copy link
Owner

Yeah thats a good solution. I will expose getRules, and add setter for request. Thanks!

@kristijanhusak
Copy link
Owner

I added getRules and setRequest methods on the Form class.

If you want, you can override some rules from getRules with parameter:

$form->getRules(['name' => 'max:10']);

Request can be set, but it's not required. If not provided, it will use current request (which should be the same as the one that is passed probably).

@kristijanhusak
Copy link
Owner

Everything is not available in latest stable version (1.6.20). If you run into some issues please let me know. Thanks!

@barryvdh
Copy link
Contributor Author

So the validation part here was picked up, but can we revisit the $form->handle() part?

So filling the model with the (validated) data from the the form (just the allowed values and actual fields on the form).

I feel like it could basically be just $model->forceFill($form->getFieldValues())

Except that you can't fill relations and values that are not directly mapped. Symfony uses a mapped boolean option for this on the field. Would this make sense to include?

    public function handle()
    {
        if ($model = $this->getModel()) {
            foreach ($this->getFieldValues() as $field => $value) {
                if ($this->isMapped($field)) {
                    $model->{$field} = $value;
                }
            }
        }
    }

And of course something to handle nested forms..

@kristijanhusak
Copy link
Owner

@barryvdh sure. That looks ok, i'm just not sure how hard it will be to handle nested forms.

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

3 participants