[5.5] Add an argument to withTrashed

[KKSzymanowski]

This adds an option to pass an argument to withTrashed which decides whether or not to include the trashed records.

Model::withTrashed(true)->get(); // Retrieves trashed records
Model::withTrashed(false)->get(); // Does not retrieve trashed records

I believe this doesn't introduce any breaking changes, hence the 5.5 branch.

Use case:

public function index(Request $request)
{
    return User::withTrashed($request->showDeleted)->get();
}

instead of:

public function index(Request $request)
{
    $query = User::query();

    if ($request->showDeleted) {
        $query->withTrashed();
    }

    return $query->get();
}

[arcanedev-maroc]

There is already a method when() to solve this case:

public function index(Request $request)
{
    return User::query()->when($request->showDeleted, function ($query) {
        return $query->withTrashed();
    })->get();
}

[antonkomarev]

Hmmm... I thought it would be better to have 2 methods: withTrashed() & withoutTrashed() or it's not so obvious. Developer can understand that withTrashed(false) means - don't apply withTrashed.

[KKSzymanowski]

There is a withoutTrashed method(as well as onlyTrashed, which I discovered today).
The point of this PR was to add a possibility to decide whether or not to include deleted records in one line(without an if or any callbacks).

[sisve]

How would this work for everyone that has overwritten the macro and doesn't have the new parameter? Imagine all packages calling Model::withTrashed(false), and only getting trashed items...

[Dylan-DPC-zz]

Why would you use withTrashed(false) ? if you don't want the trashed columns you don't mention it?

[KKSzymanowski]

Dylan, there's a use case in the PR description, and I think it's pretty self-explanatory.

[KKSzymanowski]

@sisve, the custom macro would just overwrite the stock one and there will be no collision, but just to prove it let's walk through a process of creating it.

withTrashed is registered as a local macro in Eloquent Builder, because it's registered via the __call method(not __callStatic). Therefore the only ways I see to overwrite it is to:

1. Create a new global scope and register it after the SoftDeletingScope.
2. Register the macro on the fly on the existing instance of Eloquent Builder.

For both these methods let's assume the stock withTrashed macro has the following definition:

$builder->macro('withTrashed', function (Builder $builder, $argument = true) {
    dump('Original macro', $argument);
});

1. You're gonna have to create a new trait which adds the scope:

<?php

namespace App\Support;

trait CustomSoftDeletes
{
    public static function bootCustomSoftDeletes()
    {
        static::addGlobalScope(new CustomSoftDeletingScope());
    }
}

And along with it, the new scope:

<?php

namespace App\Support;

use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Scope;

class CustomSoftDeletingScope implements Scope
{
    public function apply(Builder $builder, Model $model)
    {
        //
    }

    public function extend(Builder $builder)
    {
        $builder->macro('withTrashed', function (Builder $builder) {
            dump('Overwritten macro');
        });
    }
}

When a model uses both SoftDeletes and our custom trait, and the withTrashed method is called, only Overwritten macro is being dumped, therefore there is no collision.

class Product extends Model
{
    use SoftDeletes, CustomSoftDeletes;

    //
}

Product::withTrashed(); // Overwritten macro

2. Get the fresh query off of the Model, register a macro, call withTrashed and only Overwritten macro is being dumped, so no collision here too.

$query = Product::query();
$query->macro('withTrashed', function(Builder $builder) {
    dump('Overwritten macro');
});
$query->withTrashed();

Does this answer your question?

[Dylan-DPC-zz]

I consider it an anti-pattern to have withTrashed and then say "false" which literally translates to "give me all the records withTrashed without trashed".

[KKSzymanowski]

Also:

Imagine all packages calling Model::withTrashed(false), and only getting trashed items...

I think you misunderstood the argument here.

1. withTrashed() works the same as before.
2. withTrashed(true) works the same as withTrashed().
3. withTrashed(false) essentially pretends like withTrashed has never been called.

[KKSzymanowski]

I consider it an anti-pattern to have withTrashed and then say "false" which literally translates to "give me all the records withTrashed without trashed".

I would see it as

Do you want to fetch the results with trashed records(true/false)? [true]

- I want them with trashed records, so I pass true(or nothing, since it's a default argument).
- Model::withTrashed();

- No, I don't want trashed records, so I pass false.
- Model::withTrashed(false);

[Dylan-DPC-zz]

• No, I don't want trashed records, so I pass false.
• Model::withTrashed(false);

If you don't want trashed records, you don't call it

[KKSzymanowski]

That's why I provided the use case, to show in which situations it's useful.
If you want to keep your controller method as clean as possible, you don't want to split your control flow. This does just that, behind the scenes.

[Miguel-Serejo]

I think this would have been better as a new method withTrashedIf. Self-descriptive, avoids any confusion.

[Dylan-DPC-zz]

I get your use case but then what this is doing is passing the dirt to the framework code. And this practice will spill on to the other functions as well.

[jordyvandomselaar]

I cannot think of any reason why this could ever be a good idea.

[KKSzymanowski]

@36864 I like your proposal and I understand all the criticism.
@taylorotwell Would you rather have withTrashedIf($condition) and withTrashed reverted back to its previous form?

[mnabialek]

@36864 I also think something like withTrashedIf would make more sense. Now it's hard to say without looking at function code what exactly withTrashed(false) would do.

[KKSzymanowski]

I'll spin up a new PR reverting this and adding withTrashedIf.
Thank you all for your insight.

[KKSzymanowski]

I have created a new PR which reverts this and adds withTrashedIf and onlyTrashedIf. Please take a look if this solves your concerns.

[devcircus]

Definitely an antipattern. Very strange merge.