Framework: [Request] BelongsToMany's attach() return value should match $query->insert() return value

[nik-418]

Since the BelongsToMany attach() implementation relies exclusively on the Query\Builder's insert(), wouldn't it make more sense to return the result of that insert() rather than void? It's logically the same thing - inserting a row into a table.

Current implementation:

    /**
     * Attach a model to the parent.
     *
     * @param  mixed  $id
     * @param  array  $attributes
     * @param  bool   $touch
     * @return void
     */
    public function attach($id, array $attributes = array(), $touch = true)
    {
        if ($id instanceof Model) $id = $id->getKey();

        $query = $this->newPivotStatement();

        $query->insert($this->createAttachRecords((array) $id, $attributes));

        if ($touch) $this->touchIfTouching();
    }

Suggested implementation:

    /**
     * Attach a model to the parent.
     *
     * @param  mixed  $id
     * @param  array  $attributes
     * @param  bool   $touch
     * @return bool
     */
    public function attach($id, array $attributes = array(), $touch = true)
    {
        if ($id instanceof Model) $id = $id->getKey();

        $query = $this->newPivotStatement();

        $inserted = $query->insert($this->createAttachRecords((array) $id, $attributes));

        if ($touch) $this->touchIfTouching();

        return $inserted;
    }

[bgallagh3r]

+1 for this. Don't understand why Detach returns a result but attach doesn't.

[nik-418]

Nice one, thanks!

[joshbodine21]

Seems like 5.2.14 still returns void for attach

[josegonzalez]

My pr was never merged because of this comment from @taylorotwell:

It's not actually possible for the insert method to return a boolean false. If it fails, it will ALWAYS throw an exception. The booleans are pointless and I should remove them from insert altogether really.

Note: I think that's an incorrect statement from my reading of the docs, but I could be wrong. _shrug_

[joshbodine21]

If not a boolean, I would think it should at least return the result of the insert()

[Karl456]

@taylorotwell how come this was closed and never implemented, just curious as it would be useful if it did return the insert?

[josegonzalez]

@Karl456 as he mentioned here, he didn't think the consistency warranted the merge (even though I'm pretty sure it actually fixes a bug as well). shrug

[ghost]

Were there any developments on this? It makes sense to me that the method should at least return the result, otherwise one would have to query the last inserted row to get the value. Is there actually a reason that justifies not doing that?

[stephan-v]

Why has this been closed without any comment. I am running into the issue as well that I would like the id of the newly inserted pivot row.