[9.x] Query builder interface

[inxilpro]

Inspiration

For developers who rely on type hints for static analysis, refactoring, or code completion in their IDE, the lack of a shared interface or inheritance between Query\Builder, Eloquent\Builder and Eloquent\Relation can be pretty tricky:

return Model::query()
  ->whereNotExists(function($query) {
    // $query is a Query\Builder
  })
  ->whereHas('relation', function($query) {
    // $query is an Eloquent\Builder
  })
  ->with('relation', function($query) {
    // $query is an Eloquent\Relation
  });

Often case you just take your best guess, and then change the type hint when you hit a TypeError exception:

TypeError: Argument 1 passed to closure must be an instance of Illuminate\Database\Eloquent\Builder, instance of Illuminate\Database\Eloquent\Relations\HasMany given

History

The idea of a Query Builder contract has been kicked around for the last 3–4 years. I think when it was first brought up, the use of type hints was much less common. Today, I would venture to guess that a good number of Laravel codebases rely on type hints extensively, which makes me think that maybe it's time to revisit the idea.

Overview of Changes

This PR adds a new Illuminate\Contracts\Database\QueryBuilder interface and a Illuminate\Database\Eloquent\Concerns\DecoratesQueryBuilder trait that implements the interface in place of the existing __call implementation.

Please Note: The vast majority of ~2,000 additions in this PR are due to the fact that the existing Eloquent\Builder and Eloquent\Relation classes call the underlying Query\Builder object via the __call method, but once an interface exists, those methods (and doc blocks) have to be defined.

Implementation Decisions

Because of the (somewhat artificially) big diff, I want to point out a few decisions/changes:

• The QueryBuilder contract only covers the build side of queries. Query execution is excluded for 3 reasons:
  1. This keeps the scope of changes as small as possible
  2. The vast majority of use-cases exist in building queries, whether it's adding a subquery, setting relation constraints, or similar
  3. There's more difference in query execution between the three classes than there is in query building
• A few minor changes were added to keep the interface clean. These include:
  • changing mergeWheres to a fluent method rather than returning void (it's used fluently in Eloquent\Builder but returns void in Query\Builder) and
  • adding a toBase() method to Eloquent\Relation so that the same method name can be used across all implementations. (I've also marked getBaseQuery as deprecated in favor of the new toBase method.)
• I've updated a few doc blocks to better represent the existing code. This includes:
  • Adding QueryBuilder and Expression types as valid arguments to orderBy and similar functions (the current implementation accepts these types, but the doc blocks did not).
  • Adding QueryBuilder as a valid type for the $column argument in where() (again, this was already the case—it was just missing in the doc block).
• Any methods not implemented in the interface/trait are still caught by __call(), so all existing functionality continues to work

Files Changed

src/Illuminate/Contracts/Database/QueryBuilder.php

This is the new query builder contract. It was created by copying the existing signature and doc block from Query\Builder and BuildsQueries (where appropriate) with as few changes as possible (noted above).

src/Illuminate/Database/Eloquent/Builder.php

The Eloquent builder now implements the new interface and uses the new trait. Because the DecoratesQueryBuilder trait handles calling the underlying $query directly, the $passthru array can be safely removed.

src/Illuminate/Database/Eloquent/Concerns/DecoratesQueryBuilder.php

This trait replaces dynamically calling the base builder via __call (although it still supports forwarding calls to the builder to catch any other method that's not defined in the interface).

Most method calls delegate to forwardCallToQueryBuilder(), which calls the function on the underlying query builder while ensuring that methods remain fluent.

Methods that had been in Eloquent\Builder::$passthru call the base builder directly (which was the purpose of $passthru). This is done thru a toBase() method, which allows the Eloquent Builder to apply scopes beforehand.

src/Illuminate/Database/Eloquent/Relations/MorphTo.php

The changes here are to move the $macroBuffer logic that had been handled in __call into the concrete implementations of those functions.

src/Illuminate/Database/Eloquent/Relations/Relation.php

Relations now also implement the new interface and use the new trait. The only other change is to add a new toBase and deprecate getBaseQuery so that all classes that use DecoratesQueryBuilder can use the same method name.

src/Illuminate/Database/Query/Builder.php

The base query builder now implements the new interface. The mergeBindings method was updated to use the interface rather than a self type hint. The only other change in this file is to replace the existing doc blocks with {@inheritdoc} tags, since the canonical doc blocks now exist in the QueryBuilder interface.

Tests

It's worth noting that this PR passes all existing tests without the need for any changes!

Whew!

OK, I know that's a lot. I tried to be as descriptive as possible because I know these large copy-and-paste type PR's can be daunting. All-in-all, the number of changes are fairly minimal. Most of the PR size comes from the inherent boilerplate necessary. Please let me know if there's anything else I can do to make it easier to review!

[taylorotwell]

So, if query building is excluded, is the IDE / static analysis tool going to complain when you call the execution method in your code?

[inxilpro]

So, if query building is excluded, is the IDE / static analysis tool going to complain when you call the execution method in your code?

If you type hint QueryBuilder then you won't get query execution auto-completed. I think that's fine, because nearly all the cases where you need the interface are solely in the building context:

return Model::query()
  ->whereNotExists(function(QueryBuilder $query) {
    $query->select('id')->from('model')->whereColumn(...); // Build context
  })
  ->whereHas('relation', function(QueryBuilder $query) {
    $query->where('is_special', true); // Build context
  })
  ->with('relation', function(QueryBuilder $query) {
    $query->where('is_special', true); // Build context
  })
  ->get(); // Execution context — still works fine because Model::query() returns an Eloquent Builder

I think it'd probably be possible to add the rest of the functions to the interface. The decision not to what more to make the PR a little more manageable to review, and the main benefits don't need those functions in the interface.

[driesvints]

@inxilpro I just merged 8.x into master so there's a couple of conflicts to fix.

[driesvints]

Tbvh this method can be removed and deprecated in 8.x instead. But up to @taylorotwell.

[driesvints]

@inxilpro looks good now. Amazing work 👍

[deleugpn]

So, if query building is excluded, is the IDE / static analysis tool going to complain when you call the execution method in your code?

I think even if we end up needing more than this, it's totally fine to get this out and wait one other major version to gather feedback on which direction it might need improvement, if needed are all.

[LukeTowers]

@driesvints @inxilpro this has caused a problem for us at @wintercms since having the Query methods defined on the Builder object through the use of the DecoratesQueryBuilder trait breaks the usage of $builder->macro('orderBy') to override the default behaviour of a given Query method call on the Builder instance.

The practical application that it was being used by was our Sortable model trait which attaches the SortableScope global scope to the model. This scope overrides the orderBy method through the use of a macro in order to remove itself from the query whenever orderBy() is explicitly called on that Builder instance.

Doing a search on GitHub didn't seem to reveal any other use cases similar to ours, but do you have any ideas on how we can achieve the same end result in Laravel 9 (remove the global scope when an explicit orderBy() is called)?

[inxilpro]

I'm not sure that I fully understand, but would it be possible to implement the newQuery method in your trait to return a custom builder with the method overridden?

[LukeTowers]

Possibly, although that would prevent any other traits from doing a similar thing. At this point (since Winter already extends and replaces the base Builder class) I may just override the method there and check the macros first before calling the parent, but I'm wondering if @driesvints has any better ideas?

[inxilpro]

It seems like another option would be to just check whether any orders are applied to the query and only set your default if there aren't any. Something like (in your scope's apply method):

if (empty($builder->getQuery()->orders) && empty($this->getQuery()->unionOrders)) {
  $builder->orderBy($model->getSortOrderColumn());
}

Since applyScopes() is called just before the query is executed, that should work.

[LukeTowers]

Not a bad idea, thanks @inxilpro!

[LukeTowers]

@inxilpro worked great, thanks again!

[MatanYadaev]

@inxilpro Do you think it's feasible to make it work with custom Eloquent Builders?

class Post extends Model
{
  public function newEloquentBuilder($query): PostBuilder
  {
    return new PostBuilder($query);
  }
}

use Illuminate\Database\Eloquent\Builder;

class PostBuilder extends Builder
{
  public function published(): self
  {
    return $this->whereNotNull('published_at');
  }
}

return User::query()
  ->whereHas('posts', function(PostBuilder $query) { 
    // This will work. `$query` is `PostBuilder`
    $query->published();
  })
  ->with('posts', function(PostBuilder $query) {
    // This won't work, `$query` is `HasMany` and not `PostBuilder
    $query->published();
  })
  ->get();

[taylorotwell]

This PR is pretty busted and needs to be entirely reverted - it has caused the issue above and is the cause of the following new issue: #40400

I'm not willing to try to support this in 9.x on my own so will reverting it all out.

[taylorotwell]

Reverted.

[inxilpro]

@taylorotwell what issue above?

The issue that @LukeTowers brought up was caused by them globally replacing the orderBy() method via a macro, which I would argue was a bit hacky to begin with. The solution we came up with is much simpler and requires no macro hacks.

The question from @MatanYadaev isn't a bug. I saw it on my phone and meant to reply later, but the basic answer is that you can't type-hint in the way that he's asking, but you couldn't type-hint in that way in the 8.x branch, either.

As for #40400 — I'm happy to look into it.

Are there other issues that came up from this PR that I'm not aware of?

[inxilpro]

I've resolved the bug in #40400 and submitted #40402 as a fix. I hope you'll re-consider!

[bryceandy]

@inxilpro Is this the way to go?

use Illuminate\Contracts\Database\Query\Builder;

Model::query()
  ->whereNotExists(function(Builder $query) {
    //
  })
  ->whereHas('relation', function(Builder $query) {
    //
  })
  ->with('relation', function(Builder $query) {
    //
  });

[inxilpro]

Is this the way to go?

@bryceandy in Laravel 8.x, and 9.x if #40402 isn't merged, you would have to do:

use Illuminate\Database\Query\Builder as BaseBuilder;
use Illuminate\Database\Eloquent\Builder as EloquentBuilder;
use Illuminate\Database\Eloquent\Relations\Relation;

Model::query()
  ->whereNotExists(function(BaseBuilder $query) {
    //
  })
  ->whereHas('relation', function(EloquentBuilder $query) {
    //
  })
  ->with('relation', function(Relation $query) {
    //
  });

In Laravel 9.x if #40402 is merged, then the code would work by just type-hinting the contract everywhere.

[bryceandy]

In Laravel 9.x if #40402 is merged

Makes sense. I tried in 9.x thinking it was merged 👍🏿

[inxilpro]

@bryceandy it was merged in July, but got reverted recently. We're discussing restoring it or coming up with a partial option in #40402.

[LukeTowers]

@driesvints do you think mentioning that macro() can't be used to override QueryBuilder methods anymore in the Laravel 9 upgrade guide would be worth it?

I'll be mentioning it in the Winter CMS upgrade guide ("Default QueryBuilder methods are defined in the Builder class now preventing the user of $builder->macro() to override them in userspace, see #37956 (comment) for details.") but thought I'd check with you if you think it's worth bringing up. I noticed that @octobercms hasn't seemed to have picked up on the fact that this approach no longer works: octobercms/library@bdc5776

[inxilpro]

@LukeTowers this implementation was eventually reverted in favor of #40546 — which means that macros should continue to work the same way that they used to…