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.8] Add deleted_at date cast for soft deleting models #26985

Closed
wants to merge 1 commit into from
Closed

[5.8] Add deleted_at date cast for soft deleting models #26985

wants to merge 1 commit into from

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Dec 29, 2018

This PR utilises a trait initializer to automatically add deleted_at to the $dates array of any model that uses the SoftDeletes trait. Previously this has to be added manually to each model - unlike the created_at and updated_at that is handled "under the hood".

In essence...

class Post extends Eloquent
{
    use SoftDeletes;

    // no need for this anymore
    // protected $dates = ['deleted_at'];
}

Notes:

  • An existing$casts entry for deleted_at will take precedence (test exists).
  • A duplicate $dates entry for deleted_at will have no impact as the array is not a key => value pair, but a value array. Also when getDates is called it already runs array_unique to remove any duplicate entries (test exists).
  • An existing mutator getDeletedAtAttribute will take precedence (test exists).

Breaking:

This will be a breaking change if you are not expecting the value of deleted_at to be cast, i.e. you never added it to the $dates array as outlined in the docs. But as mentioned earlier, a mutator will take precedence if for whatever reason you don't want the automatic casting.

@taylorotwell
Copy link
Member

Is there any path for opting out of the cast?

@timacdonald
Copy link
Member Author

timacdonald commented Jan 2, 2019

Just added a new test to confirm the current behaviour can be maintained by adding a cast.

This is the easiest opt-out and the one I see being in the upgrade guide if this PR was to be accepted. A boolean toggle seemed like overkill as the cast is a one-liner.

protected $casts = [
    'deleted_at' => 'string', // retain current functionality
]; 

It is also possible to opt-out with an attribute mutator - but obviously that is more than a one liner.

The order of precedence is:

  1. Mutator
  2. $casts
  3. $dates

If you feel a boolean toggle opt-out attribute would be the best way I will happily add that.

Edit: I think there might be some other cases I need to consider that the cast to string might not handle and perhaps a Boolean toggle to opt out would be best. Will take a look at them tomorrow.

@timacdonald
Copy link
Member Author

timacdonald commented Jan 4, 2019

After sitting on this for the day I think the boolean flag is the better overall approach. I've implemented it in a seperate commit, so you can roll it back if you prefer opting out via a cast to string as mentioned above.

With the current PR you can opt-out by setting the boolean flag on the model (test exists)...

class Post extends Eloquent
{
    // opt-out of the automatic casting...
    protected $castDeletedAtToDateInstance = false;
}

@driesvints
Copy link
Member

Not that I have a better alternative but the current property name looks super confusing and long..

@taylorotwell
Copy link
Member

Eh, I think I prefer the string cast to the property.

@timacdonald
Copy link
Member Author

Just reverted and removed the boolean flag. However Travis is said to have failed - but as I'm looking at it right now it actually hasn't run the tests. Not sure if you can whack it with a big spanner to get it to run again?

@taylorotwell
Copy link
Member

Merged.

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.

3 participants