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

Time travel #1

Closed
wants to merge 12 commits into
base: master
from

Conversation

2 participants
@davedevelopment

davedevelopment commented Sep 12, 2012

What do you think to this? I imagine there's some perculiar concoctions of the date strings that might not work as expected with the time travel, but I think it's a reasonable start. As a precaution, the constructor has been short-circuited to behave as before if now time travelling is taking place.

@briannesbitt

This comment has been minimized.

Show comment
Hide comment
@briannesbitt

briannesbitt Sep 12, 2012

Owner

Hi Dave. I like where this is going! Maybe we can simply it though. Not sure it needs to provide multiple time travel offsets? Maybe also time travelling to a DateTime instance would simplify the code. ie. Carbon::timeTravelTo("+5 years"); would be Carbon::timeTravelTo(Carbon::now()->addYears(5));. Something like that. One of the goals is to be more explicit with dates so you don't see + 86,400 anywhere!

Owner

briannesbitt commented Sep 12, 2012

Hi Dave. I like where this is going! Maybe we can simply it though. Not sure it needs to provide multiple time travel offsets? Maybe also time travelling to a DateTime instance would simplify the code. ie. Carbon::timeTravelTo("+5 years"); would be Carbon::timeTravelTo(Carbon::now()->addYears(5));. Something like that. One of the goals is to be more explicit with dates so you don't see + 86,400 anywhere!

@davedevelopment

This comment has been minimized.

Show comment
Hide comment
@davedevelopment

davedevelopment Sep 12, 2012

@briannesbitt errm, not sure of a specific reason for time travelling twice, I just copied the delorean API! I guess an example would be if you wanted to travel to a specific date in a setup for all tests, then jump relative to that in individual tests?

Same for the parameter to timeTravelTo, I just copied the Delorean API. You can pass a string or a DateTime.

I can make these simplifications, but it would only save 4 or 5 lines of production code, e.g.

<?php
foreach (static::$timeTravelOffsets as $offset) {
    $this->addSeconds($offset);
}

// becomes

$this->addSeconds(static::$timeTravelOffset);

// and we could remove

if (!($time instanceof \DateTime)) {
    $time = static::now()->modify($time);
}

I must admit I prefer to see Carbon::timeTravelTo("+5 years"); to Carbon::timeTravelTo(Carbon::now()->addYears(5));, but it's your project, let me know what you want.

davedevelopment commented Sep 12, 2012

@briannesbitt errm, not sure of a specific reason for time travelling twice, I just copied the delorean API! I guess an example would be if you wanted to travel to a specific date in a setup for all tests, then jump relative to that in individual tests?

Same for the parameter to timeTravelTo, I just copied the Delorean API. You can pass a string or a DateTime.

I can make these simplifications, but it would only save 4 or 5 lines of production code, e.g.

<?php
foreach (static::$timeTravelOffsets as $offset) {
    $this->addSeconds($offset);
}

// becomes

$this->addSeconds(static::$timeTravelOffset);

// and we could remove

if (!($time instanceof \DateTime)) {
    $time = static::now()->modify($time);
}

I must admit I prefer to see Carbon::timeTravelTo("+5 years"); to Carbon::timeTravelTo(Carbon::now()->addYears(5));, but it's your project, let me know what you want.

@briannesbitt

This comment has been minimized.

Show comment
Hide comment
@briannesbitt

briannesbitt Sep 17, 2012

Owner

What confuses me about the current implementation is if I use Carbon::create(2012, 9, 16) I don't get time travelled. Ok. If I do new Carbon('2012-09-16') I don't get time travelled. Ok. If I do new Carbon('2012') I do get time travelled ?!? Why the difference? I see its because modify() resets the value back to 2012-09-16 but just not sure thats expected?

If thats right, to minimize confusion I think the implementation should only affect calls to now()... and today(), yesterday(), tomorrow() (I added those tonight) ?? 99% of the time I assume most are just trying to mock the call to ::now().

I think timeTravelTo() should take a Timezone parameter??

Owner

briannesbitt commented Sep 17, 2012

What confuses me about the current implementation is if I use Carbon::create(2012, 9, 16) I don't get time travelled. Ok. If I do new Carbon('2012-09-16') I don't get time travelled. Ok. If I do new Carbon('2012') I do get time travelled ?!? Why the difference? I see its because modify() resets the value back to 2012-09-16 but just not sure thats expected?

If thats right, to minimize confusion I think the implementation should only affect calls to now()... and today(), yesterday(), tomorrow() (I added those tonight) ?? 99% of the time I assume most are just trying to mock the call to ::now().

