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

Refresh updated_at timestamp on soft delete #19538

Merged

Conversation

haakym
Copy link
Contributor

@haakym haakym commented Jun 9, 2017

For issue #19529

Performing a soft delete updates the updated_at timestamp in the database but not on the model. Assumed the fix would be in line with the desired behaviour?

The changes to SoftDeletes makes the testDeleteSetsSoftDeletedColumn method on DatabaseSoftDeletingTraitTest fail, have written the code to get it passing again, but unsure if my thinking is in the right direction for the test.

Would appreciate any feedback as still pretty new to contributing. Thanks!

@taylorotwell taylorotwell merged commit a1395f2 into laravel:5.4 Jun 9, 2017
@Namoshek
Copy link
Contributor

Namoshek commented Jun 9, 2017

Thank you for the fix! Unfortunately, I think your change forces an updated_at attribute on the model, even if public $timestamps = false; is set on it. I doubt there are many models with SoftDeletes enabled while not having normal timestamps. But to be sure it always works, something along the lines

protected function runSoftDelete()
{
    $query = $this->newQueryWithoutScopes()->where($this->getKeyName(), $this->getKey());

    $time = $this->freshTimestamp();

    $this->{$this->getDeletedAtColumn()} = $time;
    $attributes = [$this->getDeletedAtColumn() => $this->fromDateTime($time)];

    // first check of the if is pretty much optional but a safe bet
    if ($this instanceof Concerns\HasAttributes && $this->timestamps === true) {
        $this->{$this->getUpdatedAtColumn()} = $time;
        $attributes[$this->getUpdatedAtColumn()] = $this->fromDateTime($time);
    }

    $query->update($attributes);
}

seems more realistic to me. I'm also only a new Laravel contributor though and still trying to understand everything behind the scenes... :)

For full coverage, this obviously requires two tests thanks to the if statement.

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