Laravel-Backpack / CRUD

Build custom admin panels. Fast!
https://backpackforlaravel.com
MIT License
3.08k stars 886 forks source link

[Bug] Select/Select2 Multiple cannot update a hasManyThrough relationship. #1545

Closed juniorgarcia closed 3 years ago

juniorgarcia commented 6 years ago

Bug report

What I did:

Tried to use a select2_multiple field type to update a relationship who comes from a hasManyThrough relationship.

What I expected to happen:

The pivot table should be synced with the information sent.

What happened:

I've got the error Method Illuminate\Database\Query\Builder::sync does not exist.

What I've already tried to fix it:

Nothing, since it seems that this field type try to sync a direct relationship, not a query builder type.

Backpack, Laravel, PHP, DB version:

Example of code

// UserCrudController.php

$this->crud->addcolumn([
  'label'     => 'Subsidiary',
  'type'      => 'select2_multiple',
  'name'      => 'subsidiaryCities',
  'entity'    => 'subsidiaryCities',
  'attribute' => 'subsidiaryNameWithCityName',
  'model'     => 'App\SubsidiaryCity',
]);

// App/Models/User.php
public function subsidiaryCities()
{
    return $this->hasManyThrough(SubsidiaryCity::class,
        SubsidiaryCityUser::class, 'subsidiarycity_id', 'id', 'id',
        'user_id');
}

The query works properly, but I can't sync.

welcome[bot] commented 6 years ago

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication mediums:

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

-- Justin Case The Backpack Robot

promatik commented 3 years ago

Hi @juniorgarcia! Can you please give us more information about your models?

I'm asking you this because I don't believe hasManyThrough relations should have a field and be "edited".

I'll give the example on Laravel Docs regarding hasManyThrough relations.

countries
    id - integer
    name - string

users
    id - integer
    country_id - integer
    name - string

posts
    id - integer
    user_id - integer
    title - string

A user belongsTo a country, and a user hasMany posts, so;

To include a field type hasManyThrough the idea would be to have it on a country Model? And remove or add posts? Or have it on a post, and add or remove countries? It would be impossible because a post belongs to a user, a country can't remove the posts of their users 🤷‍♂️

@tabacitu and @pxpm what do you think about this? Am I missing any use case?

juniorgarcia commented 3 years ago

Hello, how are you? Well, unfortunately I'm no longer part of the project which I found this issue, hence, I have no more access to the source code nor remember about the details.

I'm sorry for that.

tabacitu commented 3 years ago

Thanks for the update @juniorgarcia ! It's been more than 2 years so that's understandable 🤣 😞 sorry about that.

@promatik after our meeting yesterday I think I agree with you. I don't see any use case where an admin should be able to create/edit a hasOneThrough/hasManyThrough relationship. Because that'd basically mean editing the intermediary entry. Which is OK when it's just a pivot table, but in this case, it's NOT a just pivot table, it's a real entry with meaning outside the hasOneThrough/hasManyThrough relationship - so editing it might have unintended consequences.

To rephrase, I've used hasOneThrought & hasManyThrough relationships myself, and I find them useful. But I think they're more meant to be used when READING, not CREATING or UPDATING entries... at least that's how I see it.

@pxpm you're the relationship expert here, what do you think? Do you see any use case where we should be supporting hasOneThrought / hasManyThrough? Both from the admin perspective and from the developer perspective?

promatik commented 3 years ago

@pxpm reminding you on this 🙌

tabacitu commented 3 years ago

Bump @pxpm - if you agree with @promatik and I, let's close this issue please.

pxpm commented 3 years ago

I will be the one giving it the last hammer!

I do agree with @promatik .

I was trying (and tried before, this is not the first time I look at this issue), to make sense from what @juniorgarcia tried to do, from what it seems at the time, the user can have many subsidiaries and it seems it would be suitable for a belongsToMany relation, and if you need a pivot model (SubcidiaryCityUser) you should be totally fine with that and not with this relation.

That said, I don't think this relations should be editable, otherwise the changes would be made at intermediary tables.