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

[5.4] Add ability to remove a global scope with another global scope #19657

Merged
merged 3 commits into from
Jun 18, 2017
Merged

[5.4] Add ability to remove a global scope with another global scope #19657

merged 3 commits into from
Jun 18, 2017

Conversation

KKSzymanowski
Copy link
Contributor

As described in #19282, there is currently no way to remove a global scope with another global scope.
Example use case would be to automatically allow users with elevated privileges to view soft-deleted models.

class Product extends Model 
{

    use SoftDeletes;

    protected static function boot()
    {
        static::addGlobalScope(function(Builder $query) {
            if (auth()->user()->hasElevatedPrivileges()) {
                $query->withoutGlobalScope(SoftDeletingScope::class);
            }
        });

        parent::boot();
    }
}

This allows for replacing:

class ProductController extends Controller
{
    public function show($productId, Request $request)
    {
        if ($request->user()->hasElevatedPrivileges()) {
            return Product::withTrashed()->findOrFail($productId);
        }

        return Product::findOrFail($productId);
    }
}

with

class ProductController extends Controller
{
    public function show(Product $product)
    {
        return $product;
    }
}

@taylorotwell
Copy link
Member

Feels like a pretty hacky thing to do in your app but ok, heh.

@taylorotwell taylorotwell merged commit 1fe2ef8 into laravel:5.4 Jun 18, 2017
@devcircus
Copy link
Contributor

devcircus commented Jun 18, 2017

Definitely hacky. Doesn't feel laravelish.

@KKSzymanowski
Copy link
Contributor Author

@taylorotwell, @devcircus What approach would you recommend?

@devcircus
Copy link
Contributor

@taylorotwell merged it so I guess that would be the approach I'd take now. The part that seems hacky to me is the need for something like this in your app. Laravel devs often need to conditionally apply different scopes, yet I've never heard of this approach. Actually, to me, global scoping seems like the wrong approach if you're needing to conditionally apply it or alternate with another scope.
However, now that it's baked in to the framework, run with it.

@KKSzymanowski
Copy link
Contributor Author

Well, it felt a lot easier to strip down every conditional call to withTrashed from controllers and replace it with one global scope removal.

I don't think this PR was merged because it is the correct approach to solving this problem but rather because it was a bug which allowed for applying a scope which shouldn't be applied(didn't exist in the scopes array).

I'm open for a discussion about this. To be honest, removing the global scope after it was added felt weird but at the same time it felt a lot simpler than eg. overwriting SoftDeletes and SoftDeletingScope and adding the where clause under some condition.

mathieutu added a commit to mathieutu/framework that referenced this pull request Jun 19, 2017
* commit '05060b183dd09cee6dce498da5afde9e66fd1a29':
  Apply fixes from StyleCI (laravel#19659)
  [5.4] Add ability to remove a global scope with another global scope (laravel#19657)
  [5.4] Add fresh method on Eloquent\Collection. (laravel#19616)
  support multiple fields to validateDifferent (laravel#19637)
  escape default value of yield blade directive (laravel#19643)
  [5.4] Add "avg" and "average" to higher order proxy. (laravel#19628)
  tagged v5.4.27 release notes
  version
  Add missing dependency. (laravel#19623)
  update timestamp on soft delete only when its used (laravel#19627)
  Add new `diffAssoc()` method to collection (laravel#19604)
  Fix tests code coverage config. (laravel#19609)

# Conflicts:
#	src/Illuminate/Database/Eloquent/Collection.php
chrishardinge pushed a commit to chrishardinge/framework that referenced this pull request Jun 21, 2017
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.

3 participants