Problem/Motivation

In #1802278: Add a Date component to core, we added support for the IntlDateFormatter class, which was added in PHP 5.3. This class provides locale-aware date formatting for PHP dates. When a Drupal site is running PHP is compiled with INTL support (http://ca2.php.net/manual/en/intl.installation.php), DrupalDateTime::format() attempts to automatically switch to IntlDateFormatter instead of the longstanding date_format() used in Drupal 7 and before.

We use the INTL date formatter if you have a default country set - which will be the majority of sites. If you leave this unset in the installer or use drush and don't explicitly set this, then we use PHP date formatting (how Drupal 7 and older worked).

The INTL/PHP date switching is not well-tested and has numerous bugs that will affect a site with INTL support:

  • If you configure all your data formats and then set a default country or unset the default country all your formats will change. When configuring a date format you only get to set up the currently active format.
  • Currently, only PHP formats can be translated. The INTL class does handle some forms of translation, but we've still removed the ability to have different Drupal date formats depending on translation.
  • f you have a view with a custom_date_format then this will break if you switch.
  • It is difficult to provide a migration path for the switch from PHP to INTL date format
    Furthermore the move from PHP date format to INTL is interesting in terms of migration, and custom date formats are currently not migrated for this reason. From the Known issues with the Drupal 6 -> 8 migrate path:

    Only the default, short, medium and long formats are migrated. All other formats default to the fallback format and need to be reconfigured after migration.

Proposed resolution

Remove the automatic switching to INTL dates and consistently use the PHP date format.

Remaining tasks

Patch is RTBC.

The following issues may be resolved by this patch:

User interface changes

Before

After

API changes

Yes. (List here.)

Files: 
CommentFileSizeAuthor
#28 DateFormatBefore.png51.38 KBGábor Hojtsy
#28 DateFormatAfter.png42.97 KBGábor Hojtsy
#23 2276183.23.patch51.75 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,608 pass(es). View
#23 19-23-interdiff.txt330 bytesalexpott
#19 2276183.19.patch51.32 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,599 pass(es). View
#19 11-19-interdiff.txt2.14 KBalexpott
#11 2276183.11.patch48.76 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,545 pass(es), 9 fail(s), and 1 exception(s). View
#11 10-11-interdiff.txt2.8 KBalexpott
#10 2276183.10.patch48.61 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,542 pass(es), 9 fail(s), and 1 exception(s). View

Comments

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes

php-intl is not ubiquitous in php (my bad). So only option 2 is available. Updated summary.

alexpott’s picture

chx’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes

I don't understand what the bug is here. I think it is somewhere in this paragraph:

