Skip to content
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

ext-intl's use of IntlDateFormatter prototype has several bugs #3568

Open
drjayvee opened this issue Sep 30, 2021 · 11 comments
Open

ext-intl's use of IntlDateFormatter prototype has several bugs #3568

drjayvee opened this issue Sep 30, 2021 · 11 comments

Comments

@drjayvee
Copy link
Contributor

drjayvee commented Sep 30, 2021

\Twig\Extra\Intl\IntlExtension's constructor takes an optional IntlDateFormatter prototype. That works great for settings various defaults (dateType, timeType, pattern), but it doesn't enable automatic timezone conversion.

IntlDateFormatter

Consider this plain PHP example:

echo (new IntlDateFormatter(
	'nl_NL',
	IntlDateFormatter::MEDIUM, IntlDateFormatter::MEDIUM,
	new DateTimeZone('Europe/Amsterdam')
))->format(new DateTime()) . "\n";

This automatically formats the UTC datetime using Amsterdam time. Neat!

twig/intl-extra

new IntlExtension(new IntlDateFormatter(/* ... */)) however doesn't apply the timezone, unfortunately.

Why not?

IntlExtension::formatDateTime lets \twig_date_converter create a DateTime using CoreExtension::getTimezone() and passes its timezone to IntlExtension::createDateFormatter() - even when no timezone was passed to formatDateTime.

