[10.x] Add Closure Type Hinting for Query Builders

[AJenbo]

This pull request adds type hinting for closures in Laravel's query builders and Eloquent, enhancing IDE support by providing hints about expected input functions and available operations for the passed builder type.

This improvement follows the existing pattern applied to some closures, such as in Illuminate\Database\Eloquent\Builder::withSavepointIfNeeded(). For more information on callable types in PHPDoc, refer to phpstan.org phpdoc callables.

Additionally, this PR removes obsolete @param annotations for parameters that have long been removed from the functions themselves.

Your feedback and review are greatly appreciated.

[taylorotwell]

Do any of these situations where you type-hinted a database query builder also support an Eloquent query builder because of the decorated nature of the Eloquent query builder?

[AJenbo]

@taylorotwell The closures do not receive an Eloquent query builder even when used within that context. This is because the Eloquent query builder doesn't extend the underlying query builder class; instead, it forwards the calls. The motivation behind adding these type hints was to address the potential confusion that arose from this behavior, as we encountered issues in our own code.

Typically, the closures are provided with a query from \Illuminate\Database\Query\Builder::newQuery() or an instance of $this, representing the inner \Illuminate\Database\Query\Builder.

I hope this clarifies the reasoning behind the type hints. If you have any other concerns let me know.

[markwalet]

This unfortunately breaks phpstan in the following example:

DB::table('table')
    ->leftJoin('other', fn (JoinClause $sub) => $sub->on('column', 'other'))

Error:

Parameter #2 $first of method Illuminate\Database\Query\Builder::leftJoin() expects
         (Closure(Illuminate\Database\Query\JoinClause): void)|string,

The error makes sense, because the join callback should not return anything. This however prevents us from using the arrow function as syntactic sugar. What is your opinion about that @taylorotwell ?

[taylorotwell]

Guess I'll revert for now. I swear we spend more time maintaining these comments than code at this point. 😬

[AJenbo]

I can send a follow in a little bit if you like?

[driesvints]

@AJenbo best that we just don't touch docblock for now.

[AJenbo]

Parameter #2 $first of method Illuminate\Database\Query\Builder::leftJoin() expects
         (Closure(Illuminate\Database\Query\JoinClause): void)|string,

The error makes sense, because the join callback should not return anything. This however prevents us from using the arrow function as syntactic sugar. What is your opinion about that

If I can give my opinion I would suggest we hint it as accepting both void and JoinClause as the return.
Since using anonymous functions returning void is the simpler option.

Alternativ we can also hint it as mixed since the caller doesn't really care about the return value?

[AJenbo]

@driesvints fair enough, would it be something to target for 11.x instead then?

Adding these hints did allow us to catch a handful of bugs in our production code so I do think there is a real value to adding them in the long run.

[gehrisandro]

@AJenbo I agree that this would be an improvement if carefully done. But I run into two more errors:

One for this:

$users = User::with(['posts' => function (Builder $query) {
    $query->where('title', 'like', '%code%');
}])->get();

Parameter #1 $relations of method Illuminate\Database\Eloquent\Builder<App\Models\User>::with() expects array<(Closure(Illuminate\Database\Eloquent\Relations\Relation): Illuminate\Database\Eloquent\Builder)|string|null>|string, array{'posts': Closure(mixed)} given.

And one more here:

User::query()->each(function (User $user): void {
// ...
}

Parameter #1 $callback of method Illuminate\Database\Eloquent\Builder<App\Models\User>::each() expects callable(object, int): bool, Closure(App\Models\User): void given.

[driesvints]

@AJenbo no, let's just leave this be. Like Taylor says, this is giving us too much headaches lately.

[AJenbo]

@gehrisandro

$users = User::with(['posts' => function (Builder $query) {
    $query->where('title', 'like', '%code%');
}])->get();

Parameter #1 $relations of method Illuminate\Database\Eloquent\Builder<App\Models\User>::with() expects array<(Closure(Illuminate\Database\Eloquent\Relations\Relation): Illuminate\Database\Eloquent\Builder)|string|null>|string, array{'posts': Closure(mixed)} given.

Yeah this one was a mistake I made, I was actually in the middle of testing a PR to fix this when things got reverted. Things get a bit hard to track because of the many places where calls are forwarded using __call. Also Relation does not declare @mixin with Illuminate\Database\Eloquent\Builder which also causes some issues when it's hinted as things like ->where() do not appear to be valid functions.

User::query()->each(function (User $user): void {
// ...
}

Parameter #1 $callback of method Illuminate\Database\Eloquent\Builder<App\Models\User>::each() expects callable(object, int): bool, Closure(App\Models\User): void given.

The thing here is that you can break out of each() by returning false: https://laravel.com/docs/10.x/collections#method-each.
I wonder how that could other wise best be hinted since mixed|false doesn't really bring any value. Maybe void|bool could be a viable alternative, although I don't think comparing void to false is seen as valid either.

@driesvints Ok, sorry for any trouble it may have caused, and thank you for the feedback. Is there any sort of roadmap/strategy for generally improving types and hints in Laravel? Maybe a better place to start would be looking at internal consistency?