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

[9.x] Accept attribute to touch #36157

Merged
merged 2 commits into from
Feb 8, 2021
Merged

[9.x] Accept attribute to touch #36157

merged 2 commits into from
Feb 8, 2021

Conversation

freekmurze
Copy link
Contributor

With the changes in this PR you can replace this code...

$this->update(['attribute' => now()]);

... with ...

$this->touch('attribute');

Which feels a tad nicer.

I didn't find any test around the normal behaviour of touch so I didn't add any.

If you want to accept this PR, but want tests, point me where these should be created.

{
if ($attribute) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would it happen if a given model does not use timestamp? - wouldn't the guard ! $this->usesTimestamps() be necessary for this use case too?

Copy link
Member

@crynobone crynobone Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be covered by Line 30. This line covers $model->touch('custom_attribute'); and should affect usesTimestamps().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usesTimestamps() only tells you if the Model is using the standard updated_at and created_at, so it should not be checked when $attribute is passed to the touch method.

It is valid to disable the updated/created timestamps (via $timestamps = false) but still use your own custom timestamp column for some other use case, which is when the $attribute argument would be used.

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.

6 participants