Allow N-N relationships to save the same pivot more than once

[Kasparsu]

WHY

Seems like an option that should exist. Also referencing https://github.com/Laravel-Backpack/CRUD/issues/4858

BEFORE - What was wrong? What was happening before this PR?

If relationship with subfields was used then it would not allow same pivot key to be saved multiple times with different pivot fields.

AFTER - What is happening after this PR?

Now you can save same pivot key multiple times

HOW

How did you achieve that, in technical terms?

I changed how $relationValues array was formed as well as made it so that every update all relations are removed and remade.

Is it a breaking change?

Might be. All tests do still run so might not be.

How can we test the before & after?

Try saving same key to pivot table multiple times.

[Kasparsu]

Since issue was originally assigned to you @pxpm can you verify and comment if need be.

[Kasparsu]

Hello @Kasparsu

Thank you very much for your contribution 🙏

I've just did a quick review on the changes your proposed, I would go somewhat in the same direction, but I think we need to make this behind a setting, or next month someone will complain that they don't want duplicated pivots and there is no way to enforce that at creation time. Just inverting the problem we have now to the other side may come to bit us later.

I think a reasonable solution is to provide both ways to deal with pivots.

There are also other scenarios that concerns me, for example when people use a PivotModel in their belongsToMany definition, but I would leave that to deal at the last.

A test here is a must, but if you don't have time (or are confused by our confusing test suite 🙃 ) I can write that test for you and you work around it, just make sure this PR is in a branch I can commit too.

Thanks again for pushing this forward, this is a long due request I hope we can nail it down here 🙏

I have tested this with pivot model and seems to work just as well. Putting this behind flag is surely idea but the fact remains that as far as I understand that currently you are able to use generated forms to assign same relation multiple times allready just duplicates will get ignored on submit this is not documented anywhere nor we let user know or prevent user from assigning same relation to pivot table multiple times. You can see it in action in https://demo.backpackforlaravel.com/admin/monster/141/edit#relationship BelongsToMany (n-n) + subfields for pivot table Add first one in select and add a note. Add another one select the same option in select add different note save and result is that only one of those is saved last one.

https://github.com/Laravel-Backpack/CRUD/assets/3220262/184fd2f3-8cae-4fc8-818b-880f03f9d99b

Haven't tested this, but I think uniqueness can be enforced in form validators if somebody really wants it.

[pxpm]

the fact remains that as far as I understand that currently you are able to use generated forms to assign same relation multiple times allready just duplicates will get ignored on submit

That's unintended, and a possible bug. I may have to look at it to fix (not related to this PR). We have some code in place that should prevent the selection of the same pivot multiple times, I will investigate to figure out what's happening.

While at it, and since we are working on this already I will probably make it configurable, but to avoid BC I will need to keep the behavior of choosing the same pivot multiple times on the UI as default. It does not "save" by default multiple pivots, but at this time anyone could have overwritten the store() method in the controller and saved the multiple pivots, so "fixing" the bug now on the UI side could be a major BC.

So I think the real fix for the bug is in this PR where the same pivot can be selected multiple times and saved multiple times.

I will get back to you with news regarding this.

Thanks again 🙏

[Kasparsu]

I will get back to you with news regarding this.

Yeah since I don't have access to PRO fields can't actually take a look at it myself. I have clobbered together my own implementation of relationship and it most definitely does not have any code to avoid this in front end.

[pxpm]

Thanks @Kasparsu once again .

I've spent some time on this issue and I can't see a way this could work like this. I think deleting all the relation instances and recreating everything is out of question here, we can't and shouldn't do that. It's a recipe for disaster IMO, plus what happens when you have a pivot model with events that "do something", it will "do something" everytime you update this entry.

I think there is a way for us to support it, with some developer preparation upfront. What I mean is that, if there is an id or uuid (a unique identifier) in the pivot table, we can use that to match the current sent records vs the stored in the database and kind of "recreate the sync" process without deleting the whole relationship.

So in the $values we should have something like:

[
  0 => [
    "dummyproducts" => "1",
    "notes" => "note 2",
+   "id" => 1
  ]
  1 => [
    "dummyproducts" => "2",
    "notes" => "note ",
+   "id" => 2
  ]
]

In the meanwhile I'v found and submitted the fix for the issue with the relationship pivot select.

I will have a look at this again tomorrow.

Cheers

[Kasparsu]

I was thinking this myself and in most cases if you use PivotModel you would have an id field in that case we could match and sync. But if you don't have pivot model then deleting is a way to go I feel like or another way would be to get existing fields and overwrite them and delete or add when needed. But in that case what could happen is that they delete something and add new thing that would get mapped to old value. In the case that this value is somehow used somewhere else with other fields it does not remove that altho in UI user removed and added a new one there is no way to actually check since we only transmit data.

Might be super weird example but if they set created_at for pivot field without model and give it default of now in that case when we would sync old values it would most likely fill out fine if you edit values or add them or remove but what if you remove and add then the last one doesn't get deleted but updated so no new row no new created_at altho users idea was to remove field and add a new one.

Worse example would be if this was part of multiple relationships and could be edited in different places with different fields. This is all very edge case stuff tho. Not sure if laravel allows this to be all one query to update them all. Will update this in meanwhile.

[Kasparsu]

@pxpm please look over since this code developed into such a cluster fk so quickly. PS: I really want to suck off a shotgun now

[pxpm]

@pxpm please look over since this code developed into such a cluster fk so quickly. PS: I really want to suck off a shotgun now

Hahahaha love it! What happen to my code a lot of the time 😍

No worries this is going in the right direction. I've worked this a bit yesterday so I may have some suggestions how we can improve here.

I will submit a PR with my changes to your "original" PR (before last day changes), and maybe we can merge/cleanup both PR's into one and merge it 🙏

See you in a while! Cheers

[pxpm]

Hey @Kasparsu this was what I came up with: https://github.com/Laravel-Backpack/CRUD/pull/5538

You can pull that branch by changing your composer.json to "backpack/crud":"dev-belongs-to-many-save-duplicate-pivots as 6.7"

I haven't tested the interface yet, the tests only cover under the hood. 👍

[Kasparsu]

Hey @Kasparsu this was what I came up with: #5538

You can pull that branch by changing your composer.json to "backpack/crud":"dev-belongs-to-many-save-duplicate-pivots as 6.7"

I haven't tested the interface yet, the tests only cover under the hood. 👍

I cant even test official interface 😭 so thats on you.

[pxpm]

I think it's safe to close this PR now.

I've updated the #5538 PR with your review.

I think it's pretty much ready? Would you mind giving that PR a go to check if it fixes your issue or you can find something I missed covering ?

Thanks again for your hardwork here, and for pushing this feature/bug fix further. 🙏

Cheers