[12.x] Streamline functionality and performance increase on custom pivot models attach, update, and detach

[abetwothree]

This merge request is a follow up to this rejected simpler pull request #51776. This pull request is targetting 12.x as Taylor mentioned on the other rejected PR that that update wouldn't be a good idea as a patch release.

This larger pull request does 3 things as described below.

• Performance boost when doing pivot record updates and using a custom pivot model class
• Consistent data provided to model events when doing attach, update, and detach
• Consistent use of where constraints defined on relationships whether doing raw queries or using a custom pivot model class.

Pivot record update performance fix when using a custom pivot model class

Currently for every record updated using a custom class, the code is unnecessarily doing a get() to load all pivot records and hydrating them to then only attempt to grab the specific record needed, tossing the rest aside.

This is a performance issue that compounds exponentially as more pivot records exist, regardless of how many need to be updated. For example, if 5 matching records exist and 3 need to be updated, it will load and hydrate those 5 records 3 times unnecessarily loading and hydrating 15 records overall instead of just 3.

In my situation, any user could currently have up to 260 pivot records for a certain relationship. If all 260 records needed to be updated in a single request (extremely rare by the way), it would load and hydrate 67,600 records.

See use of getCurrentlyAttachedPivots below:

This pull request fixes that performance issue to only grab the wanted record so that it only loads and hydrates the equal number of records being updated. See use of getCurrentlyAttachedPivot below:

Same 1-to-1 functionality for detaching

Consistency between data loaded for pivot update and pivot detach

This pull request consistently loads the entire pivot record for update and detach so that the entire record is available for model events. Previously only pivot update loaded the entire record while detach only made the foreign keys available.

Update loads the full record from the DB and passing that in updating and updated events

Detach creates a new pivot record only with foreign keys, leaving out the rest of the data available in deleting or deleted events

For my personal situation, I wanted to audit deleted events but only received the foreign keys instead of the full record so that I could record what the entire pivot record looked like when it was deleted.

This pull request loads the full record from the db in both update and detach making the full record available consistent in all model events.

Consistent use of where clauses when using custom pivot model class or doing raw queries

Lastly, the original implementation of pivot model events did not consider where clause constraints placed on relationships. As described in this previous pull request and also patched in this other commit by Taylor, it led to inconsistent functionality between using custom pivot models and using raw queries.

The fix was to ignore using custom pivot model classes if where clause constraints existed on the relationship.

Those fixes created a situation where you could only use custom pivot models if your relationship did not have any where clauses, which is not documented and also a weird limitation.

This pull request sets up both update and detach to use the newPivotQuery method to find existing pivot records when using a custom pivot model class, which do consider where clauses defined on the model relationship.

This method is used by update and detach to find existing pivot record considering where constraints

The newPivotQuery adds where constraints

The newPivotQuery method is also used on raw queries that do not use custom pivot models to respect those constraints when updating and detaching pivot records. So this change streamlines functionality to be the same whether using a custom pivot model or not.

This now allows the use of custom pivot classes alongside having where constraints on relationships.

Previously, you had to choose between using a custom pivot model class or having where constraints on a relationship definition. And since it wasn't documented, you kind of just had to stumble into this and track down why it's ignoring a defined custom pivot model.

Final thoughts

Overall, this pull request streamlines functionality between the raw queries and using custom pivot models .

Thanks again for your consideration!

[github-actions]

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

[taylorotwell]

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

[abetwothree]

Hi @taylorotwell, thank you for your feedback. I am certain that this pull request corrects bugs and inconsistencies with the how Laravel works.

This PR itself is very small so it wouldn't work as a package as you recommended. Ironically, this PR is updating functionality that was ported from packages to be in the framework itself.

Bugs fixed by this PR

• Fixes loading all pivot records matching the foreign pivot key to then only update just one
• Fixes the current triggering of inconsistent event data for attach, update, and detach
• Fixes the silent inability to trigger pivot model events if the many-to-many relationship has any where clauses

[abetwothree]

@taylorotwell wanted to highlight one of the things I had to do because of the inconsistency with how custom pivot model events work right now.

I had to add this refreshForDeletedAudit method to load all the record pivot data when it is being deleted so that I could create a correct full audit of it. This is called on the deleting event right before the pivot record is actually deleted and the deleted event is called so that that audit receives the full set of data.

use Illuminate\Database\Eloquent\Relations\Pivot;

class AbstractPivotModel extends Pivot
{
    use Auditable;

    public function refreshForDeletedAudit(): void
    {
        $result = $this->getDeleteQuery()->toBase()->first();

        $this->setRawAttributes((array) $result, true);
    }
}

This is because on delete only the foreign keys are available on events, but on attach and update the full list of DB columns are available, which is inconsistent.

This pull request makes the data available consistent for all events which would remove the need for this method to be needed.