Define a common interface for Query\Builder and Eloquent\Builder

[Patryk27]

Illuminate\Database\Query\Builder and Illuminate\Database\Eloquent\Builder share a few common methods, namely: where, whereIn etc.

I think it could be useful if both classes implemented a common interface (named, say, QueryBuilderInterface), because:

1. They actually do have common methods that do the same, thus interface could be used to ensure these both classes do not diverge on the common methods.

2. It could be useful in cases like this one:

$someConnection->where(function(QueryBuilderInterface $db) {
  $db->where(...);
});

Right now one must either skip the type-hinting or do the type-hinting depending on the actually used builder, which is kind of unnecessary, since they both share the same methods we want to use (it also makes it harder/impossible to create re-usable callbacks, for what it's worth, if someone wanted to use exact function in both the Query & Eloquent builder).

I've prepared a minimal patch depicting my idea - I think eventually we should refactor at least all the *where* methods and think what to do with the rest.

https://github.com/Patryk27/framework/commit/e76cafb136c2d01028e6220a443a770594d5218f

[sisve]

Both builders use the BuildsQueries trait, so we can possibly also expose these methods in the interface.

Anyhow, are we sure that the signatures of all the methods are identical? I just checked the where method, and the typehints in the docblocks differ between the two builders. It could be an error in the docblocks since one forwards the calls to the other, so they should be very similar.

[Patryk27]

Right, moving the methods to BuildsQueries also sounds reasonably to me :-)

Anyhow, are we sure that the signatures of all the methods are identical?

Like you said - Eloquent\Builder forwards nearly all the calls straight away, so signatures have to be identical.

Even the PHPDocs are actually very similar, with only one small difference (different types annotated for the $operator parameter, which seems to be a mistake or a left-over, since Eloquent\Builder does not do anything with that parameter except than passing it to the Query\Builder).

[inxilpro]

I would love to see this, too. I just started digging thru the repo to see if anyone else had suggested this. I'm constantly frustrated by the fact that you can't type hint "a builder" without worrying about which builder context you're dealing with.

[inxilpro]

OK, I've dug into this a little further, and there are a potential issues:

1. Eloquent\Builder checks for macros first, and then defers to the internal Query\Builder second. That means right now you can define a macro on the Eloquent builder that supersedes the Query builder call. Explicitly defining those functions on the builder via a trait/etc would prevent that behavior, making this a backwards-incompatible change.
2. The return value for the builder calls is different depending on the context, so I'm not 100% sure you can share an interface (although I think this can be solved by type hinting the Interface as the return type).

I think it's possible, but it'd definitely have to be a 5.7 feature if it happens at all. And I think we'd need to better understand how often people use macros to override methods on the query builder. I feel like that's a terrible idea, but it may be something that a lot of people rely on.

Thoughts?

[tekook]

Any Update on this?

[eberkund]

Yea this would be great for IDE autocompletion.

[AdamGaskins]

This would be incredible!

What is the expected way of doing this now? If I want to pass a query object (which could be a query builder or an eloquent builder) am I just not supposed to use explicit types?

[BenceSzalai]

Eloquent\Builder checks for macros first, and then defers to the internal Query\Builder second. That means right now you can define a macro on the Eloquent builder that supersedes the Query builder call. Explicitly defining those functions on the builder via a trait/etc would prevent that behaviour, making this a backwards-incompatible change.

But if someone implements a macro that overrides the signature of the built in method, that is breaking type hinting and class conformity anyway, isn't it? I mean what is the difference between setting up a macro on an Eloquent\Builder that contradicts the signature of Eloquent\Builder itself vs. setting up a macro that contradicts a contract implemented by Eloquent\Builder? Both cases are bad from a logical perspective and would confuse IDE autocomplete and type hinting. That is not an issue down to a common contract but due to the way the macro is defined.
Also macros can be fragile from that perspective anyway, because if I set up a macro today called e.g. whereOldest, and Laravel decides tomorrow to implement a new method on the Builder called the same, I'll have to deal with the situation when I update Laravel to the next version. So any issues down to macros could be just dealt with everyone upgrading to the latest Laravel the same as they possibly would have to even without a common interface.

The return value for the builder calls is different depending on the context, so I'm not 100% sure you can share an interface (although I think this can be solved by type hinting the Interface as the return type).

Do you have an example?

Probably a mandatory task for this to happen would be to review all methods in Eloquent\Builder (EB hereinafter for simplicity) and Query\Builder (QB hereinafter), find those that are shared and identify differences between their signatures. Once the differences are clear resolution need to be found for them. For differences in parameters and return types common interfaces have to be found or created. - In theory. However in reality, this is not needed - see analysis below.

In fact there are not that many candidates for such interface, as only a few method names are shared between the two. Those which have compatible signatures, most still has differences in their parameter and return type declarations in PHPDocs. Candidates for the interface are:

• Everything inside Illuminate\Database\Concerns\ExplainsQueries as that is used by both, but that is only one method: explain
• Everything inside Illuminate\Database\Concerns\BuildsQueries as that is used by both, except for sole which is overridden in EB
• sole : return types are different, EB: \Illuminate\Database\Eloquent\Model vs QB: \Illuminate\Database\Eloquent\Model|object|static|null, but internally they use the same method, so really this is currently incorrect and this method can be added to a common interface as well.
• where: $column parameter in QB: \Closure|string|array, in EB: \Closure|string|array|\Illuminate\Database\Query\Expression - likely a mistake, as EB forwards the $column to QB, and it accepts Query\Builder, Eloquent\Builder, Illuminate\Database\Eloquent\Relations\Relation as well, so probably PHPDoc is incomplete on both and could be the same, and can be added to a common interface
• orWhere, latest & oldest : similar situation as for where, and can be added to a common interface
• find : technically they are compatible, but they behave different, as EB accepts an array for ids and can return and array of models, while QB only works with a single record at a time. Due to this their return types are very much different as well. It could still be added to an interface, but both the $id parameter and the return type should be mixed - But that is a question how much value that would add?
• value: type of $column parameter is declared to be different, but they should be the same as both method uses first internally
• get: return type is different, EB: \Illuminate\Database\Eloquent\Collection|static[], QB: \Illuminate\Support\Collection. Interface may use \Illuminate\Support\Collection|static[].
• pluck: $column argument is string on QB, but string|\Illuminate\Database\Query\Expression on EB, however EB than uses $column in the same method of QB, so while I don't know which is the correct, but should be the same and that should be fine on the interface as well.
• cursor, update, upsert: fully compatible
• paginate, simplePaginate: there are differences in argument types, because |null is missing for some optional arguments, but they are fundamentally compatible and all should have |null added
• increment, decrement: type of $column parameter is different in PHPDoc, but EB forwards the calls to the base QB, so they should be the same, so these are fine for an interface as well.
• delete has a parameter in QB, but not in EB, so this cannot be added to a common interface without changes to the code

I could not find any other candidate methods, but I might have missed some.

It would be nice to hear the opinions of the project maintainers, if it is worth writing up a draft of Illuminate\Database\BuilderInterface for these?

[inxilpro]

I just took another stab at this. Take a look at the PR!