Date intl support is broken, remove it

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?

@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 :/

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.

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.

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.

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.

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.

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.

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.

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.

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.

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

Yeah transliteration is totally unrelated.

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.

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!

Yay! Thanks!

Published the CR.