kristijanhusak / laravel-form-builder

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

Issue with collection of forms prototype #190

Closed koichirose closed 8 years ago

koichirose commented 8 years ago

I have what I think is an edge case with the form builder and laravel-translatable. The gist of it is that everything is working correctly, but when I add the prototype() I can't seem to get the child form's 'locale' attribute.

Models Setup: QuestionGroup QuestionGroupTranslation Question QuestionTranslation

QuestionGroup has many Questions QuestionGroup and Question have many translations, respectively, via laravel translatable.

If I instantiate a new Question I automatically add empty translations in the constructor, so the create form could display empty translations (in my case, 'it' and 'en').

$a = new \App\Models\Question();
dd($a->translations); //returns an array with two QuestionTranslation objects, for italian and english, with empty attributes

Forms:

//QuestionGroupForm:
$this
            ->add('translations', 'translation', [
                'type' => 'form',
                'wrapper' => [
                    'class' => 'translations'
                ],
                'label' => false,
                'options' => [
                    'class' =>  'App\Forms\QuestionGroupTranslationForm',
                    'label' => false,
                    'wrapper' => false,
                ],
            ])
            ->add('type', 'text', [
                'label' => trans('question_groups.type'),
                'rules' => 'required',
            ])
            ->add('is_default', 'checkbox', [
                'label' => trans('question_groups.is_default'),
            ])
            ->add('questions', 'collection', [
                'type' => 'form',
                'wrapper' => [
                    'class' => 'questions'
                ],
                'label' => trans('questions.plural'),
                'prototype' => true,
                'prototype_name' => '__NAME__',
                'options' => [
                    'class' =>  'App\Forms\QuestionForm',
                    'label' => false,
                    'wrapper' => false,
                ],
            ])
            ->add('id', 'hidden')
            ->add(trans('generic.save'), 'submit', [
                'attr' => ['class' => 'btn btn-primary pull-right']
            ])
            ;

//QuestionForm:
$this
            ->add('translations', 'translation', [
                'type' => 'form',
                'wrapper' => [
                    'class' => 'translations'
                ],
                'label' => false,
                'options' => [
                    'class' =>  'App\Forms\QuestionTranslationForm',
                    'label' => false,
                    'wrapper' => false,
                ],
            ])
            ->add('is_default', 'checkbox', [
                'label' => trans('questions.is_default'),
            ])
            ->add('id', 'hidden')
            ;

The two translation forms are pretty basic, with just a text input, hidden id and hidden locale.

I created a custom 'translation' field, where I display both italian and english inputs via Bootstrap's tabbed interface: laravel-form-builder-translations To do this, I need to know the 'locale' of each translation form so I can put it inside the tabs. Here's my vendor/laravel-form-builder/translation.php, still a work in progress:

//ugly stuff: get the first child ID attribute, to be used in bootstrap's tab panes
//this is needed so we can have multiple translation forms in the same page (ex. question groups and their translations + questions and their translations)
$first_children_id = $options['children'][0]->getOptions()['attr']['id'];
$tab_pane_prefix = str_slug($first_children_id);
?>
<?php if ($showLabel && $showField) { ?>
    <?php if ($options['wrapper'] !== false) { ?>
    <div <?php echo $options['wrapperAttrs'] ?> >
    <?php } ?>
<?php } ?>

<?php
$active_locale = App::getLocale();
?>

<ul class="nav nav-tabs" role="tablist">
    <?php foreach ((array)$options['children'] as $child) {

        $translation_model = $child->getOptions()['value'];
        //not sure why, it's an array sometimes
        if (is_array($translation_model)) {
            $locale = $translation_model['locale'];
        }
        else {
            $locale = $translation_model->locale;
        }
    ?>
    <li role="presentation" class="<?php echo ($locale == $active_locale) ? 'active' : '';?>"><a href="#<?php echo $tab_pane_prefix;?>_<?php echo $locale;?>" role="tab" data-toggle="tab"><?php echo $locale;?></a></li>
    <?php } ?>
