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

[11.x] Add support for syncing associations with array or base collection of models #53495

Conversation

diaafares
Copy link
Contributor

@diaafares diaafares commented Nov 13, 2024

During a code review for a teammate’s automated tests, I proposed the following change:

$tag = Tag::create([‘name’ => Str::random()]);
$tag2 = Tag::create([‘name’ => Str::random()]);

- $post->tags()->sync([$tag->id, $tag2->id]);
+ $post->tags()->sync([$tag, $tag2]);

However, he encountered an error when attempting to implement this change, despite I was very sure that I used sync with models before.

It appears that the current implementation of parseIds doesn't support passing models unless they are passed inside an Eloquent collection, so, to make it consistent, I updated the implementation to work with an array or base collection of models.

This will also be beneficial for situations when you already have an array of models:

- $post->tags()->sync(Arr::pluck($tags, 'id'));
+ $post->tags()->sync($tags);

Furthermore, it seems that there are currently no tests that cover the scenario of passing an Eloquent collection to the sync method, so I added a check for it inside my newly added test.

@taylorotwell taylorotwell merged commit fac702d into laravel:11.x Nov 14, 2024
33 checks passed
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

Successfully merging this pull request may close these issues.

2 participants