Synchro's avatar

Many to many attach() inconsistency

Given a conventional pattern like:

$book = new Book();
$author = new Author();
$book->authors()->attach($author->id);

I discovered that you can also pass in an instance instead of just the ID, which is a bit prettier:

$book->authors()->attach($author);

attach() also accepts an array of IDs:

$book=new Book();
$author = new Author();
$author2 = new Author();
$book->authors()->attach([$author->id, $author2->id]);

but you can't pass in an array of instances:

$book->authors()->attach([$author, $author2]);

nor a collection of them:

$book->authors()->attach(collect([$author, $author2]));

even though it looks like it is meant to accept them and does some kind of transform that looks like it should extract IDs.

As a fallback, you can at least do:

$book->authors()->attach(collect([$author, $author2])->pluck('id'));

but it's kind of ugly.

Looking at the code, the first $id parameter is passed through the parseIds() method, which is supposed to extract values if it receives something other than a straight integer ID. Evidently that's not working as I expected. However, I'm not sure if this is a bug or not. It's easily worked around, but just isn't especially intuitive; passing instances is much nicer than passing IDs.

What do you think? Is this a bug? Do you think it's worth working up a PR to allow broader use of instances?

0 likes
8 replies
LaryAI's avatar
LaryAI
AI
3 years ago
Level 58

It does seem like this should be possible, and it would be nice to have the ability to pass in an array of instances. I think it would be worth submitting a pull request to the Laravel repository to allow for this functionality.

The parseIds() method should be updated to accept an array of instances and extract the IDs from them. You could try something like this:

public function parseIds($value)
{
    if (is_array($value)) {
        $value = collect($value);
    }

    if ($value instanceof Arrayable) {
        $value = $value->toArray();
    }

    if (is_array($value)) {
        return array_map(function ($value) {
            if ($value instanceof Model) {
                return $value->getKey();
            }

            return $value;
        }, $value);
    }

    return (array) $value;
}
Synchro's avatar

@LaryAI That looks plausible (ChatGPT's primary job!), and would add what I'm asking, but it would also break the existing functionality in parseIds, where I'm not quite clear on how $this->relatedKey works.

Synchro's avatar

@webrobert But pivot values go in the second param (which I've not mentioned here), not the first?

1 like
Synchro's avatar

@webrobert Fair enough. I can see the additional attributes within the first param being somewhat tricky if models were allowed instead of just IDs. It was just a thought.

1 like
webrobert's avatar

@Synchro I actually like that way. And its readable enough...

$user->projects()->sync(
    collect($projects)
        ->filter->on_project
        ->keyBy('id')
        ->map( fn($project) => [
            'type' => $project['type'] ?? ProjectUserType::JustFollowing
        ])
);

Please or to participate in this conversation.