Skip to content

Conversation

@themsaid
Copy link
Member

This PR fixes the issue reported in #19620, the breaking change was introduced in #19538

This PR doesn't revert, but only updates the updated_at column if the Model's timestamps = true.

@taylorotwell taylorotwell merged commit 8e8b562 into laravel:5.4 Jun 15, 2017
mathieutu added a commit to mathieutu/framework that referenced this pull request Jun 19, 2017
* commit '05060b183dd09cee6dce498da5afde9e66fd1a29':
  Apply fixes from StyleCI (laravel#19659)
  [5.4] Add ability to remove a global scope with another global scope (laravel#19657)
  [5.4] Add fresh method on Eloquent\Collection. (laravel#19616)
  support multiple fields to validateDifferent (laravel#19637)
  escape default value of yield blade directive (laravel#19643)
  [5.4] Add "avg" and "average" to higher order proxy. (laravel#19628)
  tagged v5.4.27 release notes
  version
  Add missing dependency. (laravel#19623)
  update timestamp on soft delete only when its used (laravel#19627)
  Add new `diffAssoc()` method to collection (laravel#19604)
  Fix tests code coverage config. (laravel#19609)

# Conflicts:
#	src/Illuminate/Database/Eloquent/Collection.php
@kamui545
Copy link
Contributor

kamui545 commented Jul 5, 2017

Hello Mohamed,

Thank you for the fix.

I would like to point out that it will still break for people having the following kind of models:

class Example extends Model
{
    public $timestamps = true;
    const CREATED_AT = 'created_at';
    const UPDATED_AT = null; // Disable `updated_at` field
}

I'm pretty sure this use case is not recommended and then it will probably be a wont-fix, but I just wanted to make sure you are aware of it.

Thanks!

@bobbybouwmann
Copy link
Contributor

@kamui545 If you don't want the updated_at field to be updated you don't need the field at all right? Instead you can only add a created_at field ;)

@kamui545
Copy link
Contributor

kamui545 commented Jul 7, 2017

@bobbybouwmann This is exactly the use case I am explaining, having no updated_at field in the table.

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