Cleanup code handling timezones when converting icalDate to PHP's DateTime #222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There are quite a few things currently wrong with this method:
* If a date is pre-1970, then a timezone *is* applied - so long as it's IANA (or
converted Microsoft): support for CLDR timezones was never added into this logic
path.
* If a date is pre-1970, the branch of code ignores the work done by the earlier
`preg_match`, and acquires the timezone through string manipulation instead.
* If neither $forceUtc or $forceTimezone are true (ie. the default state) then the
timezone attached to a date is not applied to the date.
- (If ignoring all timezone hinting is desired, then an argument "$ignoreTimezone"
would be a better option.)
* If $forceUtc is true, but $forceTimezone is false, then all dates in a calendar
are assumed to be UTC.
* If $forceTimezone is true, then the value of $forceUtc is functionally ignored.
- (Admittedly it does lead to the timezone of any new PHP `DateTime` object that
is created from a UTC time to be set to UTC twice, but who's counting?)
- This is also the only logic code path that a post-1969 date actually has its
timezone applied.
* In every call (except two) to this function, both $forceTimezone and $forceUTC
are passed as true (contrary to their default value of false).
- The two exceptions to this are the `dtstart` and `dtend` of the first date of
a Monthly recurrence rule - meaning that the first date of said rule will have
no timezone applied (unless pre-1970), whilst subsequent dates generated from
the rule will.
The provided changeset resolves all of that, and is a cleaner and saner approach
to conversion.
u01jmg3
changed the title
|
Immediately getting an exception: |
Rather than have PHP try to work it out without revlevant context. This means that if users try to feed us a date with an out-of-range date-portion (eg. 20171032), then PHP can deal with it sensibly instead of throwing an exception.
|
I'm now getting: Can you try testing with |
Given `TZID="(UTC-05:00) Eastern Time (US & Canada)":20160409T090000`: * `str_getcsv()` inside `keyValueFromString()` strips the quote marks from the timezone * The iCal Date is later put back together in `processEvents()` as `TZID=(UTC-05:00) Eastern Time (US & Canada):20160409T090000` * This violates the iCal specification, as `:` is a control character and may not be used within what the specification calls "paramtext" Thus, in this changeset: * We add back the quotes in `processEvents()` * Unfortunately, this means that CLDR timezone identifiers, (e.g. `Europe/Berlin`) also now have quotes around them and so do not get recognised as a valid timezone * So we remove the quotes before the timezone text is passed to be comprehended. *sigh*
|
@s0600204: thanks for being so quick with these fixes. Reading your latest commit message:
Can you see a way around this? |
|
The new method could, in theory, be used on all param-texts (as the iCal specification calls them) not just TZIDs, but applying it to all is beyond the intended scope of this pull request. (And I meant |
|
(Sorry for the slow response) |
Code was generating a date-time string that was essentially UTC, but formatting it in such a way that was causing `iCalDateToUnixTimestamp` to interpret it as a "floating" local time. Solution is not to format a integer into a string and then interpret it into a different integer. A better solution might be to maintain a php DateTime object instead of an integer timestamp: bumping up the days, months, weeks or years as necessary, so constantly converting between integers, strings, and the odd DateTime object is unnecessary. However, rewritting much of the event recurrence handling code is beyond the scope of the pull request this commit is destined for.
u01jmg3
approved these changes
Jul 10, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are quite a few things currently wrong with this method:
If a date is pre-1970, then a timezone is applied - so long as it's IANA (or converted Microsoft): support for CLDR timezones was never added into this logic path.
If a date is pre-1970, the branch of code ignores the work done by the earlier
preg_match, and acquires the timezone through string manipulation instead.If neither
$forceUtcor$forceTimezoneare true (i.e. the default state) then the timezone attached to a date is not applied to the date.If
$forceUtcistrue, but$forceTimezoneisfalse, then all dates in a calendar are assumed to be UTC.If
$forceTimezoneistrue, then the value of$forceUtcis functionally ignored.In all but two calls to this method, both
$forceTimezoneand$forceUTCare passed astrue(contrary to their default value offalse).dtstartanddtendof the first date of a Monthly recurrence rule - meaning that the first date of said rule will have no timezone applied (unless pre-1970), whilst subsequent dates generated from the rule will.The provided changeset resolves all of that, and is a cleaner and saner approach to conversion.