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

Make query builders macroable #5869

Closed
wants to merge 4 commits into from

Conversation

aerni
Copy link
Contributor

@aerni aerni commented Apr 20, 2022

This PR allows adding additional methods to the query builders by using Laravel's Macroable trait. As discussed a few months back in a partners' chat. This is super useful for custom query methods.

// Instead of this …
Entry::query()
    ->where('payment_status', 'succeeded')
    ->orWhere('payment_status', 'pending');

// … we can now do something like this
Entry::query()->wherePaymentSuccessful();

@jasonvarga
Copy link
Member

Nice. So the usage would be:

Statamic\Facades\Entry::query()->macro('wherePaymentSuccessful', function () {
    return $this
        ->where('payment_status', 'succeeded')
        ->orWhere('payment_status', 'pending');
});

@aerni
Copy link
Contributor Author

aerni commented Apr 20, 2022

Exactly! Or directly on the query builder like this:

Statamic\Stache\Query\EntryQueryBuilder::macro('wherePaymentSuccessful', function () {
    return $this
        ->where('payment_status', 'succeeded')
        ->orWhere('payment_status', 'pending');
});

Copy link
Member

@jasonvarga jasonvarga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting Macroable on the abstract classes like this would make the macro available to all query builders.

Entry::query()->maco('foo', ...); would make Term::query()->foo() work. That feels odd. Right?

Putting Macroable on the concrete classes makes it work as I would expect (add a macro to entries, it won't work on terms), but it's more of a pain to need to do that. 🤔

Not sure of the better solution.

(Also, if we're going to put it on the abstract classes, you should have put it on Statamic\Query\Builder instead of Statamic\Stache\Query\Builder.)

@aerni
Copy link
Contributor Author

aerni commented Apr 20, 2022

Yeah, I came across the same dilemma. Maybe add it to the concrete classes as there are not that many anyway?

@jasonvarga
Copy link
Member

The problem there is that you'd need to add it to any variations too. Like the eloquent driver, or who knows what else if someone is using another custom driver.

Another option could be to avoid Laravel's Macroable trait and make our own that only binds to the class you've called it on?

e.g. In here, instead of just putting the name of the macro directly in the array, maybe the array could also organize the macros by class names.

https://github.com/laravel/framework/blob/df67f44f58fd7fbdbb05960c1497dcd30cdb7fd0/src/Illuminate/Macroable/Traits/Macroable.php#L26-L29

Something like this:

public static function macro($name, $macro)
{
    static::$macros[static::class][$name] = $macro;
}

@aerni
Copy link
Contributor Author

aerni commented Apr 20, 2022

Thought about this too. I think that could work. I'll give it a shot.

@aerni
Copy link
Contributor Author

aerni commented Apr 20, 2022

I created a new Macroable trait that now scopes the macros to the query builders.

@aerni aerni requested a review from jasonvarga April 20, 2022 18:40
@aerni
Copy link
Contributor Author

aerni commented Apr 23, 2022

What if we added some syntactical sugar similar to Laravel's query scopes? The following adds a wherePaymentSuccessful macro to the EntryQueryBuilder.

Statamic\Facades\Entry::scopeWherePaymentSuccessful(function () {
    return $this
        ->where('payment_status', 'succeeded')
        ->orWhere('payment_status', 'pending');
});

@ryanmitchell
Copy link
Contributor

Statamic\Facades\Entry::scopeWherePaymentSuccessful(function () {
    return $this
        ->where('payment_status', 'succeeded')
        ->orWhere('payment_status', 'pending');
});

Adding scopeXXX would be so good. I've reached for this a few times and wished it was there.

@jasonvarga
Copy link
Member

If we add the scope magic methods, we should get rid of macro.

@aerni
Copy link
Contributor Author

aerni commented Apr 26, 2022

Hmm, not sure how to get rid of macro as it's still needed to add the query methods to the QueryBuilder. This is the current implementation of the scope magic within the EntryRepository.

public function addQueryScope($name, $callback)
{
    $this->query()->macro($name, $callback);
}

public function __call($method, $parameters)
{
    if (Str::contains($method, 'scope')) {
        $scope = Str::camel(Str::remove('scope', $method));
        $this->addQueryScope($scope, $parameters[0]);
    }
}

@jasonvarga
Copy link
Member

Ok maybe we just don't document it. 😄

@aerni
Copy link
Contributor Author

aerni commented Apr 27, 2022

I've given this some more thought. While I like the approach of scope methods like scopeWherePaymentSuccessful, it kind of feels like we're trying to reinvent the wheel. We already have the concept of Scopes. Maybe we can hook into those instead of using macros.

You would create a new scope class and register the scope to the desired query builder. This way, you can apply the same query scope in Antlers and PHP without having to rewrite the logic in different places.

Statamic\Facades\Entry::addQueryScope(WherePaymentSuccessful::class);

Does this make sense? This idea is untested. So I don't yet know if it's doable or not.

One downside, though, is that you will be limited with the naming of the scopes. You won't be able to create scopes with the same name for different query builders doing different things.

@jasonvarga
Copy link
Member

Good catch. There are so many features it's hard to remember them all. 😬

Your idea makes a lot of sense.

So right now when the scope is registered you can reference it in a tag like query_scope="where_payment_successful". Like you said, it's available to any queries.

Maybe we can add a property to scope classes that restricts to which query builders it becomes available.

protected static $builders = [ 'entries', 'terms'];

If not explicitly defined, it'll just get registered globally like it currently does.

(These keys could correspond to Statamic::query() aliases defined here)

collect([
'entries' => fn () => Facades\Entry::query(),
'terms' => fn () => Facades\Term::query(),
'assets' => fn () => Facades\Asset::query(),
'users' => fn () => Facades\User::query(),
])->each(function ($binding, $alias) {

As for naming, you can already define the handle property if you need it different from the filename, but we could introduce a new property for the method name.

// EntryWhereSomething.php
protected static $handle = 'where_something';
protected static $method = 'whereSomething';
protected static $builders = ['entries'];
// TermWhereSomething
protected static $handle = 'where_something';
protected static $method = 'whereSomething';
protected static $builders = ['terms'];

Maybe? 😄

@aerni
Copy link
Contributor Author

aerni commented Apr 27, 2022

I like it! I'm going to give this a go.

Also, I'm still going to add a way to manually register a scope. This is handy if you're in an addon context.

Statamic\Facades\Entry::addQueryScope(WherePaymentSuccessful::class);

@aerni
Copy link
Contributor Author

aerni commented Apr 27, 2022

Closing this PR in favor of #5927.

@aerni aerni closed this Apr 27, 2022
@aerni aerni deleted the feature/macroable-builder branch December 8, 2023 20:11
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