Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[5.3] fix bindings on update statements with advanced joins for all grammars #16368

Merged
merged 1 commit into from
Nov 14, 2016
Merged

[5.3] fix bindings on update statements with advanced joins for all grammars #16368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/Illuminate/Database/Query/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2160,12 +2160,10 @@ public function insertGetId(array $values, $sequence = null)
*/
public function update(array $values)
{
$bindings = array_values(array_merge($values, $this->getBindings()));

$sql = $this->grammar->compileUpdate($this, $values);

return $this->connection->update($sql, $this->cleanBindings(
$this->grammar->prepareBindingsForUpdate($bindings, $values)
$this->grammar->prepareBindingsForUpdate($this->bindings, $values)
));
}

Expand Down
9 changes: 8 additions & 1 deletion src/Illuminate/Database/Query/Grammars/Grammar.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use Illuminate\Database\Query\Builder;
use Illuminate\Database\Query\JoinClause;
use Illuminate\Database\Grammar as BaseGrammar;
use Illuminate\Support\Arr;

class Grammar extends BaseGrammar
{
Expand Down Expand Up @@ -730,7 +731,13 @@ public function compileUpdate(Builder $query, $values)
*/
public function prepareBindingsForUpdate(array $bindings, array $values)
{
return $bindings;
$bindingsWithoutJoin = Arr::except($bindings, 'join');

$preparedBindings = array_values(
array_merge($bindings['join'], $values, Arr::flatten($bindingsWithoutJoin))
);

return $preparedBindings;
}

/**
Expand Down
8 changes: 2 additions & 6 deletions src/Illuminate/Database/Query/Grammars/MySqlGrammar.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,18 +166,14 @@ protected function compileJsonUpdateColumn($key, JsonExpression $value)
*/
public function prepareBindingsForUpdate(array $bindings, array $values)
{
$index = 0;

foreach ($values as $column => $value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change this unrelated code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bindings are coming in raw format now, so the processing should be changed accordingly and can be simplified.

Copy link
Member

@taylorotwell taylorotwell Nov 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case how will the check for isJsonSelector work? Since you say they are coming in with numeric keys? Won't $column always be an integer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously we were receiving merged bindings and checked isJsonSelector first count($values) of items from bindings, because values always were first and the keys were of integer type.
In this case we can check only values and then merge with bindings and the keys can be of any type.

if ($this->isJsonSelector($column) &&
in_array(gettype($value), ['boolean', 'integer', 'double'])) {
unset($bindings[$index]);
unset($values[$column]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$column can be of any type, not only integer

}

$index++;
}

return $bindings;
return parent::prepareBindingsForUpdate($bindings, $values);
}

/**
Expand Down
19 changes: 19 additions & 0 deletions src/Illuminate/Database/Query/Grammars/PostgresGrammar.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Illuminate\Support\Str;
use Illuminate\Database\Query\Builder;
use Illuminate\Support\Arr;

class PostgresGrammar extends Grammar
{
Expand Down Expand Up @@ -87,6 +88,24 @@ public function compileUpdate(Builder $query, $values)
return trim("update {$table} set {$columns}{$from} $where");
}

/**
* Prepare the bindings for an update statement.
*
* @param array $bindings
* @param array $values
* @return array
*/
public function prepareBindingsForUpdate(array $bindings, array $values)
{
$bindingsWithoutJoin = Arr::except($bindings, 'join');

$preparedBindings = array_values(
array_merge($values, $bindings['join'], Arr::flatten($bindingsWithoutJoin))
);

return $preparedBindings;
}

/**
* Compile the columns for the update statement.
*
Expand Down
19 changes: 19 additions & 0 deletions src/Illuminate/Database/Query/Grammars/SqlServerGrammar.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Illuminate\Database\Query\Grammars;

use Illuminate\Database\Query\Builder;
use Illuminate\Support\Arr;

class SqlServerGrammar extends Grammar
{
Expand Down Expand Up @@ -333,6 +334,24 @@ public function compileUpdate(Builder $query, $values)
return trim("update {$table}{$joins} set $columns $where");
}

/**
* Prepare the bindings for an update statement.
*
* @param array $bindings
* @param array $values
* @return array
*/
public function prepareBindingsForUpdate(array $bindings, array $values)
{
$bindingsWithoutJoin = Arr::except($bindings, 'join');

$preparedBindings = array_values(
array_merge($values, $bindings['join'], Arr::flatten($bindingsWithoutJoin))
);

return $preparedBindings;
}

/**
* Wrap a table in keyword identifiers.
*
Expand Down
56 changes: 56 additions & 0 deletions tests/Database/DatabaseQueryBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1253,6 +1253,14 @@ public function testUpdateMethodWithJoins()
$builder->getConnection()->shouldReceive('update')->once()->with('update "users" inner join "orders" on "users"."id" = "orders"."user_id" set "email" = ?, "name" = ? where "users"."id" = ?', ['foo', 'bar', 1])->andReturn(1);
$result = $builder->from('users')->join('orders', 'users.id', '=', 'orders.user_id')->where('users.id', '=', 1)->update(['email' => 'foo', 'name' => 'bar']);
$this->assertEquals(1, $result);

$builder = $this->getBuilder();
$builder->getConnection()->shouldReceive('update')->once()->with('update "users" inner join "orders" on "users"."id" = "orders"."user_id" and "users"."id" = ? set "email" = ?, "name" = ?', [1, 'foo', 'bar'])->andReturn(1);
$result = $builder->from('users')->join('orders', function ($join) {
$join->on('users.id', '=', 'orders.user_id')
->where('users.id', '=', 1);
})->update(['email' => 'foo', 'name' => 'bar']);
$this->assertEquals(1, $result);
}

public function testUpdateMethodWithJoinsOnSqlServer()
Expand All @@ -1261,6 +1269,46 @@ public function testUpdateMethodWithJoinsOnSqlServer()
$builder->getConnection()->shouldReceive('update')->once()->with('update [users] set [email] = ?, [name] = ? from [users] inner join [orders] on [users].[id] = [orders].[user_id] where [users].[id] = ?', ['foo', 'bar', 1])->andReturn(1);
$result = $builder->from('users')->join('orders', 'users.id', '=', 'orders.user_id')->where('users.id', '=', 1)->update(['email' => 'foo', 'name' => 'bar']);
$this->assertEquals(1, $result);

$builder = $this->getSqlServerBuilder();
$builder->getConnection()->shouldReceive('update')->once()->with('update [users] set [email] = ?, [name] = ? from [users] inner join [orders] on [users].[id] = [orders].[user_id] and [users].[id] = ?', ['foo', 'bar', 1])->andReturn(1);
$result = $builder->from('users')->join('orders', function ($join) {
$join->on('users.id', '=', 'orders.user_id')
->where('users.id', '=', 1);
})->update(['email' => 'foo', 'name' => 'bar']);
$this->assertEquals(1, $result);
}

public function testUpdateMethodWithJoinsOnMySql()
{
$builder = $this->getMySqlBuilder();
$builder->getConnection()->shouldReceive('update')->once()->with('update `users` inner join `orders` on `users`.`id` = `orders`.`user_id` set `email` = ?, `name` = ? where `users`.`id` = ?', ['foo', 'bar', 1])->andReturn(1);
$result = $builder->from('users')->join('orders', 'users.id', '=', 'orders.user_id')->where('users.id', '=', 1)->update(['email' => 'foo', 'name' => 'bar']);
$this->assertEquals(1, $result);

$builder = $this->getMySqlBuilder();
$builder->getConnection()->shouldReceive('update')->once()->with('update `users` inner join `orders` on `users`.`id` = `orders`.`user_id` and `users`.`id` = ? set `email` = ?, `name` = ?', [1, 'foo', 'bar'])->andReturn(1);
$result = $builder->from('users')->join('orders', function ($join) {
$join->on('users.id', '=', 'orders.user_id')
->where('users.id', '=', 1);
})->update(['email' => 'foo', 'name' => 'bar']);
$this->assertEquals(1, $result);
}

public function testUpdateMethodWithJoinsOnSQLite()
{
$builder = $this->getSQLiteBuilder();
$builder->getConnection()->shouldReceive('update')->once()->with('update "users" inner join "orders" on "users"."id" = "orders"."user_id" set "email" = ?, "name" = ? where "users"."id" = ?', ['foo', 'bar', 1])->andReturn(1);
$result = $builder->from('users')->join('orders', 'users.id', '=', 'orders.user_id')->where('users.id', '=', 1)->update(['email' => 'foo', 'name' => 'bar']);
$this->assertEquals(1, $result);

$builder = $this->getSQLiteBuilder();
$builder->getConnection()->shouldReceive('update')->once()->with('update "users" inner join "orders" on "users"."id" = "orders"."user_id" and "users"."id" = ? set "email" = ?, "name" = ?', [1, 'foo', 'bar'])->andReturn(1);
$result = $builder->from('users')->join('orders', function ($join) {
$join->on('users.id', '=', 'orders.user_id')
->where('users.id', '=', 1);
})->update(['email' => 'foo', 'name' => 'bar']);
$this->assertEquals(1, $result);
}

public function testUpdateMethodWithJoinsAndAliasesOnSqlServer()
Expand All @@ -1285,6 +1333,14 @@ public function testUpdateMethodWithJoinsOnPostgres()
$builder->getConnection()->shouldReceive('update')->once()->with('update "users" set "email" = ?, "name" = ? from "orders" where "users"."id" = ? and "users"."id" = "orders"."user_id"', ['foo', 'bar', 1])->andReturn(1);
$result = $builder->from('users')->join('orders', 'users.id', '=', 'orders.user_id')->where('users.id', '=', 1)->update(['email' => 'foo', 'name' => 'bar']);
$this->assertEquals(1, $result);

$builder = $this->getPostgresBuilder();
$builder->getConnection()->shouldReceive('update')->once()->with('update "users" set "email" = ?, "name" = ? from "orders" where "users"."id" = "orders"."user_id" and "users"."id" = ?', ['foo', 'bar', 1])->andReturn(1);
$result = $builder->from('users')->join('orders', function ($join) {
$join->on('users.id', '=', 'orders.user_id')
->where('users.id', '=', 1);
})->update(['email' => 'foo', 'name' => 'bar']);
$this->assertEquals(1, $result);
}

public function testUpdateMethodRespectsRaw()
Expand Down