Skip to content

[7.x] Reset select bindings when setting select#32531

Merged
taylorotwell merged 1 commit intolaravel:7.xfrom
sebsel:7.x
Apr 24, 2020
Merged

[7.x] Reset select bindings when setting select#32531
taylorotwell merged 1 commit intolaravel:7.xfrom
sebsel:7.x

Conversation

@sebsel
Copy link
Copy Markdown
Contributor

@sebsel sebsel commented Apr 24, 2020

fixes #32526


This PR resets the select bindings when the select clause is set to something else.

Most operations on the Query Builder are additive: when you have an instance of it, and you call ->where('column', 'value') on it, the clause is added to the underlying query.

This also works with sub-queries: bindings are merged into the query. Take for example this orderBy():

User::orderBy(
    Book::whereColumn('users.id', 'user_id')
        ->where('has_valid_isbn', true)
        ->select(DB::raw('count(*)'))
)->orderBy('created_at');

// select * from "users" 
// order by
//     (select count(*) from "books" where "users"."id" = "user_id" and "has_valid_isbn" = ?) asc, 
//     "created_at" asc"

// Bindings: [true]

Setting a select query, however, overwrites previous selects. (Use addSelect() when you want to add.)

$builder = Book::select('title');
$builder->toSql(); //  select "title" from "books"
$builder->select('*');
$builder->toSql(); //  select * from "books"
$builder->addSelect('title');
$builder->toSql(); //  select *, "title" from "books"

When using a subquery in the select clause, registered bindings would currently stay in the query. MySQL 8.0 seems to be fine with that, but Postgres gives an error when this happens.

>>> $query = App\User::withCount('books');
=> Illuminate\Database\Eloquent\Builder {#3013}
>>> $query->toSql();
=> "select "users".*, (select count(*) from "books" where "users"."id" = "books"."user_id" and "has_valid_isbn" = ?) as "books_count" from "users""
>>> $query->getBindings();
=> [
     true,
   ]
>>> $query->select('*')->toSql();
=> "select * from "users""
>>> $query->select('*')->getBindings();
=> [
     true,
   ]
>>> $query->select('*')->get();
Illuminate/Database/QueryException with message 'SQLSTATE[08P01]: <<Unknown error>>: 7 ERROR:  bind message supplies 1 parameters, but prepared statement "pdo_stmt_00000003" requires 0 (SQL: select * from "users")

This PR fixes that by setting the bindings for the select part of the queries to an empty array when an overwriting ->select() call is made.

@driesvints driesvints changed the title Reset select bindings when setting select [7.x] Reset select bindings when setting select Apr 24, 2020
@driesvints
Copy link
Copy Markdown
Member

@sebsel best that you explain why this is needed (see the pr template)

@sebsel
Copy link
Copy Markdown
Contributor Author

sebsel commented Apr 24, 2020

@driesvints I updated the text. Hope this works, because I can't seem to find a PR template.

@driesvints
Copy link
Copy Markdown
Member

@sebsel This is visible whenever you open up a new PR:

https://raw.githubusercontent.com/laravel/framework/7.x/.github/PULL_REQUEST_TEMPLATE.md

@taylorotwell taylorotwell merged commit 2e0cfbc into laravel:7.x Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setting a new select for a query does not clear bindings

3 participants