Migrated to new validation system; old tests don't pass because of empty request

[marcoraddatz]

I migrated all validation rules to your new system, which I like a lot. However, my old tests won't pass, because I'm told, the required fields haven't been filled out – but they are.

Here's a small example:

AddressForm (I have two more fields and a submit button in this class, but have cut them for this example):

$this
    ->add('address_1', 'text', [
            'required'  => true,
            'label'     => trans('label.address_1'),
            'rules'     => 'required|max:255',
        ]
    )
    ->add('address_2', 'text', [
            'required'  => false,
            'label'     => trans('label.address_2'),
            'rules'     => 'max:255',
        ]
    )
    ->add('city', 'text', [
            'required'  => true,
            'label'     => trans('label.city'),
            'rules'     => 'required|max:255',
        ]
    )
    ->add('zip_code', 'text', [
            'required'  => true,
            'label'     => trans('label.zip_code'),
            'rules'     => 'required'
        ]
    );

AddressController → update:

if (!$validation->isValid()) {
    print_r($request->all());
    print_r($validation->getErrors());
    die();
    return $this->redirectWithErrors($validation);
}

[...]

Test:

$address = factory(App\Address::class)->make()->toArray();
$address['address_1'] = 'User 1 Address 1 - Updated';

$this
    ->actingAs(User::find(3))
    ->visit('/address/1/edit')
    ->see(trans('address.edit.title'))
    ->see('User 1 Address 1')
    ->submitForm(trans('label.submit'), $address) // Here's the problem
    ->see(trans('address.edit.success'))
    ->see('User 1 Address 1 - Updated')
;

Test output:

Array
(
    [_method] => PUT
    [_token] => qxad1oeCsKvILU5vFzGNgJ5JkNC6hMh93MtiJ1BV
    [address_1] => User 1 Address 1 - Updated
    [address_2] => Apt. 602
    [city] => Marienberg
    [zip_code] => 82756
    [country_id] => 276
)
Array
(
    [address_1] => Array
        (
            [0] => The Address Field 1 field is required.
        )

    [city] => Array
        (
            [0] => The City field is required.
        )

    [zip_code] => Array
        (
            [0] => The ZIP code field is required.
        )

    [country_id] => Array
        (
            [0] => The Country field is required.
        )

)

You can see that the data exists. Also, if I test in the browser, everything works fine. While debugging, I found out, that $this->getRequest()->all() is empty (Form.php, Line 922). Do you guys have any clue, what it could be?

Thanks you guys for this amazing package!

[kristijanhusak]

Can you please provide code where you do the validation with form?

Did you try setting the request manually with setRequest?

[marcoraddatz]

Hey Kristijan,

thanks for the reply. setRequest helps me:

Not working

public function update($id, Request $request)
{
    $form = $this->form(AddressForm::class);

    if (!$form->isValid()) {
        return $this->redirectWithErrors($form);
    }

    else
    {
    }
}

Working

public function update($id, Request $request)
{
    $form = $this->form(AddressForm::class);
    $form->setRequest($request);

    if (!$form->isValid()) {
        return $this->redirectWithErrors($form);
    }

    else
    {
    }
}

---

However, sometimes I need to set the setRequest, sometimes I don't need to do so. I currently don't understand the dependencies on that. In this example create/store needs the setRequest, edit/update doesn't.

Controller:

public function create(FormBuilder $formBuilder)
{
    $form = $formBuilder->create(FeeForm::class, [
        'method'    => 'POST',
        'url'       => route('fee.store')
    ]);

    return view('fee.create', compact('form'));
}

public function store(Request $request)
{
    $form = $this->form(FeeForm::class);
    $form->setRequest($request);

    if (!$form->isValid()) {
        return $this->redirectWithErrors($form);
    }

    else
    {
        [...]
    }
}

public function edit($id, FormBuilder $formBuilder)
{
    $form = $formBuilder->create(FeeForm::class, [
        'method'    => 'PUT',
        'url'       => route('fee.update', $id),
        'model'     => Fee::findOrFail($id)
    ]);

    return view('fee.edit', compact('form'));
}

public function update($id, Request $request)
{
    $fee    = Fee::findOrFail($id);
    $form   = $this->form(FeeForm::class, [
        'model' => $fee
    ]);

    if (!$form->isValid()) {
        return $this->redirectWithErrors($form);
    }

    else
    {
        [...]
    }
}

FeeForm:

public function buildForm()
{
    $id = $this->model && $this->model->id ? $this->model->id : null;

    $this
        ->add('name', 'text', [
            'required'  => true,
            'label'     => trans('label.name'),
            'rules'     => 'required|max:32|unique:fees,name,' . $id,
        ])
        ->add('display_name', 'text', [
            'required'  => true,
            'label'     => trans('label.display_name'),
            'rules'     => 'required|max:64',
        ])
        ->add('description', 'text', [
            'required'  => true,
            'label'     => trans('label.description'),
            'rules'     => 'required|max:255',
        ])
        ->add('module_id', 'select', [
            'choices'   => Module::modules(),
            'required'  => true,
            'label'     => trans('label.module'),
            'rules'     => 'required|integer'
        ])
        ->add('price', 'number', [
            'required'  => true,
            'label'     => trans('label.price'),
            'attr'      => [
                'min'   => '0.00',
                'step'  => '0.01'
            ],
            'rules'     => 'required|numeric'
        ])
        ->add(trans('label.submit'), 'submit', [
                'attr' => ['class' => 'btn btn-primary']
            ]
        )
    ;
}

