Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Many to Many collection displayed as select #98

Closed
koichirose opened this issue May 15, 2015 · 36 comments
Closed

Many to Many collection displayed as select #98

koichirose opened this issue May 15, 2015 · 36 comments

Comments

@koichirose
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

@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
Copy link
Owner

@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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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

@kristijanhusak
Copy link
Owner

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

@koichirose
Copy link
Contributor Author

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
Copy link
Owner

Yes, that's why i suggested to use that handling through model like in comment #98 (comment)

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
Copy link
Contributor Author

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
Copy link
Owner

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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:

  • the date of course is not shown in the input text anymore
  • languages in screening are not selected
  • my other form inputs for collections (ex. movie->countries, which is a belongsToMany too) now show a JSON representation of the country instead of the text only. This is how I add them.
->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 ab83478 for now

@kristijanhusak
Copy link
Owner

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

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

@kristijanhusak
Copy link
Owner

@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
Copy link
Contributor Author

Here it is:
Please note that I haven't looked into it much and I'm using hacky stuff such as:

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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

@koichirose
Copy link
Contributor Author

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
Copy link
Owner

@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
Copy link
Contributor Author

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
Copy link
Owner

Are you doing array_pluck ?

@koichirose
Copy link
Contributor Author

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
Copy link
Owner

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

@koichirose
Copy link
Contributor Author

It does :) thanks

@Asusas
Copy link

Asusas commented May 29, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants