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.

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

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

Merged
merged 1 commit into from
Nov 14, 2016

Conversation

litvinchuk
Copy link
Contributor

solution for issue #16367, #16363

@taylorotwell
Copy link
Member

Needs full description of how fix addresses problem. Need for passing into this overriden prepareBindingsForUpdate method, etc.

@litvinchuk
Copy link
Contributor Author

litvinchuk commented Nov 11, 2016

For different grammars, we need a different order for the bindings and values.

Example Mysql:
update users inner join orders on users.id = orders.user_id and users.id = ? set email = ?, name = ?
We need first set join parameters and then values

Example Postgres:
update "users" set "email" = ?, "name" = ? from "orders" where "users"."id" = "orders"."user_id" and "users"."id" = ?
We need first set values and then join parameters

That's why we need merge bindings and values in different way for each grammar

@themsaid
Copy link
Member

@litvinchuk thank you, looks like the syntax for Postgres and SQL Server varies from MySQL, closing my other PR so that we can work on this one.

@@ -166,18 +166,14 @@ protected function compileJsonUpdateColumn($key, JsonExpression $value)
*/
public function prepareBindingsForUpdate(array $bindings, array $values)
{
$index = 0;
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

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.

foreach ($values as $column => $value) {
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

@taylorotwell taylorotwell merged commit 83d6fb6 into laravel:5.3 Nov 14, 2016
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.

3 participants