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.2] Fix issue #11815 #13519

Merged
merged 2 commits into from
May 18, 2016
Merged

[5.2] Fix issue #11815 #13519

merged 2 commits into from
May 18, 2016

Conversation

samrap
Copy link

@samrap samrap commented May 11, 2016

Timestamps are not prefixed when updating many-to-many relationship

This issue is caused by the Eloquent Builder's addUpdatedAtColumn method, which
does not account for relationships when retrieving the "updated at" column. Unlike
the SoftDeletes trait, which knows to get the qualified "deleted at" column
when deleting relationships, the Eloquent Builder's addUpdatedAtColumn method
simply calls the model's getUpdatedAtColumn method, which returns the static
constant 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 new
getQualifiedUpdatedAtColumn, which simply concatenates the model's table name
with 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. If
the 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 from table.updated_at
to [table => updated_at], breaking the subsequent query.There are two solutions
I found to this issue. The first is to modify the Query Builder update method to
call Arr::dot on the on the values passed to the method. This is not ideal, as
it reverses the Arr::set method which causes an unnecessary latency in time and
could also have unforseen affects. The second solution goes back to the Eloquent
Builder update method. Instead of using the Arr::add method to add the
"updated at" column, we will perform a simple array_merge, which does exactly
what the Arr::add method did when updating, but doesn't break the merge when
adding a relationship to the "updated at" column.

When using the Arr::add method: update buginner joinbug_laravelonbug.id=bug_laravel.bug_idsetbug.deleted_at= ?,bug= ? wherebug_laravel.laravel_id= ? andbug.deleted_at is null
When using array_merge function: update buginner joinbug_laravelonbug.id=bug_laravel.bug_idsetbug.deleted_at= ?,bug.updated_at= ? wherebug_laravel.laravel_id= ? andbug.deleted_at is null

The first query fails, because the values returned from Arr::add are as follows:

array:2 [
  "bug.deleted_at" => "2016-05-11 18:08:12"
  "bug" => array:1 [
    "updated_at" => "2016-05-11 18:08:12"
  ]
]

But, using array_merge:

array:2 [
  "course.deleted_at" => "2016-05-11 18:09:39"
  "course.updated_at" => "2016-05-11 18:09:39"
]

This fix passes all tests, and has been tested on a working application I am currently developing.


Closes #11815.

@GrahamCampbell
Copy link
Member

Could you add a couple of tests to cover this please?

@samrap
Copy link
Author

samrap commented May 11, 2016

Sure, I'm not familiar with the extent of the framework's testing, so this might take a while.

@GrahamCampbell GrahamCampbell changed the title Fix issue [#11815](https://github.com/laravel/framework/issues/11815) [5.1] Fix issue #11815 May 11, 2016
@GrahamCampbell GrahamCampbell changed the title [5.1] Fix issue #11815 [5.2] Fix issue #11815 May 11, 2016
@samrap
Copy link
Author

samrap commented May 12, 2016

Been busy at work, finishing those tests is still a top priority for me, looking into it tonight.

@taylorotwell taylorotwell merged commit b015a5b into laravel:5.2 May 18, 2016
@tillkruss
Copy link
Collaborator

Any updates on the tests?

@taylorotwell
Copy link
Member

Breaks things: #13707

Reverted. :(

@samrap
Copy link
Author

samrap commented May 27, 2016

@taylorotwell bummer. Will look further.

@taylorotwell
Copy link
Member

The thread I linked has some details. The OP of that might be able to give more insight.

@samrap
Copy link
Author

samrap commented May 27, 2016

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 ;)

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.

4 participants