fix(CalDav): add support for Microsoft time zones #49459
Conversation
SebastianKrupinski
requested review from
st3iny and
tcitworld
as code owners
SebastianKrupinski
force-pushed
the
fix/issue-48079-windows-time-zones
branch
from
b6b5030
to
8425ae8
Compare
|
/backport to stable30 |
SebastianKrupinski
force-pushed
the
fix/issue-48079-windows-time-zones
branch
from
8425ae8
to
dca1412
Compare
Signed-off-by: SebastianKrupinski <[email protected]>
SebastianKrupinski
force-pushed
the
fix/issue-48079-windows-time-zones
branch
from
dca1412
to
2b6a44e
Compare
| 'AUS Central Standard Time' => 'Australia/Darwin', | ||
| 'Aus Central W. Standard Time' => 'Australia/Eucla', | ||
| 'AUS Eastern Standard Time' => 'Australia/Sydney', | ||
| 'Afghanistan Standard Time' => 'Asia/Kabul', | ||
| 'Alaskan Standard Time' => 'America/Anchorage', |
There was a problem hiding this comment.
What's the source of this list? Is it static?
| if ($zone instanceof DateTimeZone) { | ||
| return $zone; | ||
| } else { | ||
| return new DateTimeZone('UTC'); |
There was a problem hiding this comment.
how about returning null here too so callers can distinguish between a successful translation and the fallback?
I think calling the method with $tz = TimeZoneFactor::fromName($foo) ?? new DateTimeZone('UTC') makes it more explicit when you actually want the fallback, and allows you to continue with null when you want to handle the failure.
| * | ||
| * @return DateTimeZone | ||
| */ | ||
| public static function fromName(string $name): DateTimeZone { |
There was a problem hiding this comment.
Could this be a dynamic method to allow overrides/mocks in tests?
|
|
||
| use DateTimeZone; | ||
|
|
||
| class TimeZoneFactory { |
| /** | ||
| * conversion table of Microsoft time zones to IANA time zones | ||
| * | ||
| * @var array $ms2iana |
There was a problem hiding this comment.
| * @var array $ms2iana | |
| * @var array<string, string> $ms2iana |
didn't check but please make the type specific
| * | ||
| * @var array $ms2iana | ||
| */ | ||
| private static $ms2iana = [ |
| private $vCalendar2; | ||
| /** @var VCalendar */ | ||
| private $vCalendar3; | ||
| private VCalendar $vCalendar1a; |
There was a problem hiding this comment.
nitpick: try to avoid this in bug fixes, because it may make the backport process more complicated. I suggest to either have a separate commit for refactorings, which can be left out of the backport, or do this in a separate PR
ChristophWurst
added
the
pending documentation
@SebastianKrupinski please keep the original PR template from https://github.com/nextcloud/server/blob/master/.github/pull_request_template.md. Writing docs is very relevant here. Don't drop this. |
| /** | ||
| * Determines if given time zone name is a Microsoft time zone | ||
| * | ||
| * @since Release 31.0.0 |
There was a problem hiding this comment.
| * @since Release 31.0.0 | |
| * @since 31.0.0 |
same for the rest of the file
Summary
Checklist