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

add touch method to model #488

Open
wants to merge 2 commits into
base: 1.1-dev
Choose a base branch
from

Conversation

koenpunt
Copy link
Collaborator

Work to implement suggested feature: #486

@daniel-aranda
Copy link

Thanks for adding this. Please add tests. My two cents with a skeleton of tests for the change:

    public function test_touch(){

        $author = new Author();
        $author->save();
        $updated_at = $author->updated_at;
        sleep(1);

        $author->touch();

        $this->assert_not_null($author->updated_at);
        $this->assert_not_equals($updated_at, $author->updated_at);
        $this->assert_greater_than($updated_at, $author->updated_at);

    }

@koenpunt
Copy link
Collaborator Author

koenpunt commented Feb 5, 2015

It's not for nothing I labeled this PR "work in progress" ;)

@koenpunt
Copy link
Collaborator Author

koenpunt commented Feb 5, 2015

And I'm not a fan of using sleep in tests.

It's probably faster to set the date explicitly (untested):

$author = new Author();
$updated_at = $author->updated_at = new DateTime('yesterday');
// NB. not sure if updated_at is also updated when it has been modified manually.
$author->save();

$author->touch();

$this->assertGreaterThan($updated_at, $author->updated_at);

But like I said, there still is some work to do.

@daniel-aranda
Copy link

Cool, just wanted to collaborate. I'm also not a fan of using Sleep, I agree with that. Just wanted to piggy back to existing tests. But definitely my vote is also to the Datetime set. Thanks.

@koenpunt koenpunt force-pushed the 1.1-dev branch 2 times, most recently from 494d55e to 352e1b6 Compare June 3, 2017 14:18
@koenpunt koenpunt force-pushed the model-touch branch 2 times, most recently from 307db60 to 68cf006 Compare June 13, 2017 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants