modify after timezone issue

[fedot01]

If I want to get midnight time for different timezones with initial time I got this problem. For example with timestamp 1485943200 I need to know midnight time for Europe/Moscow and Europe/Samara.

I do this:

$initial = Carbon::createFromTimestamp(1485943200);
$moscow_midnight = $initial->copy()->timezone('Europe/Moscow')->modify('midnight')->getTimestamp();
$samara_midnight = $initial->copy()->timezone('Europe/Samara')->modify('midnight')->getTimestamp();

And got:

$moscow_midnight = 1485907200
$samara_midnight = 1485907200

This is midnight time for GMT 0 for that day not for those timezones.

[Ovsyanka]

This behavior isn't expected for me too. And i founded that it was introduced while solving #88.

Actually, as i can see, all relative modifications was broken excluded "unit" stuff:
unit (('sec' | 'second' | 'min' | 'minute' | 'hour' | 'day' | 'fortnight' | 'forthnight' | 'month' | 'year') 's'?) | 'weeks' | daytext .

[Ovsyanka]

I figured that the problem, raised in #88 relates only to units <= hour.

For example we have the date 2014-03-30 00:00:00 in Europe/London (new Carbon('2014-03-30 00:00:00, 'Europe/London')), then if i want to increase date by 1 day, i expect 2014-03-31 00:00:00, but if want to increase date by 24 hours, then i expect 2014-03-31 01:00:00, because in this timezone there will be this time after 24 hours. The same for minutes and seconds.

So, in the end, i surely suppose, that fix implemented for #88 should be used only for seconds, minutes and hours modifications (i dont know, is there another modifications less then hour?). All other modifications should work like before.

[Ovsyanka]

I think, that next issues influenced by the same source code and will be influensed (solved) by changing it:
#887, #863, #871
Maybe we should add some additional tests for that issues.

[kaufholdr]

Can you state your systems timezone? date_default_timezone_get

[Ovsyanka]

I can, but why you want to know ours timezones?

[kaufholdr]

It's only interesting if one of the used timezone is your default one or not because the Carbon::modify method behaves differently if the calculation is done in your timezone.

[kaufholdr]

I can answer the question myself. If you execute it with your local timezone it gives the correct result. So both are not your local timezone. ;-)

[Ovsyanka]

Yes, you are mostly right. But there steel be a problem with adding (sec,min,hour) in local timezone. The problem described in #88 wasnt solved complitely and tests dont cover cases in the right way. Mixing there the logic depending on "local / not local" timezone was wrong.

[Ovsyanka]

Btw, i think we should talk here now about how it should works (i described my vision, but didnt get any feedback) and my implementation of introducing that.

[kaufholdr]

The way you propose is the same as it is done in Java java.time.ZonedDateTime.

DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");
		
ZonedDateTime time = LocalDateTime.from(formatter.parse("2014-03-30 00:00:00"))
					.atZone(ZoneId.of("Europe/London"));

System.out.println(time);               // 2014-03-30T00:00Z[Europe/London]
System.out.println(time.plusDays(1));   // 2014-03-31T00:00+01:00[Europe/London]
System.out.println(time.plusHours(24)); // 2014-03-31T01:00+01:00[Europe/London]

[Ovsyanka]

It is cool, because now i see, that not only me thinks in that way =)
Thank you for pointing to this!

[balintant]

When is this - or a similar - fix going to be merged? We are struggling because of this issue 🙁

[Ovsyanka]

I investigated related issue about other units (Before I thought, that the issue relates only to units <= hour). If initial date have such time, that becomes "unexisted" after modification - it always increasing not matter what was initial offset. And it is wrong I think in case of negative changing.

I will show you in this example:

// This works well, as expected
$date =  new \DateTime('2014-03-29 01:30:00', timezone_open('Europe/London')); // UTC+0
$date->modify('+1 day');
$dateExepcted =  new \DateTime('2014-03-30 02:30:00', timezone_open('Europe/London')); // UTC+1
$this->assertEquals($dateExepcted, $date);

// But this not
$date =  new \DateTime('2014-03-31 01:30:00', timezone_open('Europe/London')); // UTC+1
$date->modify('-1 day');
$dateExepcted =  new \DateTime('2014-03-30 00:30:00', timezone_open('Europe/London')); // UTC+0
$this->assertEquals($dateExepcted, $date);

The result is:

--- Expected
+++ Actual
@@ @@
-2014-03-30T00:30:00.000000+0000
+2014-03-30T02:30:00.000000+0100

I think, that logic should be like that: 2014-03-31 01:30:00 UTC+1 minus one day will be 2014-03-30 01:30:00 UTC+1, but in that moment UTC+0 should be used, so it is not correct date. In UTC+0 this date would be 2014-03-30 00:30:00 UTC+0

What do you think about that? Should I change this at the same time as initial issue?

[Ovsyanka]

And more: if you remove and then add the same period - the date will be different. It is not good sign:

$date = new \DateTime('2014-03-31 01:30:00', timezone_open('Europe/London')); // UTC+1
$date->modify('-1 day');
$date->modify('+1 day');
$dateExepcted = new \DateTime('2014-03-31 01:30:00', timezone_open('Europe/London')); // UTC+1
$this->assertEquals($dateExepcted, $date);

result is:

--- Expected
+++ Actual
@@ @@
-2014-03-31T01:30:00.000000+0100
+2014-03-31T02:30:00.000000+0100

[vlakoff]

I tried your examples and they work here. I guess there may be something related to the PHP configuration.

(tried Carbon 1.22.1 and master)

[Ovsyanka]

I am sorry. Most likely it because modify customizations implemented in #88. In my current state I got rid of them, but they still exists in the master. They generally wrong, but could affect in some cases like "the right way". It will be more correct to replace new Carbon to new \DateTime in my examples. I edited them. The question I raised is about how it should work. Do you agree with my logic or not.

[vlakoff]

It seems I agree:

• 24 hours are 24 hours, it's real time, no matter the timezone changes,
• then for displaying use the timezone at that time.

A good illustration it's sensible is that the change can be made back and forth without degradation.

[Ovsyanka]

This comment will be changed when new details appears.
I am not ready to propose new algorithm, but here is the new detailes about how PHP works with DST changes.

• Firstly, it always tries to to change the time to look like the offset didn't changed. It doesn't matter what direction of changes or what modification units used. But if the time after such modification will be in the skipped period the change not appears or just reverts (because secondly).
• Secondly, if time of a date set up into skipped hour (between 1:00 and 1:59) it changing by 1 hour forward. 1:00 becomes 2:00, 1:59 btcomes 2:59.

All my examples will be in 'Europe/London' timezone. And i will use 2014 year.

When local standard time was about to reach 2014-03-30 01:00:00 +00:00 clocks were turned forward by 1 hour to 2014-03-30 02:00:00 +01:00 local daylight time instead.
And when local time was about to reach 2014-10-26 02:00:00 +01:00 clocks were turned backward by 1 hour to 2014-10-26 01:00:00 +00:00 local time instead.

Examples:

• '2014-03-30 00:59:59 +00:00' +2 sec = '2014-03-30 02:00:01 +01:00' - time not changed because 2:00:01 - 1 = 1:00:01 and this time automatically becomes 2:00:01.
• '2014-03-30 00:00:00 +00:00' +7199 sec = '2014-03-30 02:59:59 +01:00' - the same here
• '2014-03-30 00:00:00 +00:00' +7200 sec = '2014-03-30 02:00:00 +01:00' - but here PHP "fixes" the time like it would be in the +00:00

[Ovsyanka]

I am not sure now about all the idea to change the DST change handling standing just on modification units (#883 (comment)). It is logical, but it is not obvious. Especially for people who knows how the PHP acts itself.

Maybe it will be better to let PHP do his work as is when modify method used and implement "right" modifying only when Carbon (add|sub)(Minute|Second|Hour) used?

In that approach we will not do redundant and complicated checking job on each modify call and will not confuse peoples who expects default behavior as in PHP. And we can put the comment about difference with PHP in the docs of Carbon functions.

Plase, react with +1 if you agree and -1 plus write comment if you don't.

[Ovsyanka]

The situation when removing and then adding the same period leads to time changing is quite separate issue and I think it really wrong and should be fixed.

update: I founded the PHP bug 74274 about this with the link to suggested rfc

Plase, react with +1 if you agree and -1 plus write comment if you don't.

[klarkent]

I would argue that additions/subtractions should be done using UTC and then back to the timezone to avoid loosing added/subtracted time and that for clarity the behaviour should be consistent in the whole library.
Currently if you take a DateInterval and add/subtract it do a Carbon date, DST is ignored and this brings to the correct result of adding that exact amount of time. Another (a bit ugly) option would be to have in all those helper functions a parameter like absolute=true that would allow you to addHours(5, false) or addDays(90, false) and do that using the current timezone (which then respects the DST changes) and take all the risks that this brings like adding and removing the same amount of time gives time loss that would never happen if we do calculations in UTC. Then state in huge fonts in the documentation what's the default behaviour and how can one use the desired one.
Edit: of course this would mean a major version since we are breaking compatibility with the old behaviour but at least it adding/removing time will be deterministic and not based on what unit you're using.

[Ovsyanka]

@klarkent, thank you for your opinion.

It is important to speak in common language so I want to clarify terms. What do you mean "respect DST changes"? Am I right that in this example DST changes was "respected"

'2014-03-30 00:00:00 +00:00' +1 day = '2014-03-31 01:00:00 +01:00'

and in this "ignored"?

'2014-03-30 00:00:00 +00:00' +1 day = '2014-03-31 00:00:00 +01:00'

[klarkent]

Sorry my post was not that clear in that regard. So yes respecting DST is the first of your examples, ignoring it is the second one. But what I want to point out is that the developer should have a choice and that the behaviour should be consistent and not change just because you're using another unit of time. It does not matter what's the default to me but I want to know it and know how to use the other one if I need to.

[Ovsyanka]

@klarkent,

the developer should have a choice

I am completely agree with you at this point. And in my proposal there is possibility to use default PHP handling by calling Carbon->modify() method.

the behaviour should be consistent and not change just because you're using another unit of time

Here I personally don't agree. But is it questionable. Here is my arguments:

1. Initially there was Adding to a time provides an inconsistent result #88 issue (that was not solved correctly and now there is errors because it). The thing is that default PHP handling quite confusing. If I add 24 hours I expect that there will be 24 hours difference. But it is not (in case with DST changes).
2. In matter of realistic using of code, I think it is rarely, when you want to see "respected" DST change when you operates with days or higher units (time in the new offset will be changed).
3. I think that it is rare situation when you want to use both Carbon->modify() and Carbon->add|sub[unit](). More often you use one of them only.

I agree, that consistent is important. In that case I think it is worth to sacrifice it for convinience sake. But it still the root question. It is why I started a discussion. As I see you suppose the second argument to add|sub functions. If the community will support your choise - we will stand on that.

[klarkent]

I think that it is rare situation when you want to use both Carbon->modify() and Carbon->add|subunit. More often you use one of them only.

I will give you a concrete example that will explain the problem with the solution of having different behaviour:
Assume that you work for a company that offers 1-3-6-12 months subscriptions to a product. Either you will take the current date and do a Carbon::now()->addDays(90) or you could express the same in months for example but you will probably never have the idea of doing Carbon::now()->addHours(90*24) because that kind of defeats the reason of using Carbon to an extent. The problem with this is that, depending on the amount that you're adding you WILL cross the DST day and users will actually get some hours more/less depending on which is the current timezone. Giving more time by mistake can be fine, giving less may considered stealing.

But it still the root question. It is why I started a discussion. As I see you suppose the second argument to add|sub functions. If the community will support your choise - we will stand on that.

I understand that, I also am giving my opinion. I just hope that this doesn't hang for another year

[Ovsyanka]

I will give you a concrete example that will explain the problem with the solution of having different behaviour.

Actually it not explains "different behavior" (inconsistence) problem. But it is a good example when people could expect "respects" handling. It is not so simple, though. For example, you talking about months, but there is days in your code. Why? I think because a month != 30 days and in fact that company offers days and not months. Same for day. Day != 24 hours (you can read about it in the wiki - there is not only DST changes, but in addition there is the leap second and maybe something else.

Giving more time by mistake can be fine, giving less may considered stealing.

The same could happens with "respects" handling too. For example: Subscription from 2014-03-30 00:00:00 +00:00 for 30 days will be ended on 2014-04-29 23:00:00 +01:00. And the customer will be surprised. Because you sell him the subscription "in days", not "in hours". So, If you want to take money for actually passed time, you maybe should write about it in the documents (that day means not calendar day, but 24h or 86 400 seconds) and calculate the end date according to that.

When I was thinking about that all I realised, that in my approach there is no way to handle >= day modifications in the "respects" mode. And, as I said before, I agree with you, that developer should have a choice. I can suggest different methods for that, like addDaysRespectDst, etc. But I don't like this function name and can't imagine the better one.

By now I see two realistic ways:

1. modify "ignores" DST changes (like in DateTime); add|sub{unit<day} "respects" DST change; add|sub{unit>=day} "ignores" (like in DateTime).

2. modify and add|sub functions both "respects" DST change by default. That is not like in DateTime. They accept optional arguments to let them "ignore" DST changes.

[Ovsyanka]

I got new thoughts. The third way:

3. Both modify and add|sub functions works like in php DateTime without any corrections (excluded possibly bug workarounds, like I described before).

Why? Because I realise (why I didn't do it before?) that PHP DateTime already have the way to not bothering about DST influence. By using the UTC timezone. So if you want to timestamp corrections when DST changes - you should use some local timezone. If you don't - use UTC. That is it.

And first two ways just add more complecity, but they didn't really offer anything in exchange because you still have to think about what do you want in particular case and how you can achieve this.

And now I stand on that, third variant. I have some other arguments in the hand, but don't think they needed.

[Ovsyanka]

In the end I suggest to:

• In this issue revert the Adding to a time provides an inconsistent result #88 changes (see my first comments here) because they was wrong and that will fix initial problem described here. There is Reverted pull-request #548 about issue #88 #1070 pull-request.
• Create different issue to fix the PHP bug 74274 and make things work like described in rfc in all volume or in part of it. There is Workaround about php bug 74274 #1071 pull-request.
• Create different issue to add some tips to the documentstion to help developers to choose right timezone to their goals.

[klarkent]

In the end I suggest to:
In this issue revert the #88 changes (see my first comments here) because they was wrong and that will fix initial problem described here.
Create different issue to fix the PHP bug 74274 and make things work like described in rfc in all volume or in part of it.
Create different issue to add some tips to the documentstion to help developers to choose right timezone to their goals.

This sounds like the cleanest option to me and the developer would have a choice which would be to use UTC to avoid DST to be taken into account in date operations. It will also be consistent if the bugs that PHP has are fixed in the library, so I'll vote for it!

[Ovsyanka]

For all peoples, who interested in this: fixing can take a few time, especially because I am quite confusing about is that project alive or not. So when I will make the fixes - quite expecting that they will not be merged.

But you already could just revert the 253f381 commit and you will be just with the php bug that arises quite rarely I think, but without #88 implemented errors, that is the main problem.

@klarkent, thanks for the useful discussion. I glad we came to agreement =)

[Ovsyanka]

I created 2 pull-requests:

• Reverted pull-request #548 about issue #88 #1070 with reverting Adding to a time provides an inconsistent result #88 changes
• Workaround about php bug 74274 #1071 for php 74274 bug workaroud. There just unit tests for now and I suggest you to check them out and say what do you think. But by now I don't see effective way to fix it without modifying PHP itself.

[Glavic]

Working on solution in #1070 PR. TY