search

kristijanhusak / laravel-form-builder

Laravel Form builder for version 5+!
https://packagist.org/packages/kris/laravel-form-builder
MIT License
1.7k stars 298 forks source link

Form collection only validating first item #464

Open Vusys opened 5 years ago

Vusys commented 5 years ago

Seems like the same issue as #268

Routes

//web.php
Route::any('/', function (FormBuilder $formBuilder) {

    $form = $formBuilder->create(\App\Http\Forms\PostForm::class, ['method' => 'POST']);

    if (!$form->isValid()) {
        return redirect()->back()->withErrors($form->getErrors())->withInput();
    }

    return view('welcome', compact('form'));
});

View

<!doctype html>
<html lang="{{ app()->getLocale() }}">
<head>
    <meta charset="utf-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Laravel</title>
    <link href="https://stackpath.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css" rel="stylesheet" integrity="sha384-BVYiiSIFeK1dGmJRAkycuHAHRg32OmUcww7on3RYdg4Va+PmSTsz/K68vbdEjh4u" crossorigin="anonymous">
    <script src="https://code.jquery.com/jquery-2.1.3.min.js"></script>
    <script src="https://stackpath.bootstrapcdn.com/bootstrap/3.3.7/js/bootstrap.min.js" integrity="sha384-Tc5IQib027qvyjSMfHjOMaLkfuWVxZxUPnCJA7l2mCWNIpG9mGCD8wGNIcPD7Txa" crossorigin="anonymous"></script>
</head>
<body>
<div class="container">
    <div class="row">
        <div class="col-md-12">

            <?php dump($errors->toArray()) ?>

            {!! form_start($form) !!}
            {!! form_row($form->title) !!}
            {!! form_row($form->body) !!}
            <hr>
            <div class="collection-container" data-prototype="{{ form_row($form->tags->prototype()) }}">
                {!! form_row($form->tags) !!}
            </div>
            {!! form_end($form) !!}

            <button type="button" class="add-to-collection">Add to collection</button>
            <script>
                $(document).ready(function () {
                    $('.add-to-collection').on('click', function (e) {
                        e.preventDefault();
                        var container = $('.collection-container');
                        var count = container.children().length;
                        var proto = container.data('prototype').replace(/__NAME__/g, count);
                        container.append(proto);
                    });
                });
            </script>
        </div>
    </div>
</div>
</body>
</html>

Forms

namespace App\Http\Forms;
use Kris\LaravelFormBuilder\Form;

class PostForm extends Form
{
    public function buildForm()
    {
        $this
            ->add('title', 'text')
            ->add('body', 'textarea')
            ->add('tags', 'collection', [
                'type'    => 'form',
                'options' => [    // these are options for a single type
                    'class' => TagsForm::class,
                    'label' => false,
                ],
            ])
            ->add('submit', 'submit');
    }
}
namespace App\Http\Forms;
use Kris\LaravelFormBuilder\Form;

class TagsForm extends Form
{
    public function buildForm()
    {
        $this
            ->add('name', 'text', ['rules' => 'url'])
            ->add('desc', 'text');
    }
}

Form with errors:

screen shot 2018-11-30 at 09 35 18

I have been trying to diagnose the issue, and it appears to be something to do with collectionType::createChildren. When validating a request, the method does not build enough children - it only ever builds one which is why only the first child form gets validated.

Replacing

//collectionType::createChildren

// If no value is provided, get values from current request.
if (!is_null($data) && count($data) === 0) {
    $data = $currentInput;
}

// Needs to have more than 1 item because 1 is rendered by default.
// This overrides current request in situations when validation fails.
if ($oldInput && count($oldInput) > 1) {
    $data = $oldInput;
}

with

$data = $oldInput ?? $currentInput ?? $data;

Seems to resolve the issue, but I don't really understand what the old code was trying to accomplish so I may have introduced more problems here.

kristijanhusak commented 5 years ago

I'll check. Looks like your code is correct, but i can't really remember why i did it othe way around.

TsePakKwan commented 5 years ago

is this issue already fixed? I have same problem.

Vusys commented 5 years ago

@TsePakKwan The fix I proposed has not been merged or even made a PR, so no.

As a work around, I installed cweagans/composer-patches and patched it in the vendor directory. Just install that package, then add to your composer.json:

"extra": {
       "patches": {
            "kris/laravel-form-builder": {
                "Collection Validation": "patches/form-builder-collection-validation.diff"
            }
        }
    }

and create patches/form-builder-collection-validation.diff:

diff --git a/src/Kris/LaravelFormBuilder/Fields/CollectionType.php b/src/Kris/LaravelFormBuilder/Fields/CollectionType.php
index df47e8c..d15291b 100644
--- a/src/Kris/LaravelFormBuilder/Fields/CollectionType.php
+++ b/src/Kris/LaravelFormBuilder/Fields/CollectionType.php
@@ -90,16 +90,18 @@ class CollectionType extends ParentType

         $data = $this->getOption($this->valueProperty, []);

-        // If no value is provided, get values from current request.
-        if (!is_null($data) && count($data) === 0) {
-            $data = $currentInput;
-        }
-
-        // Needs to have more than 1 item because 1 is rendered by default.
-        // This overrides current request in situations when validation fails.
-        if ($oldInput && count($oldInput) > 1) {
-            $data = $oldInput;
-        }
+        // // If no value is provided, get values from current request.
+        // if (!is_null($data) && count($data) === 0) {
+        //     $data = $currentInput;
+        // }
+
+        // // Needs to have more than 1 item because 1 is rendered by default.
+        // // This overrides current request in situations when validation fails.
+        // if ($oldInput && count($oldInput) > 1) {
+        //     $data = $oldInput;
+        // }
+
+        $data = $oldInput ?? $currentInput ?? $data;

         if ($data instanceof Collection) {
             $data = $data->all();

I've since left the business where this was a problem, otherwise I'd probably have more insight into the effects of my patch and would have pull requested it if it solved the bug with no unintended side effects.

rudiedirkx commented 5 years ago

There are several good ways to 'fix' this.

The first way is always good. There is no reason the POST form would be different from the GET form. Make sure it's built exactly the same.

TsePakKwan commented 5 years ago

There are several good ways to 'fix' this.

  • Make sure the POST form is built exactly like the GET form. If the GET form has multiple collection items, the POST form should have too, and they will all be validated and returned from getFieldValues().
  • If the POST data is dynamic (probably because of prototype + JS), set the prefer_input=true option on the collection, and the POST data will be used as a base, not the form's collection items.

The first way is always good. There is no reason the POST form would be different from the GET form. Make sure it's built exactly the same.

My POST data is dynamic (probably because of prototype + JS),But I try to set prefer_input=true still only validating first item.Is any idea to fix?

rudiedirkx commented 5 years ago

You'll have to the debugging yourself, but you should start with $form->getAllAttributes() in GET and POST. In its output, you can see which elements will be validated and kept. Collection items depend on field settings and input and how the form is built. That's where the debugging comes is. But let's start with the list. In GET it should be predictable: same as the form output. What is it in POST? If prefer_input is true, it should look like $_POST, with as many collection items.