-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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.2] Fix issue #11815 #13519
[5.2] Fix issue #11815 #13519
Conversation
Could you add a couple of tests to cover this please? |
Sure, I'm not familiar with the extent of the framework's testing, so this might take a while. |
Been busy at work, finishing those tests is still a top priority for me, looking into it tonight. |
Any updates on the tests? |
Breaks things: #13707 Reverted. :( |
@taylorotwell bummer. Will look further. |
The thread I linked has some details. The OP of that might be able to give more insight. |
Got it. It's pretty apparent what's going on. Just need to find a way to use the qualified column on the join part of the query only. Easier said than done ;) |
Timestamps are not prefixed when updating many-to-many relationship
This issue is caused by the Eloquent Builder's
addUpdatedAtColumn
method, whichdoes not account for relationships when retrieving the "updated at" column. Unlike
the
SoftDeletes
trait, which knows to get the qualified "deleted at" columnwhen deleting relationships, the Eloquent Builder's
addUpdatedAtColumn
methodsimply calls the model's
getUpdatedAtColumn
method, which returns the staticconstant who's value is "updated_at".
The fix for this is implementing the same methods for deleting relationships when
updated them. The
Illuminate\Database\Eloquent\Model
class receives a newgetQualifiedUpdatedAtColumn
, which simply concatenates the model's table namewith the model's "updated at" column name. Meanwhile, the
Illuminate\Database\Eloquent\Builder
addUpdatedAtColumn
method checks to see if the current query has any joins. Ifthe query does have joins, it will call the model's
getQualifiedUpdatedAtColumn
,otherwise it will do what it has always done, call the
getUpdatedAtColumn
method.This finally gives us the proper "updated at" query whether we have joins or not.
However, the method returns a merged array using the
Illuminate\Support\Arr
class, which merges the existing values with the new "updated at" column, but
due to the nature of the
Arr::set
method, breaks the join query fromtable.updated_at
to
[table => updated_at]
, breaking the subsequent query.There are two solutionsI found to this issue. The first is to modify the Query Builder
update
method tocall
Arr::dot
on the on the values passed to the method. This is not ideal, asit reverses the
Arr::set
method which causes an unnecessary latency in time andcould also have unforseen affects. The second solution goes back to the Eloquent
Builder
update
method. Instead of using theArr::add
method to add the"updated at" column, we will perform a simple
array_merge
, which does exactlywhat the
Arr::add
method did when updating, but doesn't break the merge whenadding a relationship to the "updated at" column.
When using the
Arr::add
method:update
buginner join
bug_laravelon
bug.
id=
bug_laravel.
bug_idset
bug.
deleted_at= ?,
bug= ? where
bug_laravel.
laravel_id= ? and
bug.
deleted_atis null
When using
array_merge
function:update
buginner join
bug_laravelon
bug.
id=
bug_laravel.
bug_idset
bug.
deleted_at= ?,
bug.
updated_at= ? where
bug_laravel.
laravel_id= ? and
bug.
deleted_atis null
The first query fails, because the values returned from
Arr::add
are as follows:But, using
array_merge
:This fix passes all tests, and has been tested on a working application I am currently developing.
Closes #11815.