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] Add dynamic trashed factory state #42414

Merged
merged 3 commits into from
May 18, 2022

Conversation

jasonmccreary
Copy link
Contributor

This adds dynamic support of a trashed factory state for models using the SoftDeletes trait. It has an optional argument of the date value. If not passed, the default is yesterday.

Before

// in Factory class
public function trashed()
{
    return $this->state([
        'deleted_at' => now()->subDay(),
    ]);
}

If this is merged, you would no longer need to define such a state in factories for SoftDeletes models.

This was implemented within __call. As such, this should be fully backward compatible if a trashed method already exists. A test case also demonstrates this.

@lukeraymonddowning
Copy link
Contributor

Gorgeous stuff 👌 I would just say set the deleted at default to be now() as subDay might cause confusion.

@jasonmccreary
Copy link
Contributor Author

Not sure about the failing tests. All the database test cases passed for me locally (PHP 8.1).

@driesvints driesvints changed the title Add dynamic trashed factory state [9.x] Add dynamic trashed factory state May 17, 2022
@jasonmccreary
Copy link
Contributor Author

@lukeraymonddowning, the only reason for a "past" date would be to avoid any foot-gun date comparisons in tests since they run so quickly. For example:

$model = Model::factory()->trashed()->create();
$this->assertTrue($model->deleted_at->isPast())

Open to changing it. But that was my reasoning.

@lukeraymonddowning
Copy link
Contributor

@jasonmccreary good point 👍

@taylorotwell
Copy link
Member

taylorotwell commented May 17, 2022

I'm getting one test failure and one error locally on PHP 8.1. I do not see any failures or errors on current 9.x branch.

@taylorotwell taylorotwell marked this pull request as draft May 17, 2022 14:42
@jasonmccreary
Copy link
Contributor Author

@taylorotwell, unfortunately, I don't get these failures locally on PHP 8 or PHP 8.1.

@driesvints
Copy link
Member

@jasonmccreary retriggered the tests for ya. Might be a hiccup.

@jasonmccreary
Copy link
Contributor Author

@driesvints, actually, seems like I do get the failure when I run the entire test suite. Strange, as when it's run independently it passes. Maybe some kind of test pollution. I'll take a closer look.

@jasonmccreary
Copy link
Contributor Author

jasonmccreary commented May 17, 2022

These same tests fail even when I remove the new code. I rebased, just in case there was an error on 9.x, but no luck either. 🤷

I'm at a bit of a loss. I'll look more closely this evening.

@DarkGhostHunter
Copy link
Contributor

I think the deleted_at timestamp should be not minor than the created_at timestamp, or it would introduce some edge cases when a comparing both.

$timeOnline = $trashed->created_at->diff($trashed->deleted_at);

It could be fixed by only using now().

// in Factory class
public function trashed()
{
    return $this->state([
        'deleted_at' => now()
    ]);
}

@jasonmccreary
Copy link
Contributor Author

@DarkGhostHunter, I'm open to changing it. Keep in mind, this does support passing in the $deleted_at timestamp.

@jasonmccreary
Copy link
Contributor Author

@taylorotwell + @driesvints, it was test pollution. Carbon::setTestNow() needed to be reset. I assumed a tearDown method did this somewhere.

Everything is passing now, so I'm marking this for review. Let me know what you think about the default timestamp (now versus yesterday). Glad to make any final changes.

@jasonmccreary jasonmccreary marked this pull request as ready for review May 17, 2022 20:19
@DarkGhostHunter
Copy link
Contributor

Does it handles intervals? For example, I need to compute messages that have been "online" for more than a minute.

This way I can warn the user that deleting it may not remove it from the internerks.

@taylorotwell taylorotwell merged commit bc4f436 into laravel:9.x May 18, 2022
@jasonmccreary jasonmccreary deleted the auto-trashed-state branch May 24, 2022 15:22
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.

5 participants