PECL Intl extension usage in DateTimePlus class broken and untested

Hmmm very weird... initial investigations have not lead to a cause...

Using the following script with drush scr

use Symfony\Component\Yaml\Yaml;

$yaml = new Yaml;

$label = array(
  'intl' => 'yyyy-MM-dd\Tkk:mm:ssZZ',
);

var_dump($yaml->dump($label));

$str = file_get_contents('a.yml');
var_dump($str);
var_dump($yaml->parse($str));

And a.yml contents being:

intl: 'yyyy-MM-dd\Tkk:mm:ssZZ'

Does not elicit this error... hmm... :(

Output:

string(31) "intl: 'yyyy-MM-dd\Tkk:mm:ssZZ'\n"
string(31) "intl: 'yyyy-MM-dd\Tkk:mm:ssZZ'\n"
array(1) {
  'intl' =>
  string(22) "yyyy-MM-dd\Tkk:mm:ssZZ"
}

Update issue to summary to reflect nature of issue

Updating title to reflect nature of the issue.

The only yaml file I could find with this issue so far is system.date.yml

Hmmm.. actually what we are looking at here is are bugs! The intl format is supposed to be equivalent to the php format... so...

  html_datetime:
    name: 'HTML Datetime'
    pattern:
      php: 'Y-m-d\TH:i:sO'
      intl: "yyyy-MM-dd'Tkk:mm:ssZZ"
    locked: 1

looks wrong... looking at http://userguide.icu-project.org/formatparse/datetime I think it should be...

      intl: "yyyy-MM-dd'T'HH:mm:ssZZ"

And

     name: 'HTML Week'
     pattern:
       php: 'Y-\WW'
       intl: "Y-'WW"

should be

      intl: "Y-'W'W"

In order to test this you need to install intl extension from PECL

Matched packages, channel pecl.php.net:
=======================================
Package Stable/(Latest) Local
intl    3.0.0b1 (beta)        Internationalization extension

Hmmm... this is actually part of a much larger critical bug. The DateTimePlus class's use of PECL intl extension is completely broken and un-tested.

If a server had the extension installed and the system.date:country.default configuration set dates would be broken due to passing the format string into IntlDateFormatter::format() and not the date object!!!

After getting the pecl extension installed I realised that none of tests can possible satisfy the conditions checked in DateTimePlus::canUseIntl()

Patch attached fixes this but I think more work needs to be done to test that our usage of the PECL intl extension meets Drupal's expectations. For now I've added a test that (if the extension is installed) confirms that the default formats supplied by system.date.yml match regardless of whether intl or php format strings are used.

Fixing title

Tagging for D8MI. Thanks for digging this up :)

#9: 1944636.php-intl.9.patch queued for re-testing.

Ok, unblocked. Thanks for all your work on this important task and for your patience.

