Problem

In a YAML file, how should we escape a single quote character?

For example, in the default system.date.yml we use double quotes:

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

However when this is written to the active store during installation is changes use single quotes:

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

Proposed solution

Use the single quote to escape a single quote so that config is not changed at all during installation.

Files: 
CommentFileSizeAuthor
#22 1944636.php-intl.22.patch7.82 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 55,720 pass(es). View
#22 interdiff-21-22.txt959 bytesYesCT
#21 20-21-interdiff.txt571 bytesalexpott
#21 1944636.php-intl.21.patch7.83 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 53,993 pass(es). View
#20 17-20-interdiff.txt1.35 KBalexpott
#20 1944636.php-intl.20.patch7.83 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 54,088 pass(es). View
#17 1944636.php-intl.17.patch7.94 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 54,064 pass(es). View
#17 interdiff-16-17.txt2.43 KBYesCT
#16 9-16-interdiff.txt3.19 KBalexpott
#16 1944636.php-intl.16.patch7.9 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 54,072 pass(es). View
#13 2013-03-20_162508.png13.9 KBvijaycs85
#9 1944636.php-intl.9.patch10.5 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 53,806 pass(es). View
#5 1944636.yaml_.single.quote_.5.patch635 bytesalexpott
PASSED: [[SimpleTest]]: [MySQL] 53,337 pass(es). View

Comments

alexpott’s picture

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"
}
vijaycs85’s picture

Sorry for the wrong input @alexpott. here is the right one.

modules/system.date.yml

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

active/system.date.yml

  html_datetime:
    name: 'HTML Datetime'
    pattern:
      php: 'Y-m-d\TH:i:sO'
      intl: 'yyyy-MM-dd''Tkk:mm:ssZZ'
vijaycs85’s picture

Sorry for the wrong input @alexpott. here is the right one.

modules/system.date.yml

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

active/system.date.yml

  html_datetime:
    name: 'HTML Datetime'
    pattern:
      php: 'Y-m-d\TH:i:sO'
      intl: 'yyyy-MM-dd''Tkk:mm:ssZZ'
alexpott’s picture

Issue summary: View changes

Update issue to summary to reflect nature of issue

alexpott’s picture

Title: Yaml dumper escaping the escape character » Escaping a single quote in Yaml files can lead to differences between default and active config where there are none

Updating title to reflect nature of the issue.

alexpott’s picture

Status: Active » Needs review
FileSize
635 bytes
PASSED: [[SimpleTest]]: [MySQL] 53,337 pass(es). View

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

vijaycs85’s picture

Thanks for the patch @alexpott. I just tried it to test and found that this escaping stuff breaking the original functionality of the date format :( Here is the test:

formats.html_datetime.pattern.intl: 'yyyy-MM-dd''Tkk:mm:ssZZ'

When try to format a timestamp with this format(the same way, date_format() does):

  $format = config('system.date')->get('formats.html_datetime.pattern.intl');
  $date = new DrupalDateTime();
  // Getting yyyy-MM-dd''Tkk:mm:ssZZ in $format.
  $wrong_date = $date->format($format);

  // Hard coded format.
  $correct_date = $date->format('yyyy-MM-dd\Tkk:mm:ssZZ');

  var_dump($wrong_date);
  var_dump($correct_date);

We get below output:

string '13131313-MarMar-1818'ISTkk:0303:29291980019800' (length=46)

string '13131313-MarMar-1818Tkk:0303:29291980019800' (length=43)

Believe that the second string is the right format that we are expecting from configuration. Because of escaping of date and Yml are same and it is getting converted into something else?

vijaycs85’s picture

Status: Needs review » Needs work
alexpott’s picture

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
alexpott’s picture

Title: Escaping a single quote in Yaml files can lead to differences between default and active config where there are none » PHP intl usage in DateTimePlus class broken and untested
Component: configuration system » system.module
Priority: Normal » Critical
Status: Needs work » Needs review
Issue tags: +datetime
FileSize
10.5 KB
PASSED: [[SimpleTest]]: [MySQL] 53,806 pass(es). View

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.

alexpott’s picture

Title: PHP intl usage in DateTimePlus class broken and untested » PECL Intl extension usage in DateTimePlus class broken and untested

Fixing title

Status: Needs review » Needs work

The last submitted patch, 1944636.php-intl.9.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-base

Tagging for D8MI. Thanks for digging this up :)

vijaycs85’s picture

Status: Needs work » Postponed
FileSize
13.9 KB

2013-03-20_162508.png

Seems we need to enable PECL intl extension to test @alexpott's new test cases. Do we need to raise issue wit environment team?

UPDATE: Blocked by #1947688: PECL intl extension needs to be installed and enabled

rfay’s picture

Status: Postponed » Needs review

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

rfay’s picture

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

alexpott’s picture

FileSize
7.9 KB
PASSED: [[SimpleTest]]: [MySQL] 54,072 pass(es). View
3.19 KB

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

YesCT’s picture

FileSize
2.43 KB
7.94 KB
PASSED: [[SimpleTest]]: [MySQL] 54,064 pass(es). View
+++ 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.

arlinsandbulte’s picture

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'

YesCT’s picture

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.

alexpott’s picture

FileSize
7.83 KB
PASSED: [[SimpleTest]]: [MySQL] 54,088 pass(es). View
1.35 KB

@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.

alexpott’s picture

FileSize
7.83 KB
PASSED: [[SimpleTest]]: [MySQL] 53,993 pass(es). View
571 bytes

ugh...

YesCT’s picture

FileSize
959 bytes
7.82 KB
PASSED: [[SimpleTest]]: [MySQL] 55,720 pass(es). View
+++ 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.

Status: Needs review » Needs work
Issue tags: -datetime, -Configuration system, -D8MI, -language-base

The last submitted patch, 1944636.php-intl.22.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

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

alexpott’s picture

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

YesCT’s picture

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

arlinsandbulte’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this critical off the list!

catch’s picture

Status: Reviewed & tested by the community » Fixed

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

xmacinfo’s picture

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

Status: Fixed » Closed (fixed)

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

jaredsmith’s picture

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

jaredsmith’s picture

Issue summary: View changes

Fix some formatting issues and shorten for clarity.