I think timeTravelTo() should take a Timezone parameter??

@briannesbitt

This comment has been minimized.

Show comment
Hide comment
@briannesbitt

briannesbitt Sep 17, 2012

Owner

Also the current implementation is failing. (because of timezones/daylight savings time)

public function testTimeTravelWithAbsolute()
{
    Carbon::timeTravelTo("1999-12-31");
    var_dump(Carbon::now()->toDateTimeString());
    $this->assertSame(1999, Carbon::now()->year);
}

I just ran this test and it fails. The var_dump shows 2000-01-01 00:13:51. This happened because the time travel is captured as the difference in timestamps (which occurs at UTC), but then the offset is added using addSeconds() which occurs in the timezone time. Since I am currently in daylight savings time and the time to travel to is not in DST the offset is off by an hour so I only make it back to 1999-12-31 + 1 hour. This is exaggerated and rolls over to the next day when you run it between 11 PM and midnight since the time portion of 1999-12-31 defaults to the current time.

Owner

briannesbitt commented Sep 17, 2012

Also the current implementation is failing. (because of timezones/daylight savings time)

public function testTimeTravelWithAbsolute()
{
    Carbon::timeTravelTo("1999-12-31");
    var_dump(Carbon::now()->toDateTimeString());
    $this->assertSame(1999, Carbon::now()->year);
}

I just ran this test and it fails. The var_dump shows 2000-01-01 00:13:51. This happened because the time travel is captured as the difference in timestamps (which occurs at UTC), but then the offset is added using addSeconds() which occurs in the timezone time. Since I am currently in daylight savings time and the time to travel to is not in DST the offset is off by an hour so I only make it back to 1999-12-31 + 1 hour. This is exaggerated and rolls over to the next day when you run it between 11 PM and midnight since the time portion of 1999-12-31 defaults to the current time.

@briannesbitt

This comment has been minimized.

Show comment
Hide comment
@briannesbitt

briannesbitt Sep 17, 2012

Owner

And you don't call parent::tearDown() when you override it in your Test class. :-)

Owner

briannesbitt commented Sep 17, 2012

And you don't call parent::tearDown() when you override it in your Test class. :-)

@davedevelopment

This comment has been minimized.

Show comment
Hide comment
@davedevelopment

davedevelopment Sep 17, 2012

I started out just mocking the now() method, but the problem is, I and I imagine most developers who are used to using DateTime, would carry on using Carbon in the same way. I also like the idea of carbon being a drop in replacement with a use Carbon\Carbon as DateTime. So when I thought I could make it work, I threw it in.

I'm not sure what you're expecting with new Carbon('2012'), but if I've time travelled to 1999, and I create a new Carbon with just a time, I'd expect to be on the date I'd travelled to, with the time I gave it?

<?php 

require "vendor/autoload.php";
var_dump((new Carbon\Carbon("2012")));
/**
class Carbon\Carbon#3 (3) {
  public $date =>
  string(19) "2012-09-17 20:12:00"
  public $timezone_type =>
  int(3)
  public $timezone =>
  string(13) "Europe/London"
}
*/
Carbon\Carbon::timeTravelTo("2012-01-01");
var_dump((new Carbon\Carbon("2012")));
/**
class Carbon\Carbon#3 (3) {
  public $date =>
  string(19) "2012-01-01 20:12:00"
  public $timezone_type =>
  int(3)
  public $timezone =>
  string(13) "Europe/London"
}
*/

Which makes sense to me? 

As for the timezone, not sure how to go about making that right, and it hurts my head :)

davedevelopment commented Sep 17, 2012

I started out just mocking the now() method, but the problem is, I and I imagine most developers who are used to using DateTime, would carry on using Carbon in the same way. I also like the idea of carbon being a drop in replacement with a use Carbon\Carbon as DateTime. So when I thought I could make it work, I threw it in.

I'm not sure what you're expecting with new Carbon('2012'), but if I've time travelled to 1999, and I create a new Carbon with just a time, I'd expect to be on the date I'd travelled to, with the time I gave it?

<?php 

require "vendor/autoload.php";
var_dump((new Carbon\Carbon("2012")));
/**
class Carbon\Carbon#3 (3) {
  public $date =>
  string(19) "2012-09-17 20:12:00"
  public $timezone_type =>
  int(3)
  public $timezone =>
  string(13) "Europe/London"
}
*/
Carbon\Carbon::timeTravelTo("2012-01-01");
var_dump((new Carbon\Carbon("2012")));
/**
class Carbon\Carbon#3 (3) {
  public $date =>
  string(19) "2012-01-01 20:12:00"
  public $timezone_type =>
  int(3)
  public $timezone =>
  string(13) "Europe/London"
}
*/

