Skip to content

[Form] Intl and TimezoneType are not interchangeable #22302

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

Closed
rvanlaak opened this issue Apr 5, 2017 · 8 comments
Closed

[Form] Intl and TimezoneType are not interchangeable #22302

rvanlaak opened this issue Apr 5, 2017 · 8 comments

Comments

@rvanlaak
Copy link
Contributor

rvanlaak commented Apr 5, 2017

Q A
Bug report? yes
Feature request? yes
BC Break report? no
RFC? yes
Symfony version any

We make use of the intl extension to nicely render DateTime objects according to the active locale and timezone. The problem with this combination is that it are two different kind of PHP modules, and their datasets do not completely match.

When the diff gets calculate over the intl and datetime timezones, you expected the result to be empty, but it is not. The outcome looks like the following:

"America/Fort_Nelson",
"America/Punta_Arenas",
"Antarctica/Troll",
"Asia/Atyrau",
"Asia/Barnaul",
"Asia/Chita",
"Asia/Famagusta",
"Asia/Khandyga",
"Asia/Srednekolymsk",
"Asia/Tomsk",
"Asia/Ust-Nera",
"Asia/Yangon",
"Europe/Astrakhan",
"Europe/Busingen",
"Europe/Kirov",
"Europe/Saratov",
"Europe/Ulyanovsk",
"Pacific/Bougainville"

This can be solved by using the intersect of both datasets instead:

$intlTimezones = iterator_to_array(\IntlTimeZone::createEnumeration());
$timezoneIntersection = array_intersect($intlTimezones, \DateTimeZone::listIdentifiers());

Would it be wise to let the TimezoneType choice populater take this into account in case the intl extension is available / activated? Twig does use intl by default if I'm correct?

@artursvonda
Copy link
Contributor

What's the action plan here? Using intl for datetime doesn't sound right since that's the not the database used to create datetimezone objects used in datetime objects. Is it issue with datetimezone and intl having different sources for timezone data or they represent 2 different things?

@ro0NL
Copy link
Contributor

ro0NL commented Oct 11, 2018

I've invesigated the timezone info from Intl a bit, see also

this could be leveraged by a new TimezoneBundle in the Intl component, solving #26851 #22262, #25165 while at it.

From what ive seen so far, timezoneTypes.txt provides the aliases from intl to datetimetz.

I played around with it already (see https://github.com/ro0NL/symfony/commit/2ef271bf4096dc48031888da23f424c43673364a) but ran into some compile issues :(

https://hub.docker.com/r/jakzal/php-intl/ is missing the git binary, so doesnt work well out-of-the-box, and locally im stuck on icu 55. Would be nice if @jakzal could fix it ;)

@jakzal
Copy link
Contributor

jakzal commented Oct 11, 2018

https://hub.docker.com/r/jakzal/php-intl/ is missing the git binary, so doesnt work well out-of-the-box, and locally im stuck on icu 55. Would be nice if @jakzal could fix it ;)

For now you can just create your own image based on the above and add git into it (I added jakzal/docker-symfony-intl#6 for myself).

@ro0NL
Copy link
Contributor

ro0NL commented Oct 11, 2018

@rvanlaak the issue here with the difference in timezones is related to BC.

Intl is more up to date then PHP, which includes still some BC timezones.

for America/Punta_Arenas see https://3v4l.org/mfjIc if we look at https://github.com/unicode-org/icu/blob/master/icu4c/source/data/misc/metaZones.txt#L2268 we see it's BC, so makes sense Intl actually returns Etc\Unknown here.

Asia/Atyrau => https://github.com/unicode-org/icu/blob/master/icu4c/source/data/misc/metaZones.txt#L2724
etc.

Others, e.g. Europe/Busingen, are probably added in a later version, see https://3v4l.org/mJdOW

@rvanlaak
Copy link
Contributor Author

Great work on investigating the root cause. 👏 I think solving this issue is a matter of choosing:

  1. Supporting Intl everywhere, and solve this issue by a) alias intl to datetime and/or thereafter b) intersecting both sets to be sure no colissions can happen.
  2. Decide to not supporting intl, and solely use PHP's identifiers in the form type.
  3. As the impact of this issue is small, live with it and keep this a documented userland problem.

We use our custom form type that intersects both sets since we found out about this bug, so we would prefer 1.

@ro0NL
Copy link
Contributor

ro0NL commented Oct 11, 2018

the relevant timezone ids can be used both with php datetimezone and intl, there's no difference here.

we should only allow for timezone ids from intl that are supported by datetimezone, in a dedicated timezonebundle (symfony/intl) so the list is fixed and based on latest ICU.

the unsupported ones are not really relevant anyway :) https://3v4l.org/1Xit4

@ro0NL
Copy link
Contributor

ro0NL commented Oct 11, 2018

alternatively the timezonebundle could provide both lists, for the form type: we've recently added the input option (#23648)

so we could decide which list to use based on input=intltimezone vs. input=datetimezone

@ro0NL
Copy link
Contributor

ro0NL commented Oct 12, 2018

@rvanlaak im not sure about excluding timezones not supported by \IntlTimeZone. Symfony doesnt support it as an underlying model.. only string and \DateTimeZone and for those the current list works fine.

So the topic is if we should exclude BC zones in the TimezoneType list.... it didnt raise any issues so far and it's basically is a BC break :) so im skeptical.

Looking at the timezone widget from Google Calendar, even there BC zones seem to be included (e.g. America/Punta_Arenas).

I've also opened #28836 to propose switching to symfony/intl instead. That would still includes BC zones though, but we could check for that from there on (if needed).

So im 👎 for the suggested change here as is, but after #28836 we have a much better base to check it and e.g. support input=intltimezone to use \IntlTimeZone as underlying model. Only then the BC zones need to be excluded, as it's technically unsupported for the model).

For now, i think your best of to set the available zones manually.

@fabpot fabpot closed this as completed Apr 27, 2019
fabpot added a commit that referenced this issue Apr 27, 2019
This PR was squashed before being merged into the 4.3-dev branch (closes #31195).

Discussion
----------

[Form] Add intltimezone input to TimezoneType

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #22302 (ref #28836)
| License       | MIT
| Doc PR        | symfony/symfony-docs#11463

cc @rvanlaak

Commits
-------

e169dfb [Form] Add intltimezone input to TimezoneType
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants