[9.x] 🤞 Restore query builder interface

[inxilpro]

After a little digging, it turns out #40400 was due to an issue with trying to remain backwards-compatible with the deprecated Relation::getBaseQuery method (which isn't used anywhere in the framework). The bug affected toSql() but didn't actually impact query execution, which is why it wasn't caught in the tests.

I've added additional tests to DatabaseEloquentSoftDeletesIntegrationTest to verify that toSql() has been fixed, and also added an additional test for get() (which I confirmed passed both before and after this fix).

The only change in this PR is the one test case and this:

All-in-all, I hope you'll consider restoring the query builder interface. As you can see from the original PR, it had a lot of support!

And aside from one small issue in WinterCMS (which we resolved with a better solution), and this toSql() bug, the entire change has been quite seamless as far as I'm aware. Please let me know if there are other issues that have come up. I'm happy to help resolve them before the 9.0 release deadline!

[driesvints]

One big downside that I see with this is that we won't be able to add any methods to the interface for new methods added to the query builder as that's a breaking change. People will need to wait until the next major release until it'll be available.

[inxilpro]

My thought process there is that we add the methods to the builder and update the interface, if necessary, in the next major version. The interface only defines a subset of the builder, anyway. It's supposed to provide a common interface for typical query usage like where/etc. You can always do what we have to do in 8.x and type-hint the implementation (like HasMany) if you're relying on methods outside the interface. So essentially, the downsides already exist in 8.x and the upsides cover 95% of typical usage.

[taylorotwell]

How did you fix the toSql bug?

[inxilpro]

Relation has a getBaseQuery method that's never used anywhere in the framework, but I had kept the behavior of it in toBase in my original PR. toSql calls toBase directly instead of using the call forwarding helper. That meant that scopes weren't properly applied to toSql.

To fix I updated the implementation of toBase on relations to call toBase on the eloquent builder (rather than getQuery).

You can see a screen shot of the line that actually changed in the original description of the PR.

[inxilpro]

To be clear, toBase is a new method on Relation, so this change is not a backwards-compatibility issue for that method.

It does change the behavior of getBaseQuery a tiny bit, but we could arguably remove that method since it's not used anywhere (or leave it, as I have, with a @deprecated annotation).

[inxilpro]

In reviewing the tests, I noticed that there aren't any for a few other query builder methods when applied to relationships, so in addition to tests for toSql, I've added tests for:

• Relation::exists and Relation::doesntExist
• Relation::count
• Relation::min, Relation::max, Relation::sum, and Relation::avg
• Relation::newQuery

All these tests pass without changes to the PR, but it's just nice to have a little more coverage of fairly essential Eloquent code.

[driesvints]

I'm sorry but I really think we should hold off with this until Laravel 10 at the earliest now. A lot of packages have started implementing Laravel 9 and this is a very big breaking change for them to only have one week to deal with.

[inxilpro]

this is a very big breaking change for them to only have one week to deal with

I guess I would look at it the opposite way: this change was merged 6 months ago and removed very suddenly just before release. If we don't restore it, it could be a big breaking change for anyone who prepared for Laravel 9 any time in the last 6 months.

[inxilpro]

@taylorotwell @driesvints — I have a potential alternative option! If merging this back into 9.x feels too risky, maybe we could do this as a stop-gap between 9.x and 10.x…

We could potentially add an empty interface that has all the common builder methods annotated via docblocks. Something like:

/**
 * @method $this select(array|mixed $columns = ['*'])
 * ...
 */
interface Builder {}

And then have all the different builders implement that interface. Since the interface is empty, it wouldn't actually have to touch any methods, but the docblocks would allow for autocompletion in IDEs at least.

It's not as good a solution, since it would mean that cmd+click would take you to the docblock rather than the actual method, but it'd be better than ditching the interface altogether…

[driesvints]

@inxilpro I like that idea. It would allow changes to be made to the interface docblock's since they're not part of the public API. The only downside to it would be if there was a differentiation between the eloquent and regular query builder's methods. You wouldn't be able to fix that.

Please wait with doing any changes until @taylorotwell has responded.

[inxilpro]

You can actually get away with a @mixin annotation it looks like, which means the implementation would look something like:

<?php

namespace Illuminate\Contracts\Database\Query;

/**
 * @mixin \Illuminate\Database\Query\Builder
 */
interface Builder
{
}

The question of eloquent vs. base still remains, but I think we can figure that out. We could even add a second interface to use if you know for a fact you need the Eloquent\Builder features, but that would undermine some of the simplicity of this approach.

[inxilpro]

Another novel option would be to introduce a handful of interfaces that are simply applied to the correct Builder instances.

If we step back for a second, the goal is to make the ergonomics of code like this better:

Model::query()
  ->whereNotExists(function(BaseBuilder $query) {
    // I need to know that this is a Query Builder
  })
  ->whereHas('relation', function(EloquentBuilder $query) {
    // But that this is an Eloquent Builder
  })
  ->with('relation', function(Relation $query) {
    // And that this is a Relation instance
  });

We could introduce a handful of interfaces for common cases. This would let me write the code as:

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

If the builder interfaces matched the method names/intentions properly, it could be a way to solve the base/eloquent problem more easily. We'd do the hard part of identifying which is which so that the developer just needs to use the interface that matches the method call…

It's a little cute, but I thought I'd throw it out there as a potential solution. The upside is that whereNotExists and with are two separate APIs, and this would properly identify that difference for the developer without them needing to know exactly which flavor of builder they're working with ahead of time.

[taylorotwell]

I think some of these alternative ideas are a bit more interesting / compelling to me than this PR which has a ton of changes and is pretty overwhelming to commit to maintaining on my end forever. So, will close this for now in hopes one of these alternative approaches can be explored in the future.

[inxilpro]

I think some of these alternative ideas are a bit more interesting / compelling to me than this PR

@taylorotwell any thoughts on which direction you're the most comfortable with?