framework: BelongsToMany#updateOrCreate is checking the wrong table for Model existence

Fixed in 9.x.

@taylorotwell Even so, absence of the pivot would then result in the creation of a related model, which may not include the corresponding DB column for the attribute they checked for, causing just a different kind of PDOException.

I think it could and should be treated as a bug fix for that inconsistency alone. Albeit anecdotal evidence, I haven’t found many threads when researching this issue. Didn’t seem to have ever been mentioned on the Laravel GitHub either.

Alternatively, there could be a bool flag option or something in the method signature, with the default choosing the pivot?

That is kind of unexpected. It’s been this way for at least 4 years and I’m surprised it has not come up before. I do worry about potentially breaking changes if people have depended on this checking the pivot table attributes.

Should any of you require the fixed behavior in your application right now, you could override the Relation type like so:

<?php

namespace App\Models\Relations;


use Log;

class BelongsToMany extends \Illuminate\Database\Eloquent\Relations\BelongsToMany
{
    private static bool $loggedNotice = false;

    /**
     * @param   array  $attributes
     * @param   array  $values
     * @param   array  $joining
     * @param   bool   $touch
     *
     * @return \Illuminate\Database\Eloquent\Builder|\Illuminate\Database\Eloquent\Model|object|null
     */
    public function updateOrCreate(array $attributes, array $values = [], array $joining = [], $touch = true)
    {
        if(!self::$loggedNotice) {
            Log::notice("Laravel's original `BelongsToMany#updateOrCreate()` method was overridden by"
                       . " `App\Models\Relationships\BelongsToMany#updateOrCreate()`");
            self::$loggedNotice = true;
        }

        if (is_null($instance = $this->related->where($attributes)->first())) {
            return $this->create($values, $joining, $touch);
        }

        $instance->fill($values);

        $instance->save(['touch' => false]);

        return $instance;
    }

}

And put this into the affected Model’s class:

class Foo extends Illuminate\Database\Eloquent\Model
{

    // ... 

    /**
     * This method extends the default Laravel BelongsToMany by returning our custom BelongsToMany instance instead.
     *
     * @param   \Illuminate\Database\Eloquent\Builder  $query
     * @param   \Illuminate\Database\Eloquent\Model    $parent
     * @param   string                                 $table
     * @param   string                                 $foreignPivotKey
     * @param   string                                 $relatedPivotKey
     * @param   string                                 $parentKey
     * @param   string                                 $relatedKey
     * @param   null                                   $relationName
     *
     * @return \App\Models\Relations\BelongsToMany|\Illuminate\Database\Eloquent\Relations\BelongsToMany
     */
    protected function newBelongsToMany(Builder $query, EloquentModel $parent, $table, $foreignPivotKey, $relatedPivotKey, $relatedKey, $relationName = null) {
        return new \App\Models\Relations\BelongsToMany($query, $parent, $table, $foreignPivotKey, $relatedPivotKey, $parentKey, $relatedKey);
    }

I haven’t gotten to this yet sorry. Bit swamped atm.

I seem to remember encountering this several years ago (back in 2015, IIRC.) I thought I was just using the API incorrectly, but it still caused me a great deal of confusion. At that point, I believe the documentation reflected the intended functionality of update, not the actual (improper) function.

I think it mainly comes down to confusion between update() and sync(). I’m pretty sure changing my code to use sync solved the problem, although the application is so old that I’d have to do quite a bit of digging to find it. In any case, this should be fixed somehow. However, as @taylorotwell mentioned, that runs the risk of breaking older applications that somehow rely on this incorrect behavior.

As an alternative to modifying this part of the code and potentially breaking applications, it might be helpful to update the documentation for many-to-many relationships to clearly point out this pitfall. A warning of what not to do certainly would have helped me 5 years ago, but it’s never too late to help others.

Looks like it was added in a PR in 2015: https://github.com/laravel/framework/commit/955c9d71ff1da20a43a7cba342a478ff43a847b9