protonemedia / laravel-form-components

A set of Blade components to rapidly build forms with Tailwind CSS (v1.0 and v2.0) and Bootstrap 4/5. Supports validation, model binding, default values, translations, Laravel Livewire, includes default vendor styling and fully customizable!
https://protone.media/blog/laravel-form-components-to-rapidly-build-forms-with-tailwind-css-and-bootstrap-4
MIT License
814 stars 105 forks source link

select options with integers as keys breaks the form on user errors #17

Closed anhofmann closed 4 years ago

anhofmann commented 4 years ago

For this issue I copied the example from the readme:

<x-form-select name="country_code">
   <option value="be">Belgium</option>
   <option value="nl">The Netherlands</option>
</x-form-select>

suppose that we generated the above options with a $options array. This will work:

$options = ['be' => 'Belgium', 'nl' => 'The Netherlands'];
<x-form-select name="country_code" :options="$options">

But the following will not work correctly:

$options = ['Belgium','The Netherlands'];
<x-form-select name="country_code" :options="$options">

The above will generate this HTML:

<select name="country_code">
                            <option value="0">
                    Belgium
                </option>
                            <option value="1">
                    The Netherlands
                </option>
                    </select>

This will not correctly bind the old values of the form when the user made an error. The form will be reloaded with the error messages in it but without the selected item from the select list. The reason for this lies in the file src/Components/FormSelect.php on line 51: if ($this->selectedKey === $key) { this will return false every time because $this->selectedKey is a string and $key is an integer. Is it possible to change the === to ==? The example with the country_code does not make much sense at first. But if I want to generate the form options from an eloquent relation (and other data than country codes), I have exactly this problem. The ID of the foreign key is an int and the above mentioned if will never return true.

anhofmann commented 4 years ago

An alternative would be to change resources/views/tailwind/form-select.blade.php Line 20 to this:

<option value="{{ $key }}" @if($isSelected((string)$key)) selected="selected" @endif>

just cast $key to a string.

xcopy commented 4 years ago

Yeah, same problem for me. That code uses Identical comparison (===) instead of Equal (==) which always returns false. So, should be something like type checking feature.

Well, temp solution to me: just override isSelected method in my component.

<?php

namespace App\View\Components;

use ProtoneMedia\LaravelFormComponents\Components\FormSelect as BaseFormSelect;

class FormSelect extends BaseFormSelect
{
    public function isSelected($key): bool
    {
        if ($this->isWired()) {
            return false;
        }

        // result of comparing string and number will always be false
        // and we'll overwrite only line 50 of the source code
        // @see vendor/protonemedia/laravel-form-components/src/Components/FormSelect.php#L50
        if ($this->selectedKey == $key) {
            return true;
        }

        if (is_array($this->selectedKey) && in_array($key, $this->selectedKey)) {
            return true;
        }

        return false;
    }
}

And don't forget to update component configuration

'form-select' => [
   'view'  => 'form-components::{framework}.form-select',
   'class' => \App\View\Components\FormSelect::class
]
anhofmann commented 4 years ago

Well, temp solution to me: just override isSelected method in my component.

sort of temp solution for me was to publish the blade templates and casting the @key to a string. I figured that data from html forms are always strings so this should be safe to do.

xcopy commented 4 years ago

@anhofmann yeah, I guess that's pretty solid temp solution too.

As we talking about the state after validation, I guess values of the posted/validated data returned by old() (aka session flash data) are always strings.

Am I missing something else?

pascalbaljet commented 4 years ago

Thanks @anhofmann and @xcopy. Will look into it today.

pascalbaljet commented 4 years ago

I've created a branch with a bugfix. Could you try and test it?

composer require protonemedia/laravel-form-components:dev-select-bugfix-17

pascalbaljet commented 4 years ago

Fixed in v2.1.1

xcopy commented 4 years ago

@pascalbaljet great, thanks!