laravel / ideas

Issues board used for Laravel internals discussions.
939 stars 28 forks source link

[7.x] belongsToMany SoftDeletes #1908

Closed skaiblinger closed 4 years ago

skaiblinger commented 5 years ago

Hey guys!

I'm porting a pretty big legacy application (150+ tables) to laravel, but hit a pretty big roadblock regarding belongsToMany-relationships: SoftDeletes. Since the whole legacy application heavily depends on soft deleting, not having soft-deletable belongsToMany-relationships is a real pain the the rear.

Instead of a simple $group->members()->attach($member); i would have to load the pivot-model, restore it if it exists but is trashed, or create a new one if it doesn't exists yet.

And even simply loading all members like $members = Group::findOrFail(1)->members; will get pretty ugly and complex when dealing with many pivot-tables or nested belongsToMany-relationships.

Since there are a few issues regarding soft-deletable relationships (for example https://github.com/laravel/framework/issues/2733, https://github.com/laravel/framework/issues/28407, https://github.com/laravel/ideas/issues/725) i figuered i could work on a PR for that problem.

I already have a working prototype, which would implement these features. But since i am a experienced programmer but bloody beginner using laravel, i am sure there are many more things to test and fix behind the scenes.

The only questions are:

Is there a need for a solution or am i missing a already implemented solution?

Is it a viable feature for the framework and is there a chance it will get accepted?

Reading the issues and SO-posts regarding soft-deletes in laravel, it seems there is a bit of a "soft deletes is an anti pattern" thing going on. So if the maintainers would not like to see this fixed in the future because of other intentions for the framework, it's ok. But the i would have to take an other route of implementing it. Maybe as an extension with a "belongsToManySoftDeletable"-relation

So i would really like to hear feedback if there is a need and if working on a PR is worth it.

jonaslm commented 4 years ago

I'm interested to see how you solved this problem. We had a similar problem and solved it with a macro on Relation, something like:

Relation::macro('withSoftDeletablePivot', function ($deletedAtColumn = 'deleted_at') {
    $this->query->withGlobalScope('softDeletePivot', function ($builder) use ($deletedAtColumn) {
        $builder->whereNull("{$this->table}.$deletedAtColumn");
    });

    $this->query->macro('withTrashedPivots', static function ($builder) {
        return $builder->withoutGlobalScope('softDeletePivot');
    });

    return $this;
});

It's a bit less smooth than the regular SoftDeletes-trait since you need to enable it on the relation level rather than on the model level, e.g.

public function users() {
    return $this->belongsToMany(User::class, 'entity_user')
        ->withSoftDeletablePivot();
}

It then works similarly to the regular SoftDeletes trait in that it will by default exclude models where the pivots deleted_at is null, and when fetching applying withTrashedPivots() it will include those with deleted pivots as well, e.g. $entity->users()->withTrashedPivots()->get()

We would also welcome similar functionality being included in Laravel by default.

Sladewill commented 4 years ago

You have also got the custom pivot model with the term ->using(ClassName::class) you can then apply soft deletes into that, that part of it automates the soft deletes on the pivot table.

skaiblinger commented 4 years ago

The current idea is to really make use of a supplied pivot model when "using" is called.

With that model i am joining the pivot-table using the Eloquent query builder and not just via a simple "low-level" join. Using that query-builder i am able to apply the global scopes for the pivot model.

Then, extending the __call() of the relation, i get the ability to call all methods on the pivot-query. For example: $group->members()->pivotWithTrashed()

For now fetching the results works, but sync() and syncWithOutDetatching() are far more complex, since i have to avoid the n+1 query problem and have to work with all the scopes.

Hopefully i will have a full solution ready after the holidays.

skaiblinger commented 4 years ago

@Sladewill Sadly this never worked but was not documented until 5.7 i think. See https://github.com/laravel/framework/issues/14008

The query will not check if the entry in the pivot-table is soft deleted.

Sladewill commented 4 years ago

Thats without the use of the using statement and a custom pivot model. I am currently using the custom pivot model with soft deletes on a 5.8.x project and it's working as intended. The only downside is when you resync it creates a problem so you have to force deleted_at to null as part of the sync process.

jonaslm commented 4 years ago

You have also got the custom pivot model with the term ->using(ClassName::class) you can then apply soft deletes into that, that part of it automates the soft deletes on the pivot table.

This doesn't work for me at least (tried on Laravel 5.8.24). Have you modified your pivot class in some other way to get this to work?

The example I tried:

Entity-model

public function users() {
    return $this->belongsToMany(User::class, 'entity_user')
        ->using(EntityUser::class);
}

EntityUser-model

class EntityUser extends Pivot {
    use SoftDeletes;
    [...]
}

When fetching $entity->users, it never adds a deleted_at check for the entity_user table. The query it generates is:

SELECT `user`.*, 
       `entity_user`.`entity_id` AS `pivot_entity_id`, 
       `entity_user`.`user_id`   AS `pivot_user_id` 
FROM   `user` 
       INNER JOIN `entity_user` 
               ON `user`.`id` = `entity_user`.`user_id` 
WHERE  `entity_user`.`entity_id` = ? 

Furthermore, if you were to add $entity->users()->withTrashed()->get(), how would it differentiate trashed pivots from trashed models (assuming both User and EntityUser are soft-deletable)?

jonaslm commented 4 years ago

The current idea is to really make use of a supplied pivot model when "using" is called.

I like the idea of leveraging ->using() for this. Looking forward to seeing your solution.

Sladewill commented 4 years ago
<?php namespace App\Jobs\Dates;

use Illuminate\Database\Eloquent\Relations\Pivot;
use Illuminate\Database\Eloquent\SoftDeletes;

class DateUserPivot extends Pivot
{
    use SoftDeletes;
}
/**
 * The users.
 *
 * @return \Illuminate\Database\Eloquent\Relations\BelongsToMany
 */
public function users()
{
    return $this->belongsToMany('App\Users\User', 'job_dates_users', 'job_date_id', 'user_id')
        ->using('App\Jobs\Dates\DateUserPivot')
        ->whereNull('job_dates_users.deleted_at')
        ->withTimestamps();
}
jonaslm commented 4 years ago

I see what you mean. We had something like that before (the custom pivot model isn't necessary either - it's just the explicit whereNull that removes the deleted pivots), but it's annoying since you have to define a separate relation (e.g. usersWithTrashedPivots()) that doesn't apply the whereNull('deleted_at') if you want to be able to retrieve those as well.

skaiblinger commented 4 years ago

@jonaslm Thank you for the full explanation of the problem. This is the exact behavior i'm trying to fix (including writes)

Defining multiple relationships for the different states (withTrashed, withoutTrashed, onlyTrashed) is not really a feasible workaround for me.

skaiblinger commented 4 years ago

I made it to the point where i were confident enough to create the PR.

It may not look like many things changed, but finding all the things which needed to change, and then finding all the points where i could apply the scopes and create the table-joins was a bit of a nightmare.

Maybe there are some testers out there which will give these changes a ride.

driesvints commented 4 years ago

Since Taylor closed the pr it's unlikely that we'll implement this anytime soon, sorry.