Skip to content

Fixed issue #403 where a DateTime::createFromFormat('U.u', $microtime) w...#406

Merged
ryasmi merged 2 commits intoLearningLocker:masterfrom
pondermatic:403
Oct 9, 2014
Merged

Fixed issue #403 where a DateTime::createFromFormat('U.u', $microtime) w...#406
ryasmi merged 2 commits intoLearningLocker:masterfrom
pondermatic:403

Conversation

@pondermatic
Copy link
Copy Markdown
Contributor

...ould be FALSE if microtime's subseconds were 0.

…u', $microtime) would be FALSE if microtime's subseconds were 0.
@ryasmi
Copy link
Copy Markdown
Contributor

ryasmi commented Oct 8, 2014

#403 @pondermatic thanks for the pull request! Is this only done in those two files and have you tested that this works under both normal and previously erroneous conditions. If you have tested this please describe how so that these tests can be replicated in the future and for reference. Once that's done I'll merge it and get this in for 1.1.2 which I hope to release on Friday 😄

@ryasmi
Copy link
Copy Markdown
Contributor

ryasmi commented Oct 8, 2014

@pondermatic this appears to fail two tests. You can run these tests yourself in the terminal by running ./vendor/bin/phpunit. Please see the Travis build where the tests were ran to get a stack trace of what failed.

@pondermatic
Copy link
Copy Markdown
Contributor Author

I searched and found that these two were the only places that microtime is converted to a DateTime object.

To be honest, I am not currently set up to run learning locker on my development machine due to a recent rebuild. I tested this code to determine the fix:

while (TRUE) {
    $time = microtime(TRUE);
    if ((int)$time == $time) {
        var_dump(DateTime::createFromFormat('U.u', sprintf('%.4f', $time)));
    }
}

I will try to get set up today to run unit tests.

@ryasmi
Copy link
Copy Markdown
Contributor

ryasmi commented Oct 8, 2014

I searched and found that these two were the only places that microtime is converted to a DateTime object.

Brilliant thanks 👍

To be honest, I am not currently set up to run learning locker on my development machine due to a recent rebuild. I tested this code to determine the fix:

Ok. Nice that seems fair enough to me.

I will try to get set up today to run unit tests.

Ok that's great! Keep me posted on your progress via this pull request if you can 😄 Thanks for taking the time to make a pull request for this issue.

@ryasmi ryasmi added this to the 1.1.2 milestone Oct 8, 2014
…teTime.

Changed strftime() to sprintf(). Brain fart.
@pondermatic
Copy link
Copy Markdown
Contributor Author

I used the correct command, sprintf, in my test code, but for some unknown idiotic reason used strftime in the code that I committed.

Thanks for your help and your patience. I haven't contributed to LL since June, which was before Travis CI. Very slick!

@ryasmi
Copy link
Copy Markdown
Contributor

ryasmi commented Oct 9, 2014

Haha brilliant! I'm glad it was something simple. Thanks for updating your pull request.

No worries, you're very welcome. Thanks for the pull request. Ah right, yeah we think so too, a lot of work has gone into improving the workflow for contributors. It's great to hear your feedback.

ryasmi added a commit that referenced this pull request Oct 9, 2014
@ryasmi ryasmi merged commit 40f509f into LearningLocker:master Oct 9, 2014
@pondermatic pondermatic deleted the 403 branch October 9, 2014 20:23
@pondermatic pondermatic restored the 403 branch October 9, 2014 20:51
@pondermatic pondermatic deleted the 403 branch October 9, 2014 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants