Skip to content

[5.8] Make updateExistingPivot() safe on non-existent pivot#29362

Merged
taylorotwell merged 1 commit intolaravel:5.8from
mpyw:fix-update-existing-pivot-error-on-non-existent-record
Aug 1, 2019
Merged

[5.8] Make updateExistingPivot() safe on non-existent pivot#29362
taylorotwell merged 1 commit intolaravel:5.8from
mpyw:fix-update-existing-pivot-error-on-non-existent-record

Conversation

@mpyw
Copy link
Contributor

@mpyw mpyw commented Aug 1, 2019

Fixes issue from [5.8] Fix many to many sync results with custom pivot model by themsaid · Pull Request #28416 · laravel/framework.

Currently, when we use updateExistingPivot() with a custom pivot class, the following code will be executed.

$updated = $this->getCurrentlyAttachedPivots()
    ->where($this->foreignPivotKey, $this->parent->{$this->parentKey})
    ->where($this->relatedPivotKey, $this->parseId($id))
    ->first()
    ->fill($attributes)
    ->isDirty();

However, it is unsafe; If we call updateExistingPivot() for non-existent record, it will unexpectedly cause FatalThrowableError.

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 xxx

This PR resolves this issue by fixing like this:

$pivot = $this->getCurrentlyAttachedPivots()
    ->where($this->foreignPivotKey, $this->parent->{$this->parentKey})
    ->where($this->relatedPivotKey, $this->parseId($id))
    ->first();

$updated = $pivot ? $pivot->fill($attributes)->isDitry() : false;

@mpyw mpyw marked this pull request as ready for review August 1, 2019 10:31
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