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

Conversation

Projects
None yet
3 participants
@timacdonald
Copy link
Contributor

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.

@timacdonald timacdonald force-pushed the timacdonald:add_deleted_at_for_soft_deletes branch from dd00759 to 84061c1 Dec 29, 2018

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

Is there any path for opting out of the cast?

@timacdonald timacdonald force-pushed the timacdonald:add_deleted_at_for_soft_deletes branch from 84061c1 to 545baee Jan 2, 2019

@timacdonald

This comment has been minimized.

Copy link
Contributor Author

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 timacdonald force-pushed the timacdonald:add_deleted_at_for_soft_deletes branch from 545baee to eec7401 Jan 2, 2019

@antonkomarev antonkomarev referenced this pull request Jan 3, 2019

Closed

Auto-cast flag values #43

@timacdonald

This comment has been minimized.

Copy link
Contributor Author

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;
}

@timacdonald timacdonald force-pushed the timacdonald:add_deleted_at_for_soft_deletes branch from cd721a2 to b8d4869 Jan 4, 2019

@driesvints

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

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

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

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

@timacdonald timacdonald force-pushed the timacdonald:add_deleted_at_for_soft_deletes branch from 5c206d6 to b8d4869 Jan 5, 2019

@timacdonald

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.