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
base: 5.1
from

Conversation

Projects
None yet
4 participants
@acasar
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@JosephSilber

JosephSilber Feb 21, 2016

Contributor

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

Contributor

JosephSilber commented Feb 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Feb 21, 2016

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.

Member

taylorotwell commented Feb 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@acasar

acasar Feb 21, 2016

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Feb 21, 2016

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?

Member

taylorotwell commented Feb 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@vlakoff

vlakoff Feb 22, 2016

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@acasar

acasar Feb 22, 2016

Contributor

@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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Mar 1, 2016

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.

Member

taylorotwell commented Mar 1, 2016

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