kristijanhusak / laravel-form-builder

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

Many to Many collection displayed as select #98

Closed koichirose closed 9 years ago

koichirose commented 9 years ago

Not really an issue but I wouldn't know where else to ask.

I have a Movie with a belongsToMany languages association.

Let's say a Movie has 2 languages (english and spanish).

I'm trying to show 2 selects with a list of all languages, where the first one has 'english' as selected and the second one has 'spanish'. option value should be the language id.

This is my attempt, within MovieForm:

$this
->add('languages', 'collection', [
  'type' => 'select',
  'property' => 'name',
  'choices' => $this->model->languages()->lists('id', 'name')
])

This is obviously wrong (shows an empty select), but I'm not sure how to accomplish this, the documentation is not very clear about this. I'd like to avoid using a child LanguageForm as it would just display a select.

kristijanhusak commented 9 years ago

This should do the trick:

$this
->add('languages', 'collection', [
  'type' => 'select',
  'property' => 'name',
  'options' => [
    'choices' => $this->model->languages()->lists('id', 'name')
  ]
])

You have example in first code block here which says 'options' => [ // these are options for a single type.

Probably it's not descriptive enough.

koichirose commented 9 years ago

The 'choices' you wrote doesn't work for me: it only lists already associated languages. I made some progress with:

'choices' => Language::all()->lists('name', 'id') //they have to be inverted: name will be the option text, id will be the option value

But I don't know how to select the currently associated language. I guess I need to use 'selected', but I don't know how to set the language for each particular iteration. Referring to my original example, the first select should list all languages with 'english' selected, the second one with 'spanish'.

EDIT: I managed to do it for Movies like this:

->add('languages', 'collection', [
                                'type' => 'select',
                                'property' => 'id',
                                'options' => [
                                        'choices' => Language::all()->lists('name', 'id'),
                                        'label' => false,
                                ]
                        ])

It automagically selects the correct one.

koichirose commented 9 years ago

Sorry about the multiple comments. I'm trying to do the same thing with Screening. A user can watch a movie multiple times (screenings), in different languages (a screening has many languages like a movie).

//MovieForm
->add('languages', 'choice', [ //I switched to a multiselect instead of multiple separate select
                                'choices' => Language::all()->lists('name', 'id'),
                                'selected' => $this->model->languages()->lists('id'),
                                'multiple' => true,
                        ])
->add('screenings', 'collection', [
                                'type' => 'form',
                                'options' => [
                                        'class' =>  'App\Forms\ScreeningForm',
                                        'label' => false,
                                ]
                        ])

//ScreeningForm
->add('languages', 'choice', [
                                'choices' => Language::all()->lists('name', 'id'),
                                'selected' => $this->model->languages()->lists('id'), //$this->model is NULL here
                                'multiple' => true,
                        ])
->add('Save', 'submit', [
                                'attr' => ['class' => 'btn btn-primary pull-right']
                        ])

Issues here: $this->model is NULL in Screening so I can't set the selected options. It works perfectly in MovieForm but I need to set 'selected' (whereas it worked automatically with separate select elements, see previous comment). The screening child form has a save button, which of course I'd like to remove when I'm displaying the movie form. How do I access a field of a child form from the parent to remove it?

kristijanhusak commented 9 years ago

If your model structure is good, and you eager load all your relationships on your models, you don't need to set up selected at all, it will be done automatically for you. When you pass the model to the form class, in this situation, it needs to have structure something like this:

$model = [
    'languages' => [1, 2, 3].
    'screenings' => [
        ['languages' => [4,5,6]]
    ]
];

Basically get model with something like this:

$model = Movie::with('languages', 'screenings')->find($id);
koichirose commented 9 years ago

@kristijanhusak I already eager load my relationships like this:

$movie = Movie::with(['screenings.languages', 'languages'])->findOrFail($id);

As you can see, I also eager load screening languages.

If I add languages to the form like this:

->add('languages', 'choice', [
                                'choices' => Language::all()->lists('name', 'id'),
                                //'selected' => $this->model->languages()->lists('id'),
                                'multiple' => true,
                        ])

It won't select the already associated languages, even though var_dumping $this->model->languages correctly shows the 2 languages associated with it.

The child form for screenings won't select associated languages either. As said before, $this->model is NULL within ScreeningForm, so I can't explicitly set the 'selected' array element either. Do I need to somehow pass the screening model to the child form? I also still can't remove the 'Save' button from the child form.

kristijanhusak commented 9 years ago

@koichirose You can pass the model to the child form with data option on child form, but as i said, data should be bound automatically, so it shouldn't be necessary to pass it for that reason.

Can you please give me the dump of this call:

   $movie = Movie::with(['screenings.languages', 'languages'])->findOrFail($id);
  dd($movie->toArray());

So I can analyze this and see what needs to be fixed?

koichirose commented 9 years ago

Sure.

Here's the full code:

$movie = Movie::with('screenings.languages', 'languages')->findOrFail($id);
dd($movie->toArray());
$form = $formBuilder->create('Moviedb\Forms\MovieForm', [
  'method' => 'PUT',
  'url' => route('movies.update', $movie),
  'model' => $movie,
]);
return $this->view('movies.edit', compact('form'));

Result of the dd of a movie with 2 languages and 2 screenings with one language each (some elements are removed/collapsed):

array:20 [▼
  "id" => "178"
 "screenings" => array:2 [▼
    0 => array:8 [▼
      "id" => "184"
      "name" => null
      "screening_date" => "2005-09-15 00:00:00"
      "movie_id" => "178"
      "created_at" => "2012-01-12 22:22:45"
      "updated_at" => "2012-01-12 22:22:45"
      "subtitle_language_id" => null
      "languages" => array:1 [▼
        0 => array:8 [▼
          "id" => "1"
          "name" => "Italian"
          "created_at" => "2012-01-12 22:22:43"
          "updated_at" => "2015-03-26 09:26:42"
          "pivot" => array:2 [▶]
        ]
      ]
    ]
    1 => array:8 [▼
      "id" => "390"
      "name" => null
      "screening_date" => "2010-01-04 00:00:00"
      "movie_id" => "178"
      "created_at" => "2012-01-12 22:22:45"
      "updated_at" => "2012-01-12 22:22:45"
      "subtitle_language_id" => null
      "languages" => array:1 [▼
        0 => array:8 [▼
          "id" => "4"
          "name" => "Japanese"
          "created_at" => "2012-01-12 22:22:45"
          "updated_at" => "2015-03-21 16:31:34"
          "pivot" => array:2 [▶]
        ]
      ]
    ]
  ]
  "languages" => array:2 [▼
    0 => array:8 [▼
      "id" => "4"
      "name" => "Japanese"
      "created_at" => "2012-01-12 22:22:45"
      "updated_at" => "2015-03-21 16:31:34"
      "pivot" => array:2 [▶]
    ]
    1 => array:8 [▼
      "id" => "2"
      "name" => "English"
      "created_at" => "2012-01-12 22:22:43"
      "updated_at" => "2015-04-20 11:20:24"
      "pivot" => array:2 [▼
        "movie_id" => "178"
        "language_id" => "2"
      ]
    ]
  ]
]
kristijanhusak commented 9 years ago

Thanks. As i suspected, the problem is in the data structure. Choice field selected option accepts two types of values:

  1. string that represents single selected value, or
  2. array of values that represent multiple selection.

Soe for example, something like this should work:

$this->add('languages', 'choice', [
        'choices' => Languages::all()->lists('name', 'id')
        'selected' => [2,4]
        'multiple' => true
    ]);

It's just how laravels form builder accepts values for selection. If it's string, it tracks that value and selects/checks it. If it's array, it loops over it and selects all values that are found. In your case, languages are returned as object/associative arrays, and then it doesn't know which value to get.

i would suggest to correct your languages method in the model to return something like this:

public function languagesRelationship()
{
    return $this->hasMany('App\Languages');
}

public function languages()
{
    return $this->languagesRelationship()->lists('id');
}

Hope this solves your problem.

It's a bit common issue, check here.

I'll definitely look into this problem to find a better solution for these kind of problems.

koichirose commented 9 years ago

Got it, thank you.

I'd rather set the 'selected' attribute of my forms manually rather than changing my models.

Also, I receive the following error when changing my Movie model as you suggested (leaving the controller as it was before):

FatalErrorException in Builder.php line 424: Call to a member function addEagerConstraints() on a non-object
    in Builder.php line 424 (vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php line 424)

Could it be because movie-languages is a belongsToMany relationship (not hasMany) ? I'm sorry if I'm asking obvious questions, but this is one of my first Laravel apps.

So, I'd like to set the 'selected' attribute and to do that on the child form I'd need to set the 'data' attribute as you suggested, right? Is this still related to the issue we just discussed? Also, what about the removal of the child 'save' button?

Thanks again

kristijanhusak commented 9 years ago

Ok try this in model:

class Languages extends Model {

protected $appends = ['languages'];

public function languagesData()
{
    return $this->hasMany('App\Languages');
}

public function getLanguagesAttribute()
{
    return $this->languagesData()->lists('id');
}
}

And in controller call Movie::with('languagesData')->find($id);

I think it should work, $model->lanaguages should be array of ids.

kristijanhusak commented 9 years ago

And for save button, i suggest not to add it by default, and add it on other page where you are adding languages manually after initialization.

FormBuilder::create('App\Languages')->add('save', 'submit');
koichirose commented 9 years ago

Ok for the save button.

Changing my models like that works within the form, but apparently breaks everything else, since accessing $movie->languages now only returns ids and not the full language object.

I need to investigate further though.

Before you answered, I tried passing the model to the child form (did you manage to find out why it would be NULL in the child form?). Since the screening form is added to movies as a collection, I don't know how to get the first or second (or third..etc.) from within the ScreeningForm.

I'd find it cleaner to manually set the 'selected' attribute instead of adding new attributes to all my Models to have multiple attributes.

kristijanhusak commented 9 years ago

Yes, it's a bit hacky.

I found a problem why model is null in the child form, and i fixed it. Pull the dev version and see if it works for you. If it's ok, i'll tag a release.

koichirose commented 9 years ago

I switched to dev-master and removed all the model edits. I switched back to using the 'selected' array key:

//controller
$movie = Movie::with('screenings.languages', 'languages')->findOrFail($id);
$form = $formBuilder->create('Moviedb\Forms\MovieForm', [
  'method' => 'PUT',
  'url' => route('movies.update', $movie),
  'model' => $movie,
]);

//MovieForm
->add('screenings', 'collection', [
                                'type' => 'form',
                                'options' => [
                                        'class' =>  'Moviedb\Forms\ScreeningForm',
                                        'label' => false,
                                ]
                        ])
->add('languages', 'choice', [
                                'choices' => Language::all()->lists('name', 'id'),
                                'selected' => $this->model->languages()->lists('id'),
                                'multiple' => true,
                        ])

//ScreeningForm
->add('languages', 'choice', [
                                'choices' => Language::all()->lists('name', 'id'),
                                'selected' => $this->model->languages()->lists('id'),
                                'multiple' => true,
                        ])

$this->model is no longer NULL in ScreeningForm, but the values are not selected.

Languages look correct:

//in ScreeningForm
dd($this->model->languages()->lists('id')):
array:2 [▼
  0 => "4"
  1 => "2"
]
kristijanhusak commented 9 years ago

Yes, you are right. I'm working on fix for that.

kristijanhusak commented 9 years ago

Ok, do a composer update and see if it works now.

koichirose commented 9 years ago

Now it selects values of both screenings, but they are not correct. Referring to my dd() from an earlier comment, I have two screenings with one language each, but the two forms both have two values selected, and these are the values of the parent movie (4, 2).

In my previous comment I mentioned that $this->model->languages()->lists('id') in ScreeningForm looked correct but it doesn't, it contains values 4 and 2 which are the languages associated with the parent movie and not the screening. It should be '1' for the first screening and '4' for the second.

I'd need to set 'selected' as the language ids of the current iteration of the screening, I guess.

kristijanhusak commented 9 years ago

Yes, that's why i suggested to use that handling through model like in comment https://github.com/kristijanhusak/laravel-form-builder/issues/98#issuecomment-103119536

That way data will be properly fetched for each screening. You shouldn't have any issue with that, only everywhere where you are syncing languages change it to languagesData, or if you prefer to use languages on other places, use languagesData in the form, and replace method names.

I'll try to figure out some better way that could handle this in the form automatically, or to provide some way to fetch data properly. At this moment, this is the best I can do.

koichirose commented 9 years ago

Thank you, solved it like this, for those having this issue:

//controller
$movie = Movie::with('screenings', 'languages')->findOrFail($id);

//MovieForm
->add('languages_ids', 'choice', [
                                'choices' => Language::all()->lists('name', 'id'),
                                //'selected' => $this->model->languages()->lists('id'), //Not Needed!
                                'multiple' => true,
                        ])

//ScreeningForm same as MovieForm

//Movie Model
        protected $appends = ['languages_ids'];

        public function getLanguagesIdsAttribute()
        {
                return $this->languages()->lists('id');
        }

//Screening Model same as Movie Model
kristijanhusak commented 9 years ago

@koichirose I done some modifications to enable resolving problems like this, and I would really appreciate if you could pull latest dev version and test it. Remove the language_ids field, basically revert everything that you done for this issue, and do this:

$this->add('language', 'choice', [
    'choices' => Language::all()->lists('name', 'id'),
    'multiple' => true,
    'selected' => function($data) {
        // $data is the value passed to the choice field, you can modify it here and return it as you wish 
        // Example of data: 
        // $data = [
        //    [
        //        "id" => "4"
        //        "name" => "Japanese"
        //        "created_at" => "2012-01-12 22:22:45"
        //        "updated_at" => "2015-03-21 16:31:34"
        //        "pivot" => []
        //    ],
        //    [
        //        "id" => "2"
        //        "name" => "English"
        //        "created_at" => "2012-01-12 22:22:43"
        //        "updated_at" => "2015-04-20 11:20:24"
        //        "pivot" => [
        //            "movie_id" => "178"
        //            "language_id" => "2"
        //        ]
        //    ]
        // ];
        // This will return array of id's from data above
          return array_pluck($data, 'id');
    }
]);

It should also work on the collection, where $data is languages for single collection entry.

koichirose commented 9 years ago

@kristijanhusak I get the following error (with a partial call stack):

Object of class Closure could not be converted to string
in FormBuilder.php line 555
at HandleExceptions->handleError('4096', 'Object of class Closure could not be converted to string', 'vendor/illuminate/html/FormBuilder.php', '555', array('value' => '1', 'selected' => object(Closure))) in FormBuilder.php line 555
at FormBuilder->getSelectedValue('1', object(Closure)) in FormBuilder.php line 534
at FormBuilder->option('Italian', '1', object(Closure)) in FormBuilder.php line 501
at FormBuilder->getSelectOption('Italian', '1', object(Closure)) in FormBuilder.php line 420
at FormBuilder->select('screenings[0][languages][]', array('Italian', 'English', 'French', 'Japanese', 'Korean', 'Russian', 'Norwegian', 'Spanish', 'Bosnian', 'Chinese', 'German', 'Polish', 'Czech', 'Thai', 'Greek', 'Turkish', 'Persian', 'Hungarian', 'Swedish', 'Indonesian', 'Hindi', 'Romanian', 'Danish', 'Portuguese'), object(Closure), array('class' => 'form-control', 'multiple' => true)) in Facade.php line 219

I don't know if it was your typo, but it's the same with add('language'..) or add('languages'..)

EDIT: sorry I forgot to pull the latest dev. I get another error now, but it looks like it's on my side (even though it worked before undoing my changes and pulling). Investigating.

koichirose commented 9 years ago

@kristijanhusak Nope, it broke everything here.

I had a custom field named 'dateonly', with a custom field view with this for the Screening date:

<?= Form::input($type, $name, $options['default_value']->toDateString(), $options['attr']) ?>

This returned a new error:

Call to a member function toDateString() on a non-object

removing 'toDateString()' shows the form but:

->add('countries', 'collection', [
    'type' => 'text',
    'property' => 'name',    // Which property to use on the tags model for value, defualts to id
    'data' => [],            // Data is automatically bound from model, here we can override it
    'options' => [    // these are options for a single type
        'label' => false,
       'attr' => [],
    ]
//resulting input text:
[{"id":"4","name":"Japan","iso":null,"description":null,"flag":"Japan.png","created_at":"2012-01-12 22:22:43","updated_at":"2015-04-27 11:10:47","pivot":{"movie_id":"178","country_id":"4"}}]

This is the current status with latest dev. It's the same with both versions of my code (the one with 'languages_ids' additional model attribute workaround, and without it).

Let me know if you need anything else to fix this. I reverted to commit ab8347852ad732a94f4056dff93414d644fd18a5 for now

kristijanhusak commented 9 years ago

@koichirose Ok, thanks. I'll try to fix it asap.

Edit: Can you please give me the code for your custom field?

kristijanhusak commented 9 years ago

@koichirose I fixed some of the issues (I hope). Can you please pull again latest version and see if it works now? I believe it should work for both custom field and collection.

koichirose commented 9 years ago

Here it is: Please note that I haven't looked into it much and I'm using hacky stuff such as: <?php if ($showField): $type = 'date'; ?>

It looks like I don't have a Screening object at all in there with the latest dev, so I don't know if the custom field is relevant.

//ScreeningForm
->add('screening_date', 'dateonly')

//app/Forms/Fields/Dateonly.php
<?php namespace Moviedb\Forms\Fields;

use Kris\LaravelFormBuilder\Fields\FormField;

class Dateonly extends FormField {

    protected function getTemplate()
    {
        // At first it tries to load config variable,
        // and if fails falls back to loading view
        // resources/views/fields/datetime.blade.php
        return 'vendor.laravel-form-builder.dateonly';
    }

    public function render(array $options = [], $showLabel = true, $showField = true, $showError = true)
    {
        return parent::render($options, $showLabel, $showField, $showError);
    }
}

//resources/views/vendor/laravel-form-builder/dateonly.php
<?php if ($showLabel && $showField): ?>
    <?php if ($options['wrapper'] !== false): ?>
    <div <?= $options['wrapperAttrs'] ?> >
    <?php endif; ?>
<?php endif; ?>

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

    <?php if ($showField): $type = 'date'; ?>
        <?= Form::input($type, $name, $options['default_value']->toDateString(), $options['attr']) ?>

        <?php if ($options['help_block']['text']): ?>
            <<?= $options['help_block']['tag'] ?> <?= $options['help_block']['helpBlockAttrs'] ?>>
                <?= $options['help_block']['text'] ?>
            </<?= $options['help_block']['tag'] ?>>
        <?php endif; ?>

    <?php endif; ?>

    <?php if ($showError && isset($errors)): ?>
        <?php foreach ($errors->get($nameKey) as $err): ?>
            <div <?= $options['errorAttrs'] ?>><?= $err ?></div>
        <?php endforeach; ?>
    <?php endif; ?>

<?php if ($showLabel && $showField): ?>
    <?php if ($options['wrapper'] !== false): ?>
    </div>
    <?php endif; ?>
<?php endif; ?>
kristijanhusak commented 9 years ago

Everything looks fine. You don't need render method in your custom form class, so you can remove it, and only leave template name.

Did you update to the latest version (a1c7e8d) ? Is collection working fine now?

Also, are you sure you have screening_date in the model ? If it's null, you cannot call toDateString. You could maybe add a check to see if the value is proper object.

koichirose commented 9 years ago

Just pulled latest dev. Screening date (custom field) is ok now, screening languages are not selected, movies languages are ok without the new model attributes.

Using this for both Movie and Screening:

                        ->add('languages', 'choice', [
                                'choices' => Language::all()->lists('name', 'id'),
                                //'selected' => $this->model->languages()->lists('id'),
                                'multiple' => true,
                                'label' => 'Languages',
                                'selected' => function($data) {
                                        return array_pluck($data, 'id');
                                }
                        ])

$data within the 'selected' Closure is an empty array in Screening. It's correct in Movie.

Fetching the movie like this, even though I noticed it doesn't change much if I eager load languages or not:

$movie = Movie::with('screenings.languages', 'languages')->findOrFail($id);

Thanks again

kristijanhusak commented 9 years ago

@koichirose Please update again to the latest commit (12cbd24) and just test without changing anything, i think it should work. Thanks for helping.

koichirose commented 9 years ago

Confirmed, everything is working as expected :) I can also confirm that you need to eager load relationships.

$data within the Closure in Screening languages is still an empty array, but it's working. It is an Eloquent collection in Movie languages.

Thank you again, closing this for now.

kristijanhusak commented 9 years ago

@koichirose Thanks.

How you know it's empty in the screening languages, you did var_dump? It should not work if that data is empty.

koichirose commented 9 years ago

Yes, I'm var dumping $data. I have a object(Illuminate\Database\Eloquent\Collection) in MovieForm, an empty array in ScreeningForm, but it still selects the correct languages.

kristijanhusak commented 9 years ago

Are you doing array_pluck ?

koichirose commented 9 years ago

Yes, but I'm var dumping before that:

->add('languages', 'choice', [
                                'choices' => Language::all()->lists('name', 'id'),
                                'multiple' => true,
                                'label' => 'Languages',
                                'selected' => function($data) {
                                        //var_dump($data);die();
                                        return array_pluck($data, 'id');
                                }
                        ])
kristijanhusak commented 9 years ago

Try without die(). It will probably print it out several times, but last one should have proper data.

koichirose commented 9 years ago

It does :) thanks

Asusas commented 4 years ago

Hello, For some reason method lists() doesn't work for me. This is the code:

public function buildForm()
    {
        $this
            ->add('markes_id', 'select', [
                'choices' => Automarke::all()->lists('marke', 'id'),
            ]);
    }

But i am getting this error: Method Illuminate\Database\Eloquent\Collection::lists does not exist.

Maybe anyone knows what could be the issue?

Thanks