Passing in an explicit $timezone to createDateFormatter() makes the method use that value instead of $this->dateFormatterPrototype->getTimeZone(). (which is actually broken - see twigphp/intl-extra#6 ;p).

Quirky workaround

Maybe users are supposed to

$twig->getExtension(CoreExtension::class)->setTimeZone('Europe/Amsterdam');

In that case though, date strings are not converted at all:

{{ '2021-09-30 13:37'|format_datetime() }} {# still 13:37 #}

DateTime objects are converted.

@stof
Copy link
Member

stof commented Sep 30, 2021

In that case though, date strings are not converted at all:

Well, if your string does not contain a timezone offset, which conversion should be applied ? There is no info about the existing timezone.

@drjayvee
Copy link
Contributor Author

drjayvee commented Sep 30, 2021

Wow that's a fast reply!

There is no info about the existing timezone.

There is, actually. We set

ini_set('date.timezone', 'UTC');

CoreExtension::getTimezone() uses date_default_timezone_get() to get a default value, which in this case is UTC.

@drjayvee
Copy link
Contributor Author

Looking at IntlExtension::createDateFormatter():332

$timezone = $timezone ?: $this->dateFormatterPrototype->getTimeType();

always evaluates to $timezone = $timezone as the argument is never falsy.

(This does explain why twigphp/intl-extra#6 was never noticed.)

@drjayvee
Copy link
Contributor Author

Sorry about spamming in here, but I did want to point out another flaw in the current implementation. Allow me to quote myself from twigphp/intl-extra/pull/6:

IntlDateFormatter::getTimeZone() returns IntlTimeZone, which doesn't have getName() (it does have getId(): string), which is assumed on line 337

@drjayvee
Copy link
Contributor Author

drjayvee commented Sep 30, 2021

The 2nd patch in twigphp/intl-extra/pull/6 seems sensible to me and solves this issue. What do you think, @stof?

@drjayvee
Copy link
Contributor Author

drjayvee commented Sep 30, 2021

I've found another issue which is very related to the current one. I'm fine creating a separate issue if you want.

Consider

$twig->addExtension(new IntlExtension(
    new IntlDateFormatter(
	'nl_NL',
	IntlDateFormatter::MEDIUM, IntlDateFormatter::MEDIUM,
	new DateTimeZone('Europe/Amsterdam')
    )
));
{{ '2020-02-20 22:22'|format_datetime('full', 'none') }}

I'd expect the explicit date and time types (full, none) to override the defaults (both IntlDateFormatter::MEDIUM), but that doesn't happen because createDateFormatter always uses the prototype's string $pattern, which is based on its own date and time types.

If you agree that's a bug then I'd be happy to create a patch. I'd propose only using the prototype's pattern only if (!$dateFormat && !$timeFormat && !$pattern).

diff --git a/IntlExtension.php b/IntlExtension.php
index f8537c3..d941b76 100644
--- a/IntlExtension.php
+++ b/IntlExtension.php
@@ -338,7 +338,13 @@ final class IntlExtension extends AbstractExtension
             $timeFormatValue = $timeFormatValue ?: $this->dateFormatterPrototype->getTimeType();
             $timezone = $timezone ?: $this->dateFormatterPrototype->getTimeZone()->toDateTimeZone();
             $calendar = $calendar ?: $this->dateFormatterPrototype->getCalendar();
-            $pattern = $pattern ?: $this->dateFormatterPrototype->getPattern();
+            if ('' === $pattern) {
+               if (!$dateFormat && !$timeFormat) { // no explicit values for either format given
+                   $pattern = $this->dateFormatterPrototype->getPattern();
+               } else {
+                   $pattern = null;
+               }
+           }
         }

         $hash = $locale.'|'.$dateFormatValue.'|'.$timeFormatValue.'|'.$timezone->getName().'|'.$calendar.'|'.$pattern;

@stof
Copy link
Member

stof commented Sep 30, 2021

There is, actually. We set

ini_set('date.timezone', 'UTC');

CoreExtension::getTimezone() uses date_default_timezone_get() to get a default value, which in this case is UTC.

It uses the PHP default timezone as the default configuration. But if you change the config in CoreExtension, the default timezone of Twig is now Europe/Amsterdam, not UTC

@drjayvee
Copy link
Contributor Author

drjayvee commented Oct 1, 2021

What's your point exactly?

Well, if your string does not contain a timezone offset, which conversion should be applied ? There is no info about the existing timezone.

I'd argue that "bare" date(time) strings (which lack an offset) should be interpreted as being in the currently configured timezone - which is whatever CoreExtension::getTimezone() returns.

This is actually already the case!

format_datetime() should always convert to the IntlDateFormatter prototype's timezone.

This isn't happening for string dates. (It does work for DateTime-typed variables.)

Here's example to prevent miscommunication:

date_default_timezone_set($systemTimezone);
// and / or
CoreExtension::setTimezone($systemTimezone);

$twig->addExtension(new IntlExtension(
  new IntlDateFormatter($userTimezone) // wrong parameter config for brevity
));

$twig->render("  // invalid, but you get the idea
  {{ '2020-02-02 13:37'|format_datetime() }}
");

I'd argue that the datetime string should be parsed/instantiated as being in $systemTimezone and then converted to $userTimezone.

Do you disagree?

@drjayvee
Copy link
Contributor Author

drjayvee commented Oct 1, 2021

Oh, and any comment on the other issues I've found would be greatly appreciated.

Would you like separate issues for each? On this repo or on twigphp/intl-extra?

@dsuurlant
Copy link

It appears that the same is happening for locale -- if the dateFormatterPrototype specifies a locale, format_datetime and its related functions do not take this into account.

@drjayvee
Copy link
Contributor Author

drjayvee commented Oct 7, 2021

Due to the lack of feedback, we (@dsuurlant and me) will continue working on all the issues presented here in our fork of the twigphp/intl-extra repo. We're even adding tests!

I hope that helps anyone running into the same issues in the future.

While I certainly agree that twig shouldn't break backwards compatibility, the prototype functionality is seriously broken. I'll leave the decision of whether or not to break b/c to the twig maintainers. Perhaps in 4.x?

@drjayvee drjayvee changed the title ext-intl's format_datetime doesn't convert timezone according to IntlDateFormatter ext-intl's use of IntlDateFormatter prototype has several bugs Oct 7, 2021
@drjayvee drjayvee changed the title ext-intl's use of IntlDateFormatter prototype has several bugs ext-intl's use of IntlDateFormatter prototype has several bugs Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants