Framework: [5.2] Sync and Attach problem

[jmarcher]

Today I encountered my self into a problem, I believe this is some kind of problem.

In my case:
I have a categories and sizes that can belong to many categories, therefore I use a pivot table
category_size: category_id | size_id

The problem: I attached (with $category->sizes()->attach($size_id)) many sizes, many of them may be duplicated for example:

category_id | size_id
1           |  1
1           |  1
1           |  2
2           |  1

The problem happens when I try to sync, for example if I do $category->sizes()->sync([1,2]);
the table remains the same, I mean from the documentation how sync must works is:

Any IDs that are not in the given array will be removed from the intermediate table. So, after this operation is complete, only the IDs in the array will exist in the intermediate table

I don't know if this is a misunderstanding from me or a real problem.

[themsaid]

sync() won't remove duplicates, category one is currently attached to sizes [1,1,2], when you sync([1,2]) the final output will still be [1,1,2].

I suggest you do sync([]) before doing sync([1,2]) to remove all records before adding new ones.

[jmarcher]

But exactly that is what I mean, shouldn't be this the normal behavior?
What I understand by the documentation, only if I specify sync([1,1,2]) should there be duplicates.

[themsaid]

Not sure if this is a bug or a feature honstly, I think some people might need sync to work the way it does now.

Ping @taylorotwell

[rkgrep]

@jmarcher Pivot table may contain additional fields so duplicates are OK.
I would recommend to use 2-column unique key in your migration and properly catch QueryException to avoid duplicates.

[themsaid]

What @rkgrep suggested makes good sense. I suggest you do the same.

Here's an example on how to create a 2-column primary key:

$table->primary(['category_id', 'size_id']);

That way duplicates won't be allowed by the database engine.

[jmarcher]

@rkgrep @themsaid Thanks, in this case is not a problem for me.

I know that the pivot table may have duplicates, my concern is the sync functionality as the documentation says: At the end of the sync() method, the pivot table should only have the ids I passed in the array.

If this is not the intent of sync it may be nice to have a method that does a "real" sync just as @themsaid suggested first calling sync([])

[jmarcher]

Ping
Any news if this is a feature or a bug?

In both cases I can make a PR solving it or making a new method to remove duplicates

[themsaid]

I think it's better that you make a PR that adjusts sync() to do a clean sync, in my opinion sync should remove duplicates.

[themsaid]

@jmarcher did you get the chance to work on a PR for this?

[jmarcher]

Not yet, moved back to Germany and didn't had enough time.