</ul>

<?php if ($showLabel && $options['label'] !== false) { ?>
    <?php echo Form::label($name, $options['label'], $options['label_attr']) ?>
<?php } ?>

<?php if ($showField) { ?>
    <div class="tab-content">
    <?php foreach ((array)$options['children'] as $child) {
        $translation_model = $child->getOptions()['value'];
        if (is_array($translation_model)) {
            $locale = $translation_model['locale'];
        }
        else {
            $locale = $translation_model->locale;
        }
        ?>
        <div role="tabpanel" class="tab-pane<?php echo ($locale == $active_locale) ? ' active' : '';?>" id="<?php echo $tab_pane_prefix;?>_<?php echo $locale;?>">
            <?php echo $child->render() ?>
        </div>
    <?php } ?>
    </div>
    <?php include 'help_block.php' ?>
<?php } ?>

<?php include 'errors.php' ?>

<?php if ($showLabel && $showField) { ?>
    <?php if ($options['wrapper'] !== false) { ?>
    </div>
    <?php } ?>
<?php } ?>

This is working perfectly, as long as I don't use prototypes. I'm using your example:

{!! form_start($form) !!}
        {!! form_until($form, 'is_default') !!}
        <div class="collection-container" data-prototype="{{ form_row($form->questions->prototype()) }}">
            {!! form_row($form->questions) !!}
        </div>
        {!! form_end($form) !!}
        <button type="button" class="add-to-collection">Add to collection</button>
        <script src="https://code.jquery.com/jquery-2.1.3.min.js"></script>
        <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>

What happens is that inside the foreach in translation.php, $child->getOptions()['value'] is null. Do you know how I can fix it? I tried digging around the variables but I couldn't find a reliable way that would work both in edit forms, create forms and their child variants (I have other forms that only include 'translations' using the same view, without any more child forms).

I understand this is pretty complicated and I've thought about a workaround of some sort, but I'd like to know more about laravel form builder and do things nicely...

koichirose commented 8 years ago

Digging deeper while trying to find a workaround, it seems that in the prototype I only have 1 QuestionTranslationForm in $options['children'], if this can help in any way

kristijanhusak commented 8 years ago

@koichirose I'm not sure i understand this 100%. Problem is that $child->getOptions()['value'] is null in prototype() ?

koichirose commented 8 years ago

That should be one of the issues, yes. Also, I may have found a bug with prototypes containing children: if I have a field named questions[0]translations[0], it will be called questions[__NAME__]translations[__NAME__]. __NAME__ will be then replaced by javascript creating wrong input names. I didn't investigate that much though.

I ended up building my own prototype function inside QuestionForm which instantiates a new form and performs replacements manually. This is not best but should be the only case where I need prototypes in my app.

Thanks

kristijanhusak commented 8 years ago

Prototype is just like a template for a single collection entry, so value being null is totally correct. To avoid exceptions, use $child->getValue() instead of $child->getOptions()['value']. About the __NAME__, for each collection field type you can choose what prototype_name will be http://kristijanhusak.github.io/laravel-form-builder/field/collection.html . Hope that helps.

koichirose commented 8 years ago

Got it, so I'd have to put something like __TRANSLATION_NAME__ in my QuestionTranslation form and replace accordingly in js. I'll try and go back to your original implementation and see if I can work with that instead of my own. Thank you

koichirose commented 8 years ago

Sorry if I reopen this, I found a couple issues with the prototype functionality:

Thanks

koichirose commented 8 years ago

Hmm, it looks like the issue is not prototype-related:

Main form

...
->add('embeddables', 'collection', [
                'type' => 'form',
                'wrapper' => false,
                'label' => trans('embeddables.plural'),
                'options' => [
                    'class' => 'App\Forms\EmbeddableForm',
                    'label' => false,
                    'wrapper' => [
                        'class' => 'embeddable clonable'
                    ],
                ],
            ])