Which makes sense to me? 

As for the timezone, not sure how to go about making that right, and it hurts my head :)
@briannesbitt

This comment has been minimized.

Show comment
Hide comment
@briannesbitt

briannesbitt Sep 17, 2012

Owner

You do realize that the '2012' in that constructor is setting the time to 20:12 and has nothing to do with the year? This is the exact ambiguity that I want to discourage!!

new Carbon("2012") vs Carbon->now()->setTime(20, 12)

The first is "cleaner" but in no way obvious.

I agree that using new Carbon('now') should time travel...hence just implementing Carbon::now() is not enough.

The current implementation relies on modify() doing nothing for arguments like 'now' though.

$dt = new \DateTime('2011-09-01');
$dt->modify('now');
echo $dt->format('Y-m-d');     // 2011-09-01

Which seems wrong to me in the first place! :-)

Owner

briannesbitt commented Sep 17, 2012

You do realize that the '2012' in that constructor is setting the time to 20:12 and has nothing to do with the year? This is the exact ambiguity that I want to discourage!!

new Carbon("2012") vs Carbon->now()->setTime(20, 12)

The first is "cleaner" but in no way obvious.

I agree that using new Carbon('now') should time travel...hence just implementing Carbon::now() is not enough.

The current implementation relies on modify() doing nothing for arguments like 'now' though.

$dt = new \DateTime('2011-09-01');
$dt->modify('now');
echo $dt->format('Y-m-d');     // 2011-09-01

Which seems wrong to me in the first place! :-)

@davedevelopment

This comment has been minimized.

Show comment
Hide comment
@davedevelopment

davedevelopment Sep 17, 2012

You do realize that the '2012' in that constructor is setting the time to 20:12 and has nothing to do with the year? This is the exact ambiguity that I want to discourage!!

Yes, hence my example that shows the time getting set to 20:12 on two different dates, today's date and the date I've time travelled to.

The current implementation relies on modify() doing nothing for arguments like 'now' though.

Yeah, I realise that's a weak part in the implementation. Looking at the docs as well, it seems before 5.3.6 modify didn't even take in to account the absolute parts of a string, which would make this even more flaky.

Perhaps close this and we'll move on, I don't think we're going to get a very elegant solution.

davedevelopment commented Sep 17, 2012

You do realize that the '2012' in that constructor is setting the time to 20:12 and has nothing to do with the year? This is the exact ambiguity that I want to discourage!!

Yes, hence my example that shows the time getting set to 20:12 on two different dates, today's date and the date I've time travelled to.

The current implementation relies on modify() doing nothing for arguments like 'now' though.

Yeah, I realise that's a weak part in the implementation. Looking at the docs as well, it seems before 5.3.6 modify didn't even take in to account the absolute parts of a string, which would make this even more flaky.

Perhaps close this and we'll move on, I don't think we're going to get a very elegant solution.

@briannesbitt

This comment has been minimized.

Show comment
Hide comment
@briannesbitt

briannesbitt Sep 17, 2012

Owner

Well, I will close this but the idea will live on!! I'll try a few different things and see how it goes. At least a minimal implementation will make it in that will probably suffice for 90% of the time.

Thanks for you ideas and effort on this!

Owner

briannesbitt commented Sep 17, 2012

Well, I will close this but the idea will live on!! I'll try a few different things and see how it goes. At least a minimal implementation will make it in that will probably suffice for 90% of the time.

Thanks for you ideas and effort on this!

@davedevelopment

This comment has been minimized.

Show comment
Hide comment
@davedevelopment

davedevelopment Sep 17, 2012

No problem, give me a shout if you come up with something!

davedevelopment commented Sep 17, 2012

No problem, give me a shout if you come up with something!

@nrich2010 nrich2010 referenced this pull request Jun 21, 2013

Closed

Runtime exception error #13

briannesbitt pushed a commit that referenced this pull request Jun 25, 2015

Merge pull request #1 from wangchj/wangchj-isday-1
Add methods isSunday(), isMonday(), isTuesday() ..

briannesbitt pushed a commit that referenced this pull request Jun 26, 2016

Merge pull request #1 from lucasmichot/lndj-master
Fix Zh translation and add tests

lucasmichot pushed a commit that referenced this pull request Jan 24, 2017

Merge pull request #1 from ROKAISAKKON/ROKAISAKKON-patch-no-plural-in…
…-thai-lang

Translation for th is wrong

Ovsyanka pushed a commit to Ovsyanka/Carbon that referenced this pull request Jan 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment