Update occursAt function to handle DateTime in different timezone

[gordonzhao]

No description provided.

[rlanvin]

I think this will create problems because you are overriding the timezone if one was provided. You should only set the timezone only if $date does not already contain a timezone.

[rlanvin]

Thank you, however I cannot accept this pull request as is.

First, could you please write unit tests that showcase the bug you are trying to solve?

Then, I'm guessing the bug happens when you pass a string (without a timezone information), and dtstart is not in PHP's default timezone?

The problem is that your patch always overrides the timezone. You should only set the timezone if none was provided. The only place where this can happen is inside inside parseDate, so I suggest adding a second optional parameter $default_timezone (default null). If $default_timezone is provided:

• When date is an integer: always set the timezone
• When date is already a DateTime: ignore (it's the responsibility of whoever created the DateTime in the first place)
• When date is a string, then it's tricky. ISO 8601 or RFC 2822 format both include a timezone I think, so you shouldn't override it in this case. Otherwise you can set the timezone. It can be a bit difficult to detect if the string contains timezone information or not.

(note: You can use git commit --amend and git push -f to add it to the existing commit.)

[gordonzhao]

Thanks, I will create some unit tests when I have time. The problem actually happens when a DateTime has been passed, because this function assumes the $date is in the default timezone. It's fine for a string without timezone information. However, for a DateTime or ISO8601 string which has different timezone, I think this function should convert $date to the same timezone as default timezone.

For example in the line 758 if statement
in_array($date->format('G'), $this->byhour)

$date->format('G') can be any hour value based on different timezone, so the if statement almost fails every time for me because most of my events have local timezone but my server timezone is UTC.

[rlanvin]

However, for a DateTime or ISO8601 string which has different timezone, I think this function should convert $date to the same timezone as default timezone.

I don't think that is necessary. PHP is able to compare DateTime objects which have a different timezone. For example:

var_dump(
    date_create('2015-07-02 09:00:00',new DateTimeZone('Australia/Sydney'))
    ==
    date_create('2015-07-01 23:00:00',new DateTimeZone('UTC'))
); // this is true

I added a unit test, but I didn't manage to reproduce the bug you are describing. Please have a look at e0ca376 and let me know.

[gordonzhao]

In the testOccursAtTakeTimezoneIntoAccount() function you added, if you remove line 1711:
$this->assertTrue($rrule->occursAt(date_create('2015-07-02 09:00:00',new DateTimeZone('Australia/Sydney'))));
the test would fail. That's because if line 1711 ran first it would cache the correct DateTime values.

Comparing DateTime objects in different timezones wouldn't cause any problem, but comparing the formatted DateTime strings is not safe, unless they are in the same timezone. In the occursAt() function we have:
in_array($date->format('G'), $this->byhour)
$date->format('G') is the local hour value based on timezone, so it can be any hour value. For example,
date_create('2015-07-02 09:00:00',new DateTimeZone('Australia/Sydney'))->format('G') is 9,
and date_create('2015-07-01 23:00:00',new DateTimeZone('UTC'))->format('G') is 23. Even they are the same time, the results are different.

[rlanvin]

That's because if line 1711 ran first it would cache the correct DateTime values

Indeed, good catch! I didn't think about the cache at all... I managed to reproduce the bug and I understand your pull request now, it makes total sense to convert the timezone. Thanks!

By the way, while adding tests for your bug, I also discovered another bug at weekly frequencies that the cache was also "hiding" from the unit tests, so double thanks!

[gordonzhao]

Interesting, actually I also found this bug from fixing a bug in my code.Thanks for your great work.