[kristijanhusak]

Hmm, that's strange. Every time form is instantiated current instance of request is used. That is probably not handled properly in tests because Request for tests are changed in the meantime.

Can you try dumping both requests in the update method and see what you get in the tests? To check if they are the same, or what's different, and so on. Something like this:

public function store(Request $request)
{
    $form = $this->form(FeeForm::class);

    var_dump($form->getRequest());
    var_dump($request);
}

Also i'm not sure if this is only issue with the Jeffrey's testing library that is part of 5.1, or it also happens in plain phpunit tests.

I would really appreciate if you could test these things and let me know what you get.

[marcoraddatz]

I did this on the store method and this is what I got: https://www.diffnow.com/?report=hfdba (didn't know the best way to provide them both).

The update method is handled the same way: https://www.diffnow.com/?report=klwn8

Let me know, if you need any more examples, code or whatever!

[kristijanhusak]

It's definitely different request instance in the tests, that's why it's not working.

I would suggest you stick with setRequest in the post/put/delete routes because that makes sure the proper request is used when validating the form. You can chain the setter to make it a bit more readable:

public function store(Request $request)
{
    $form = $this->form(FeeForm::class)->setRequest($request);

    if (!$form->isValid()) {
        return $this->redirectWithErrors($form);
    }

    else
    {
        [...]
    }
}

I will probably add a bit more readable method for validating specific request, instead of setter. Will probably use Symfony's handleRequest name. Something like this:

$form = $this->form(FeeForm::class);

$form->handleRequest($request);

if (!$form->isValid()) {
    return $this->redirectWithErrors($form);
}

I will also add this issue to the docs for other users. Thanks for reporting and for help :)

[marcoraddatz]

Yes, I'll do so for now. Also I thought about adding Request to form options as optional parameter, so that you don't have to chain, but since it's mostly on store/update methods, it might only make sense with unique keys, etc.

Anyway! Thanks for you great work! It really helps a lot!

[kristijanhusak]

@marcoraddatz Just want to let you know that this is fixed on dev-master branch for now. No need to set request manually.

[marcoraddatz]

Thanks Kristijan! The correct request is now used.

Btw. if you add a field and remove it later on by a condition, an exception is thrown (removed some code for easier understanding):

$this
    ->add('country_id', 'select', [
            'required' => true,
            'label'    => trans('label.country'),
            'choices'  => []
        ]
    )
    ->add(trans('label.submit'), 'submit');

$this->remove('country_id');

→

Caused by
exception 'InvalidArgumentException' with message 'Field [country_id] does not exist in App\Forms\AddressForm' in /Users/marcoraddatz/Sites/saas/vendor/kris/laravel-form-builder/src/Kris/LaravelFormBuilder/Form.php:1060
Stack trace:
#0 /Users/marcoraddatz/Sites/saas/vendor/kris/laravel-form-builder/src/Kris/LaravelFormBuilder/Form.php(305): Kris\LaravelFormBuilder\Form->fieldDoesNotExist('country_id')
#1 /Users/marcoraddatz/Sites/saas/app/Forms/AddressForm.php(90): Kris\LaravelFormBuilder\Form->remove('country_id')
#2 /Users/marcoraddatz/Sites/saas/vendor/kris/laravel-form-builder/src/Kris/LaravelFormBuilder/Form.php(131): App\Forms\AddressForm->buildForm()
#3 /Users/marcoraddatz/Sites/saas/vendor/kris/laravel-form-builder/src/Kris/LaravelFormBuilder/Form.php(576): Kris\LaravelFormBuilder\Form->rebuildForm()
#4 /Users/marcoraddatz/Sites/saas/vendor/kris/laravel-form-builder/src/Kris/LaravelFormBuilder/Form.php(478): Kris\LaravelFormBuilder\Form->setModel(Object(App\Address))
#5 /Users/marcoraddatz/Sites/saas/vendor/kris/laravel-form-builder/src/Kris/LaravelFormBuilder/Form.php(460): Kris\LaravelFormBuilder\Form->pullFromOptions('model', 'setModel')
#6 /Users/marcoraddatz/Sites/saas/vendor/kris/laravel-form-builder/src/Kris/LaravelFormBuilder/FormBuilder.php(50): Kris\LaravelFormBuilder\Form->setFormOptions(Array)
#7 /Users/marcoraddatz/Sites/saas/app/Http/Controllers/AddressController.php(63): Kris\LaravelFormBuilder\FormBuilder->create('App\\Forms\\Addre...', Array)
#8 [internal function]: App\Http\Controllers\AddressController->edit('1', Object(Kris\LaravelFormBuilder\FormBuilder))

Prior to dev-master it worked. Instead of adding all fields and removing some of them later on, I now check whether I should add them or not.

[kristijanhusak]

@marcoraddatz do you know what version u were using before updating to dev-master?

I'll probably remove that exception and just ignore remove if the field does not exist.

[marcoraddatz]

I like the idea and currently use 1.6.31.