Fixed up some stuff in the new test and removed a hunk of unrelated changes that somehow crept into the patch.imo this is rtbc

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -637,9 +637,25 @@ public static function datePad($value, $size = 2) {
+   * @return bool
+   *   True if IntlDateFormatter can be used.

nit, TRUE
http://drupal.org/node/1354

+++ b/core/modules/system/config/system.date.ymlundefined
@@ -24,7 +24,7 @@ formats:
-      intl: "yyyy-MM-dd'Tkk:mm:ssZZ"
+      intl: 'yyyy-MM-dd''T''kk:mm:ssZZ'

@@ -48,7 +48,7 @@ formats:
-      intl: "Y-'WW"
+      intl: 'Y-''W''WW'

are we making the change from double quote to single because that's a standard ... or because that's what the config writer writes out? #1948284: [policy no patch] config save format and default yml file format and coding standards
These double single quotes look strange.

+++ b/core/modules/system/lib/Drupal/system/Tests/Datetime/DateTimePlusIntlTest.phpundefined
@@ -0,0 +1,88 @@
+class DateTimePlusIntlTest extends DrupalUnitTestBase {

class needs a doc block
http://drupal.org/coding-standards/docs#classes

+++ b/core/modules/system/lib/Drupal/system/Tests/Datetime/DateTimePlusIntlTest.phpundefined
@@ -0,0 +1,88 @@
+  /**
+   * Test information.
+   */
+  public static function getInfo() {

getInfo does not get a docblock.

http://drupal.org/node/325974
"There is no PHPDoc on this function since it is an inherited method."

+++ b/core/modules/system/lib/Drupal/system/Tests/Datetime/DateTimePlusIntlTest.phpundefined
@@ -0,0 +1,88 @@
+      'description' => 'Test DateTimePlus PECL Intl functionality.',
...
+   * Ensures that the PECL intl extension is installed.

I made it consistantly PECL Intl. I tried to google to see if it was intl or Intl outside of Drupal, but I'm not sure.

+++ b/core/modules/system/lib/Drupal/system/Tests/Datetime/DateTimePlusIntlTest.phpundefined
@@ -0,0 +1,88 @@
+  /**
+   * Set up required modules.
+   */
+  public static $modules = array('system');
+
+  protected function setUp() {
+    parent::setUp();

it seemed odd to me to see modules inbetween getInfo and setUp... I checked in Views and Entity Translation tests to see what might be usual, and it looks like the order is usually: $modules, getInfo(), setUp()

I'll put this back though if I shouldn't have made this reorder.

+++ b/core/modules/system/lib/Drupal/system/Tests/Datetime/DateTimePlusIntlTest.phpundefined
@@ -0,0 +1,88 @@
+    // Set the default country. It is a requirement for using
+    // config('system.date')->set('country.default', 'GB')->save();

why is there a comment saying the default country is going to be set... but no code setting the default country?

+++ b/core/modules/system/lib/Drupal/system/Tests/Datetime/DateTimePlusIntlTest.phpundefined
@@ -0,0 +1,88 @@
+  }
+}

nit, classes get an empty line before the last curly brace.

http://drupal.org/node/1353118
... might not actually be standard. hard to find a reference to it.

Should this even be called the 'PECL' Intl extension?
According to http://drupal.org/node/1947688#comment-7232058 it is included with php now.
Perhaps it could better be described as the 'php5-intl package'

answering my own question, I applied the patch, installed the site and checked sites/default/files/config_sr16xvf6UHg6JTX1XgkgHqv9Uremgx6s6x6g4MQzXFw/active/system.date.yml
and the single quoting matches what is in this patch for those changed lines.

@arlinsandbulte you're right... it should be PHP's Intl extension... copying what they do here http://www.php.net/manual/en/intro.intl.php

Re-rolled to remove some commented out code and incorrect references to PECL.

ugh...

+++ b/core/modules/system/lib/Drupal/system/Tests/Datetime/DateTimePlusIntlTest.phpundefined
@@ -0,0 +1,88 @@
+  protected function setUp() {

this should be public:
http://drupal.org/node/325974

+++ b/core/modules/system/lib/Drupal/system/Tests/Datetime/DateTimePlusIntlTest.phpundefined
@@ -0,0 +1,88 @@
+    // Create date object from a unix timestamp and display it in
+    // local time.

and this can wrap to 80 chars.

---
I looked at this change:
- 'PECL Intl extension needs to be installed and enabled.',
+ 'PHP\'s Intl extension needs to be installed and enabled.',

and http://drupal.org/coding-standards#quotes
says:
since it's not a translated string that keeping it in single quotes is ok.

So this looks ok to me.

#22: 1944636.php-intl.22.patch queued for re-testing.

#22: 1944636.php-intl.22.patch queued for re-testing.

#22: 1944636.php-intl.22.patch queued for re-testing.

Let's get this critical off the list!

Looks good to me. Committed/pushed to 8x, thanks!

Can we get a follow up to display on the Status Report page (admin/reports/status) if the PECL Intl extension is installed?

This seems to have popped up again, as reported in http://drupal.org/node/2000384

Fix some formatting issues and shorten for clarity.