Skip to content
This repository has been archived by the owner on Jul 16, 2021. It is now read-only.

Allow to omit return in callback of Query Builder when() #233

Closed
vlakoff opened this issue Sep 29, 2016 · 4 comments
Closed

Allow to omit return in callback of Query Builder when() #233

vlakoff opened this issue Sep 29, 2016 · 4 comments

Comments

@vlakoff
Copy link

vlakoff commented Sep 29, 2016

I think laravel/framework#13091 should be reconsidered. Here is an example use case:

DB::from('foobar')
    ->when($exclude_ids, function ($qb) use ($exclude_ids) {
        return $qb->whereNotIn('id', (array) $exclude_ids);
    })
    ->inRandomOrder()
    ->limit($nb)
    ->get();

The "when" part isn't very sexy…, removing the return makes it a bit better:

DB::from('foobar')
    ->when($exclude_ids, function ($qb) use ($exclude_ids) {
        $qb->whereNotIn('id', (array) $exclude_ids);
    })
    ->inRandomOrder()
    ->limit($nb)
    ->get();
  • The Query Builder instance is mutable, so it doesn't really make sense to return the instance.
  • It is backward compatible, if you return anything else from the callback it will be used instead in the chain.
  • The change in when() implementation is very simple.
@tomschlick
Copy link

return is consistent with the rest of the framework.

For instance query scopes: https://laravel.com/docs/5.3/eloquent#query-scopes

and almost every method on the collections: https://laravel.com/docs/5.3/collections#method-map

@vlakoff
Copy link
Author

vlakoff commented Sep 29, 2016

I think Collection methods are really a different case, where return is of course perfectly fine.

About scopes:

  • The return is less bothering as it is inside simple methods, rather than a full query builder chain.
  • In fact, the return in scopes apply()'s could (should) be omitted, as the return value is not used. See Builder::applyScopes() implementation, and the @return void phpdoc for apply().
  • For dynamic scopes "scopeFoobar" there is a $this fallback similar to what I'm suggesting here. See in callScope() implementation: $result = call_user_func_array($scope, $parameters) ?: $this;

@vlakoff
Copy link
Author

vlakoff commented Oct 7, 2016

See also the code in this recent tweet: https://twitter.com/taylorotwell/status/784407839162851328

$validator = Validator::make(['name' => 'Taylor Otwell'], [
    'name' => [
        Rule::unique('users')->ignore(2)->using(function ($query) {
            $query->whereIn('foo', [1, 2, 3]);
        })
    ],
]);

edit: above new validation syntax has been implemented: laravel/framework#15809

@vlakoff
Copy link
Author

vlakoff commented Mar 22, 2017

Implemented in Laravel 5.4.16, by laravel/framework#18422.

@vlakoff vlakoff closed this as completed Mar 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants