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.1] Fix timestamps on update #12403

Closed
wants to merge 2 commits into from
Closed

[5.1] Fix timestamps on update #12403

wants to merge 2 commits into from

Conversation

acasar
Copy link
Contributor

@acasar acasar commented Feb 21, 2016

This fixes #10028 where timestamps are updated even if ['timestamps' => false] is passed to the save() method.

The easiest solution would be to simply pass the $options array to the update method on the Builder but since that would be a breaking change (someone may be extending the Builder) I added a new flag $shouldUpdateTimestamps that is set to false if timestamps were disabled.

@JosephSilber
Copy link
Member

Since we're not updating timestamps if $timestamps is set to false on the model, how about inverting that property and calling it $skipUpdatingTimestamp?

@taylorotwell
Copy link
Member

To me it feels really, really silly to have this new property that we are setting to true / false at random times. I don't know, something about this all feels really hacky.

@acasar
Copy link
Contributor Author

acasar commented Feb 21, 2016

Yeah it is a bit hacky but I don't have any other idea right now on how to fix it without breaking BC. Let me look at this a bit more tomorrow. Maybe I can find some better alternative.

@taylorotwell
Copy link
Member

I mean just off the top of my head. What if something in a model updated event updates a different field on the model and does want the timestamps updated?

@vlakoff
Copy link
Contributor

vlakoff commented Feb 22, 2016

As a reminder, DatabaseEloquentModelTest::testTimestampsAreNotUpdatedWithTimestampsFalseSaveOption isn't actually testing the functionality.

That's a good example of what could happen when a test has to hook mocks into the implementation of what is being tested.

@acasar
Copy link
Contributor Author

acasar commented Feb 22, 2016

@taylorotwell I found a better solution. I added a method withoutTimestamps() to the Builder which fixes the issue. This solution has additional benefits - it can be used directly when updating multiple records:

User::withoutTimestamps()->update(['active' => 1]);

@vlakoff Yeah mocking Eloquent is never a good idea. That's why I added a proper integration test for this issue.

@taylorotwell
Copy link
Member

I don't even really want to support this at all. If you want to update a DB record without updating the timestamps use the query builder. This will lead to all kinds of more code to support weird edge cases where I want to not update the timestamps on the main model but I want to touch or not touch the parent models, etc. I don't even want to go down this rabbit hole.

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