Child form (EmbeddableForm)

public function buildForm()
    {
        $this
            ->add('url', 'text', [
                'label' => trans('embeddables.url'),
            ])
            ->add('service', 'choice', [
                'choices' => config('my_settings.embeddable_services'),
                'label' => trans('embeddables.service'),
                'rules' => 'required',
                'expanded' => false,
                'multiple' => false,
            ])
            ->add('id', 'hidden');
    }

With an empty collection of embeddables, one is automatically added with an id=1 (which is the parent model's id). The other fields are empty, correctly.

kristijanhusak commented 8 years ago

Hmm, that's strange. Did you try changing the name from id to something else? Please try something different (id1, service_id, whatever), and let me know if it happens.

koichirose commented 8 years ago

What do you mean? I guess anything else I put will be empty, since there would be no attribute with that name in the model. I tried with 'whatever' and it's empty.

I also tried with another collection on another form (artist->photos instead of event->embeddables) and it happens there, too:

ArtistForm:

            ->add('photos', 'collection', [
                'type' => 'form',
                'options' => [
                    'class' =>  'App\Forms\PhotoForm',
                    'label' => false,
                    'wrapper' => [
                        'class' => 'col-md-4 col-xs-6'
                    ],
                ],
                'wrapper' => [
                    'class' => 'photos row'
                ],
                //'label' => trans('photos.plural'),
                'label' => false,
                'empty_row' => false, //do not show empty photo inputs if there are no photos for this model
            ])

PhotoForm:

class PhotoForm extends Form
{
    public function buildForm()
    {
        $this
            ->add('path', 'photo', [
                'label' => false,
                'wrapper' => false,
            ])
            ->add('id', 'hidden')
            ;
    }
}

If I comment out 'empty_row' (which you added for me a while back, thanks), and the artist doesn't have any photos, I see an empty photo row with the same id as the artist I'm editing.

I'm available to test it out further of course.

koichirose commented 8 years ago

I verified this with different relationship types (one to many, one to many morphable) and the issue is still there. Since it should be a very popular feature and only I seem to have this issue, what do I need to look for to fix it?

kristijanhusak commented 8 years ago

Try to mess around with CollectionType, do some debugging for those id fields. Not really sure where you should start. Any help is appreciated.

koichirose commented 8 years ago

First of all, I'm using version 1.6.31. I believe I found the issue but I'm not sure how to fix it. Briefly: This line sets the field's value to the parent's model value: https://github.com/kristijanhusak/laravel-form-builder/blob/master/src/Kris/LaravelFormBuilder/Fields/FormField.php#L111

I tried adding just one field on my Artist form, as shown on a comment above, dumped $this->parent->getModel() and it's an Artist model, so it gets its id. Actually, all the properties of the Photo form that have the same name as properties on the Artist form will get the Artist's value.

Unfortunately, I'm not sure how to fix this. It should somehow check if the collection is empty (and set empty values for every property of the child form) or unbind the parent's model on empty collections. This only happens for existing models with an empty collection of child forms. Existing models with a non-empty collection of child forms will have an array of values (instead of a model object), correctly set to that collection item's model values. Non-existing models are of course empty, therefore their children forms are empty too.

Looking forward to help you out and fix this.

kristijanhusak commented 8 years ago

I fixed docs now http://kristijanhusak.github.io/laravel-form-builder/field/collection.html#prototype

Please check if you are creating the prototype like it is explained there.

I'll check this out during weekend.

koichirose commented 8 years ago

Thank you for updating the docs, it works.

Let me know if I can help you further with the Child form fields keeping the parent's value if they have the same name.

kristijanhusak commented 8 years ago

@koichirose I pushed the fix to master, can you test it out by pulling dev-master branch? Thanks.

koichirose commented 8 years ago

@kristijanhusak it works on dev-master, both on newly created child forms and on the prototype, thanks!