Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Fix fresh and refresh on pivots and morph pivots #34836

Merged
merged 1 commit into from Oct 14, 2020
Merged

[8.x] Fix fresh and refresh on pivots and morph pivots #34836

merged 1 commit into from Oct 14, 2020

Conversation

axlon
Copy link
Contributor

@axlon axlon commented Oct 14, 2020

Currently the Model::fresh() and Model::refresh() methods cannot be used on pivots, because they reference the model key directly. This PR alters these methods slightly to take advantage of the Model::setKeysForSaveQuery() method which already allows methods such as Model::delete() to work on pivots.

The following is now possible:

$model->relation->pivot->refresh()
$model->relation->pivot->fresh()

@jaulz
Copy link

@jaulz jaulz commented Jun 23, 2021

@axlon I noticed that while using morphOne the following line removes the Pivot relation and does not reload Pivot relations:

$this->load(collect($this->relations)->reject(function ($relation) {
return $relation instanceof Pivot
|| (is_object($relation) && in_array(AsPivot::class, class_uses_recursive($relation), true));
})->keys()->all());

Do you think this PR can also help to reload Pivot relations when calling reload on an instance?

@axlon
Copy link
Contributor Author

@axlon axlon commented Jun 23, 2021

@jaulz From looking at this at first glance I don't think the snippet you shared is related to this PR. This PR and its followup #35193 only deal with calling fresh en refresh from the point of view of the pivot, e.g.:

$party = Party::find(1);
$invitee = $party->invitees[0];
$invite = $invitee->pivot;
$invite->refresh(); // <-- this was fixed by the PRs

It seems to me pivot relations are skipped on purpose, but I don't know why

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.

None yet

3 participants