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.8] Fix ambiguous UPDATED_AT columns #26031

Merged
merged 2 commits into from
Oct 10, 2018
Merged

[5.8] Fix ambiguous UPDATED_AT columns #26031

merged 2 commits into from
Oct 10, 2018

Conversation

staudenmeir
Copy link
Contributor

@staudenmeir staudenmeir commented Oct 10, 2018

#13519 attempted to fix ambiguous UPDATED_AT columns in JOIN queries:

class Country extends Model {
    public function posts() {
        return $this->hasManyThrough(Post::class, User::class);
    }
}

$country->posts()->update(['foo' => 'bar']);
update `posts` inner join `users` on `users`.`id` = `posts`.`user_id`
set `foo` = bar, `updated_at` = 2018-10-10 01:56:14
where `users`.`country_id` = 1

If both tables have an updated_at column, you get an "ambiguous column name" error (on MySQL).

To fix this, the previous PR qualified the column with the table name. This didn't work because PostgreSQL and SQLite don't allow qualified columns in UPDATE queries (#13707).

#22366 updated the SQLiteGrammar to remove the table name from qualified columns when an UPDATE query is compiled. This PR applies the same behavior to the PostgresGrammar.

We can now safely qualify the column on all platforms. We have to replace Arr::add() with array_merge() because the former interprets a qualified column as a nested key. $values is the second argument so that the users can override the updated_at value (as they can now).

Fixes #13909.

@samrap
Copy link

samrap commented Oct 23, 2018

Great work! 😁

@jimmypuckett
Copy link
Contributor

@driesvints Is there anyway to get this merged into 5.7? I understand that because 5.5 is LTS it gets fixed, but it would be nice to let the current stable version get the fix too?

@mfn
Copy link
Contributor

mfn commented Dec 3, 2018

I'm afraid it's not possible, as the change to compileUpdateColumns would be a breaking change in the middle of a release cycle.

@jimmypuckett
Copy link
Contributor

I'm afraid it's not possible, as the change to compileUpdateColumns would be a breaking change in the middle of a release cycle.

@mfn Thanks for the feedback. It is interesting that 5.5 was able to be patched without breaking it, but that the current stable 5.7 cannot be patched.

I appreciate you taking the time to answer.

@staudenmeir
Copy link
Contributor Author

@jimmypuckett Are you referring to #26022? We had to revert it in #26024.

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.

5 participants