-
-
Notifications
You must be signed in to change notification settings - Fork 549
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.x] Support scopes as query methods #5927
Conversation
Only filter classes have a `context` method. Regular scopes don’t.
This should fix the error `Cannot use ::class with dynamic class name`
This reverts commit 46ca391.
This static property is collected in the `builders` method, which will always return a collection.
I added a bunch of tests. The only thing I wasn't sure about was how to test the eloquent builder for entries and users? |
…-scopes # Conflicts: # src/Tags/Concerns/QueriesScopes.php # tests/Data/Assets/AssetQueryBuilderTest.php # tests/Data/Entries/EntryQueryBuilderTest.php # tests/Data/Taxonomies/TermQueryBuilderTest.php
# Conflicts: # src/Query/EloquentQueryBuilder.php
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this - it looks good! I've noticed one small thing though... 🤔
This allows the query builder to fallback to scopes on Eloquent models.
# Conflicts: # tests/Data/Assets/AssetQueryBuilderTest.php # tests/Data/Entries/EntryQueryBuilderTest.php # tests/Data/Taxonomies/TermQueryBuilderTest.php # tests/Data/Users/UserQueryBuilderTest.php
- Scopes are not automatically available on query builder. You register them through facade. eg Entry::allowQueryScope(ScopeName::class); - If calling a method that doesnt correspond to a scope, throw an "undefined method" error like you'd get before - Reverts changes to tag query scoping
…ent builder. tests.
Ahem, sorry for the delay. 🫠 We talked through how we thought this should work and have made some changes. Scopes aren't automatically applied to all builders. You have to opt into them. You can do that in a provider: Entry::allowQueryScope(MichaelsScope::class); // handle of michaels_scope
// Allows $query->michaelsScope() You can set the method in the second argument: Entry::allowQueryScope(MyScope::class, 'whereMichael');
// Allows $query->whereMichael() This made it feel more close to how it works in Laravel. In Laravel you would allow these "local scopes" by adding methods to the Model. Since you aren't touching the models in Statamic, opting in like this was an alternative. |
I like it. Would it make sense to also scope the scope (no pun intended) to a collection? A |
I get the request, but not really. The entry query builder can query across collections. Also, I've adjusted the PR so that context needs to be passed into the method as an array. This is different to Laravel, but Statamic scopes are different. We may be able to change it for v6. i.e. Instead of function apply($query, $values) {
$foo = $values['foo'];
// ...
} |
I have been waiting so long for this. Thank you!! |
Pfft relax it was only opened in April 2022. |
This PR implements the new query scopes approach as discussed in PR #5869.
Each scope in the
App\Scopes
namespace is now also available to the query builders.Example
Let's assume we've got a
App\Scopes\PaymentSuccessful
scope class. This scope can now be used in Antlers and in PHP.The query method corresponds to the scope's handle in camel case.
By default, each scope is available to all query builders. You may want to restrict the scope to be available on certain query builders only. You can do that by adding the alias of the query builder to the
$builders
property.I also added some nice exceptions. But you might decide to remove them for a more graceful handling if the scope doesn't exist.