[5.4] Cannot remove a global scope using another global scope

[KKSzymanowski]

• Laravel Version: 5.4.23
• PHP Version: 7.1
• Database Driver & Version: MariaDB 10.1.21

Description:

Let's say we have products which can be soft-deleted.
Normal users can manipulate only existing products, but users with elevated privileges can manipulate also deleted ones. Adding a global scope which removes SoftDeletingScope under some condition does not work - the SoftDeletingScope is removed but is also applied right after.

Steps To Reproduce:

Product:

class Product extends Model 
{

    use SoftDeletes;

    protected static function boot()
    {
        static::addGlobalScope(function(Builder $query) {
            if(true/* some condition here */) {
                $query->withoutGlobalScope(SoftDeletingScope::class);
            }
        });

        parent::boot();
    }
}

In eg. a controller:

\DB::listen(function($query) {
    dump($query->sql);
});

Product::get();

You'll see deleted_at is null

Possible fix:

Before applying a scope, check if it hasn't been removed from the list.
Illuminate\Database\Eloquent\Builder::applyScopes

public function applyScopes()
{
    if (! $this->scopes) {
        return $this;
    }

    $builder = clone $this;

    foreach ($builder->scopes as $identifier => $scope) {
+       if(!isset($builder->scopes[$identifier])) {
+           continue;
+       }

        $builder->callScope(function (Builder $builder) use ($scope) {
            // If the scope is a Closure we will just go ahead and call the scope with the
            // builder instance. The "callScope" method will properly group the clauses
            // that are added to this query so "where" clauses maintain proper logic.
            if ($scope instanceof Closure) {
                $scope($builder);
            }

            // If the scope is a scope object, we will call the apply method on this scope
            // passing in the builder and the model instance. After we run all of these
            // scopes we will return back the builder instance to the outside caller.
            if ($scope instanceof Scope) {
                $scope->apply($builder, $this->getModel());
            }
        });
    }

    return $builder;
}

[Dylan-DPC-zz]

Why would you want to remove a global scope with another global scope?

[KKSzymanowski]

I provided a use case in the Description section.
How, do you think, should I proceed to allow seeing the deleted models only to users with elevated permissions?

[devcircus]

Can't you just add 'withoutGlobalScope(MyScope::class)' to the query, for that situation?

[KKSzymanowski]

@devcircus What would I achieve with that? What would be MyScope class?

I feel like we're getting off track here, but what I essentially want to do is avoid repeating withTrashed all over the codebase, so instead of:

public function index()
{
    if($condition) {
        return Product::withTrashed()->get();
    }

    return Product::get();
}

publifc function show($productId)
{
    if($condition) {
        return Product::withTrashed()->findOrFail($productId);
    }

    return Product::findOrFail($productId);
}

I could just do:

public function index()
{
    return Product::get();
}

publifc function show(Product $product)
{
    return $product;
}

and let the fetching logic be applied automatically based on user privileges.

[devcircus]

You said you wanted to remove a global scope for some users. I do the same thing in several apps. I have users in a certain role that get results from queries with global scopes applied, but for users with another role, I'll remove certain global scopes. It all depends on how your scopes are set up and how you implement roles.

[KKSzymanowski]

How do you remove scopes then?

> how your scopes are set up

I only care for the scope which is applied automatically when a model uses SoftDeletes trait.

> how you implement roles

I use the Laratrust package.

[themsaid]

This issue has been inactive for 23 days and you seem to have a suggested change, so I'm closing this but please feel free to open a PR with the suggested change, it's better we discuss implementation at this point.

[KKSzymanowski]

I have worked around it, but I think it's worth fixing. I'll make a PR soon.