In order to use the intl date formatter your php must be compiled to enable intl (http://ca2.php.net/manual/en/intl.installation.php). Once that is done our intl / php date switching is only half functional at best. For example if you configure all your data formats and then set a default country or unset the default country all your formats will change. When configuring a date format you only get to set up the currently active format. Additionally the way the current set up is only PHP date formats are translatable. Whilst it is true that the INTL class does handle some forms of translation this means we've removed the ability to have wildly different date formats depending on translation. Also, if you have a view with a custom_date_format then this will break if you switch.

I see 3-4 separate possible bugs in this paragraph:

  1. For example if you configure all your data formats and then set a default country or unset the default country all your formats will change. When configuring a date format you only get to set up the currently active format.

    Why wouldn't my date formats change based on the location? Are you saying that user-defined data is somehow lost? Can you provide a specific example to illustrate the problem?

  2. Additionally the way the current set up is only PHP date formats are translatable. Whilst it is true that the INTL class does handle some forms of translation this means we've removed the ability to have wildly different date formats depending on translation.

    So, I get that I might want to have my date format as 5-29-2014 for one translation, and 29.05.2014 for another, and 30th of Rajab, 1435 A.H. for another. Are you saying this is not possible presently? Is this a regression from D7?

  3. Also, if you have a view with a custom_date_format then this will break if you switch.

    Break how? Can you provide steps to reproduce?

  4. Furthermore the move from PHP date format to INTL is interesting in terms of migration. One of the known issues for migrate is that custom date formats are not migrated (and there is no plan to fix this).

    Okay, but interesting how? The link is not a bug report; it says:

    Only the default, short, medium and long formats are migrated. All other formats default to the fallback format and need to be reconfigured after migration.

Also, it's not clear to me why this must block release and the beta. What are the implications if this doesn't get done? What are the implications of resolving it after beta? Could it be resolved in contrib, or in 8.1.0? Is it worth stopping everyone from using Drupal 8 to make the change here?

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config

@xjm: yeah date formats are separate per language in Drupal 7, so date formats not translatable is a regression. Also PHP formats ARE translatable in Drupal 8 so intl formats not being so is pretty inconsistent.

When we have been doing the date translation, we could not figure out how/when would intl be ever used and it seemed like it may be dead code but we did not follow up with that... So good find and sorry for not bringing this up there :/

alexpott’s picture

Title: Date formats switching between intl / php is a mess » [META] Date formats switching between intl / php is a mess
Issue tags: -D8MI, -language-config
  1. Because whether or not INTL is use depends on whether the result of \Drupal\Component\Datetime::canUseIntl() - if php-intl is available then changing the default country or providing a country to \Drupal\Component\Datetime::format() will change whether or not INTL is used. Actually reading the code this gets us into the interesting situation where the format function is passed a country but no default country is defined thereby using the INTL formatter but you can not configure it!!!! (So yes this is a mess). Fortunately this is currently impossible in core because we always pass in the default country - see Drupal\Core\Datetime\Date::format() - however this suggests even more issues since that means we're never setting the country of an INTL date formatter to user's country - only ever the sites country.
  2. Yes this is a regression - there is a fix in #2204207: Missing format field when translating a date format using php-intl. but the fix there is going about it the wrong way because the INTL / PHP date format is so confusing.
  3. It will break because say you've configured a PHP date format it will suddenly pass this to an INTL formatter. The steps to reproduce are:
    1. PHP compiled with INTL available
    2. Install drupal without setting a default country
    3. Configure a view to use a custom date format - this will be a PHP format
    4. Set a default country
    5. Kaboom
  4. Just pointing out that this issue should probably bear in mind migration.

This blocks beta because making the intl date format transalatable involves schema change and if we suddenly make all default formats have both PHP and INTL formats then we have no upgrade path. I'm changing this to a meta to track these issues and leaving at beta blocking since the fixes to these will involve config schema changes, views config changes, and maybe significant API changes.

alexpott’s picture

Issue tags: +D8MI, +language-config

xpost

alexpott’s picture

Thinking about it we could also remove INTL support from core.

From the issue that added it #1802278: Add a Date component to core

There is a new PHP class called the IntlDateFormatter that allows you to specify locales and calendars for much better international date handling. Unfortunately, although that is available by default from 5.3 on, it is not enabled by default, and it seems to be unavailable in lots of fairly ordinary places. OSX doesn't have it enable, for instance. But it would be nice for sites that do have that class available to be able to use it, while still providing some sort of fallback for sites that don't.

Well as noted in #2031183: Improve test coverage for node authored on widget we have a significant test deficit if using INTL so whilst I agree it would be "nice" it certainly is not mandatory. And we could try to introduce properly in 8.1 or 9.

alexpott’s picture

Status: Active » Needs review
FileSize
48.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,542 pass(es), 9 fail(s), and 1 exception(s). View

So let's remove it and not have the headache of trying to support and switch between two different ways of formatting dates.

alexpott’s picture

FileSize
2.8 KB
48.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,545 pass(es), 9 fail(s), and 1 exception(s). View

There's more we can remove

catch’s picture

Title: [META] Date formats switching between intl / php is a mess » Date intl support is broken, remove it

This seems like a good plan. We can look at adding back the feature in a later release as Alex points out in #9.

Re-titling.

xjm’s picture

I think this sounds like a better direction, especially based on @Gábor Hojtsy's comment. So removing it would potentially resolve all four of these bugs? Thanks @alexpott.

alexpott’s picture

I've talked to @Gábor Hojtsy, @chx, @Berdir in IRC /gchat and no one is clear on the advantages of supporting two ways of formatting dates in core.

In order to support INTL date formatting in core we need to do a lot of work for no obvious gain:

  • make switching it on and off less painful
  • increase our test coverage to test all situations where PHP or INTL formats could be used
  • Change the configuration forms to always allow configuration of both PHP and INTL formats

Therefore +1 on repurposing this issue to remove INTL date formatting support.

xjm’s picture

FileSize
2.8 KB

Should we file a postponed followup to add it back when it works (maybe minor version target or against 9.x)?

Edit: I did not upload that file; d.o is being weird.

The last submitted patch, 10: 2276183.10.patch, failed testing.

Gábor Hojtsy’s picture

Totally agree removing it is better.

One of the definite problems of intl support is it uses wildly different date formatting patterns, so that eg. admins need to understand that in intl dd means two digit day, d means no leading zero, in PHP that is d and j. So depending on what you configure, you need to use totally different background info, and the switch was never apparent. This was even worse integrating with translation because the config form only showed one type but the translation form would have needed to support both.

Status: Needs review » Needs work

The last submitted patch, 11: 2276183.11.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.14 KB
51.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,599 pass(es). View

Fixed the test fails

Gábor Hojtsy’s picture

Here are two kicks at the old code and one suggestion for improvement.

  1. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
    @@ -91,26 +78,11 @@ class DateTimePlus extends \DateTime {
    -   * The value of the country code passed to the constructor.
    -   */
    -  protected $country = NULL;
    
    @@ -205,38 +177,7 @@ public static function createFromFormat($format, $time, $timezone = NULL, $setti
    -      // Construct the $locale variable needed by the IntlDateFormatter.
    -      $locale = $datetimeplus->langcode . '_' . $datetimeplus->country;
    

    This is one of the many problems of intl. It would use the only country Drupal knows about -- your default country -- to format all dates. So you would end up with nonsensical combinations like en_NL, it_NL, de_NL, etc. for a site in the Netherlands.

  2. +++ b/core/modules/config_translation/src/FormElement/DateFormat.php
    @@ -23,12 +23,7 @@ class DateFormat implements ElementInterface {
    -      $description = $this->t('A user-defined date format. See the <a href="@url">PHP manual</a> for available options.', array('@url' => 'http://userguide.icu-project.org/formatparse/datetime'));
    

    Not even the PHP manual. :P

  3. +++ b/core/modules/system/config/schema/system.schema.yml
    @@ -134,15 +134,8 @@ system.date_format.*:
    +      label: 'PHP date format'
    

    I think we just want to say 'Date format' here now? No need to qualify.

alexpott’s picture

3. Well it still is a PHP date format string :)

Gábor Hojtsy’s picture

Well, my point is it does not serve to give "any new info" and is technical. The form element for the date_format type already links the proper docs and previews the format with an actual date. Not a big issue though if we want to keep the label as-is.

alexpott’s picture

FileSize
330 bytes
51.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,608 pass(es). View

Sure np

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Gábor Hojtsy’s picture

FileSize
42.97 KB
51.38 KB

To illustrate the effect of the patch to the translate form. BEFORE the patch you have a list of date formats but intl is not supported in translation, so you only get the PHP format in a fieldset:

AFTER the patch, it says 'Date format'. I made it apparent where it used to say 'PHP date format' :D

It would probably be fine to remove "Date format" from the "Date format label" but that is not in scope of this issue. The help text for the field guides the user to the right docs already.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks very good. I cannot vouch for whether this covers 100% of references, but not sure how it is possible to figure out, we will see.

xjm’s picture

Thanks for the screenshots @Gábor Hojtsy.

A quick grep to check some possible references:

[tesla:drupal | Thu 11:30:29] $ grep -ri "intl" * | grep -v "vendor"
composer.lock:                "symfony/intl": "~2.3",
composer.lock:                "symfony/intl": "",
core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php:    // If the PECL intl extension is installed, use it to determine the proper
core/lib/Drupal/Core/DrupalKernel.php:        // It's pointless to persist services not yet initialized.
core/modules/migrate_drupal/src/Tests/d6/MigrateFileTest.php:    // It is pointless to run the second half from MigrateDrupal6Test.
core/modules/migrate_drupal/src/Tests/d6/MigrateNodeTest.php:    // It is pointless to run the second half from MigrateDrupal6Test.
core/modules/quickedit/js/views/EditorView.js:        // useful for the form-based in-place editor, but pointless for any
core/scripts/transliteration_data.php.txt: * - If you plan to use the 'intl' data, you will also need to have the PECL
core/scripts/transliteration_data.php.txt: *   packages 'yaml' and 'intl' installed.  See
core/scripts/transliteration_data.php.txt: *   will install yaml and intl packages:
core/scripts/transliteration_data.php.txt: *   sudo apt-get install php5-intl
core/scripts/transliteration_data.php.txt: *   sudo pecl install intl
core/scripts/transliteration_data.php.txt: *   'extension=intl.so' and 'extension=yaml.so' are added to the php.ini file
core/scripts/transliteration_data.php.txt: *   the PECL 'intl' package, and the line that needs to be un-commented in
core/scripts/transliteration_data.php.txt:// $data = read_intl_data();
core/scripts/transliteration_data.php.txt:// Command to patch Drupal Core data, using the intl data set, and put the
core/scripts/transliteration_data.php.txt:  $types = array('drupal', 'midgard', 'cpan', 'nodejs', 'junidecode', 'intl');
core/scripts/transliteration_data.php.txt:  // $types = array('drupal', 'intl');
core/scripts/transliteration_data.php.txt: * Reads in 'intl' transliteration data and writes out changed Drupal files.
core/scripts/transliteration_data.php.txt: * match the intl data set.
core/scripts/transliteration_data.php.txt:  $types = array('drupal', 'intl');
core/scripts/transliteration_data.php.txt:      // Note that for intl, we only want to keep the transliteration if it
core/scripts/transliteration_data.php.txt:      $intl = isset($data['intl'][$bank][$chr]) && $data['intl'][$bank][$chr] != '' ? $data['intl'][$bank][$chr] : NULL;
core/scripts/transliteration_data.php.txt:      if (!isset($intl)) {
core/scripts/transliteration_data.php.txt:      if (!isset($drupal) || $drupal != $intl) {
core/scripts/transliteration_data.php.txt:        $newdata[$chr] = $intl;
core/scripts/transliteration_data.php.txt: * Loads the PECL 'intl' Transliterator class's transliteration data.
core/scripts/transliteration_data.php.txt: * You need to have the PECL 'intl' package installed for this to work.
core/scripts/transliteration_data.php.txt:function read_intl_data() {

I don't think any of those are related, so the patch looks good to me as well.

xjm’s picture

I added a change record draft:
https://drupal.org/node/2276461

Gábor Hojtsy’s picture

Yeah transliteration is totally unrelated.

xjm’s picture

Issue summary: View changes
xjm’s picture

I think #2031183: Improve test coverage for node authored on widget and #2204207: Missing format field when translating a date format using php-intl. can also be marked as duplicates of this? Do we need to test that those issues are resolved with this patch applied? Edit: I'm not sure about #2031183: Improve test coverage for node authored on widget.

xjm’s picture

Issue summary: View changes
catch’s picture

Status: Reviewed & tested by the community » Fixed

Let's do this. At least it will unblock #2031183: Improve test coverage for node authored on widget which is the 11th oldest critical bug in 8.x at the moment.

Can always add it back in a non-broken way.

Committed/pushed to 8.x, thanks!

  • Commit 949e7fe on 8.x by catch:
    Issue #2276183 by alexpott: Fixed Date intl support is broken, remove it...
Gábor Hojtsy’s picture

Yay! Thanks!

xjm’s picture

Published the CR.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.