[5.8] Fix many to many sync results with custom pivot model

[themsaid]

This PR fixes an issue (#28150) where the results of sync() while updating "Custom Pivot Model" attributes always show the record as updated even if it's not.

The reason was that we weren't loading the actual pivot model from the database and thus we didn't have any way to compare the newly provided data with the existing one.

My solution here is that we load the model from the database first, and use isDirty() to check if the attributes were update or not.

[pdewit]

Hey @themsaid, I think this PR caused a breaking change for me when trying to use updateExistingPivot when there were no records found:

Symfony\Component\Debug\Exception\FatalThrowableError: Call to a member function fill() on null
  File "/var/www/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php", line 227

My code snippet:
$ticket->subscribers()->updateExistingPivot(Auth::id(), ['last_read_at' => Carbon::now()]);

Did I use it wrong or is it broken when no records are found?

[themsaid]

@pdewit if there are no records found then it's not an existing pivot, not sure why you're using updateExistingPivot then.

[lk77]

Hello @themsaid,
since update to 5.8, updated_at field is not updating anymore on pivot when using sync,
i think it may be related to this PR, but i don't know how.

[mpyw]

At least this PR contains breaking change.

Previous:

updateExistingPivot() simply runs 1 UPDATE query. Anytime we can use this method without confirming pivot existence.

Now:

updateExistingPivot() runs 1 SELECT query + 1 UPDATE query. We must confirm its existence before using this method, otherwise it causes PHP error.

if there are no records found then it's not an existing pivot, not sure why you're using updateExistingPivot then.

@themsaid If so, when do you think you should use this method? It looks very difficult to use. Should we always use sync() instead?

@themsaid @pdewit @lk77 @taylorotwell Any ideas?

[lk77]

In my case it's not this PR but this one : #27571
i made an issue #29321
And for your issue i don't see the problem, if select fails it cause php error ?

[mpyw]

How about this?

    /**
     * Update an existing pivot record on the table via a custom class.
     *
     * @param  mixed  $id
     * @param  array  $attributes
     * @param  bool   $touch
     * @return int
     */
    protected function updateExistingPivotUsingCustomClass($id, array $attributes, $touch)
    {
-       $updated = $this->getCurrentlyAttachedPivots()
-                   ->where($this->foreignPivotKey, $this->parent->{$this->parentKey})
-                   ->where($this->relatedPivotKey, $this->parseId($id))
-                   ->first()
-                   ->fill($attributes)
-                   ->isDirty();
-
+       $pivot = $this->getCurrentlyAttachedPivots()
+                   ->where($this->foreignPivotKey, $this->parent->{$this->parentKey})
+                   ->where($this->relatedPivotKey, $this->parseId($id))
+                   ->first();
+
+       $updated = $pivot ? $pivot->fill($attributes)->isDitry() : false;

        $this->newPivot([
            $this->foreignPivotKey => $this->parent->{$this->parentKey},
            $this->relatedPivotKey => $this->parseId($id),
        ], true)->fill($attributes)->save();

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

        return (int) $updated;
    }

[mpyw]

@lk77 @themsaid @pdewit Fixed. Thanks!