Problem/Motivation

I was trying to improve the code coverage on DateTimePlus under PHPUnit, and discovered this:

    foreach (array('hour', 'minute', 'second') as $key) {
      if (array_key_exists($key, $array)) {
        $value = $array[$key];
        switch ($value) {

It's switching on $value instead of $key.

This leads the function to always use the default switch code path, validating against the minute/second regex instead of the hour one.

Proposed resolution

Switch on $key.

Remaining tasks

User interface changes

API changes

#2084477: Expand phpunit tests for \Drupal\Component\Datetime\DateTimePlus

CommentFileSizeAuthor
#14 interdiff.txt2.06 KBMile23
#14 2084455_14.patch6.1 KBMile23
#11 2084455_11.patch5.42 KBMile23
#11 2084455_11_test_only.patch3.81 KBMile23
#8 2084455_8.patch4.01 KBMile23
#8 2084455_8_test_only.patch3.38 KBMile23
#3 2084455_checkArray_3.patch921 bytesMile23
#1 2084455_checkArray.patch648 bytesMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
FileSize
2084455_checkArray.patch648 bytes

And a patch.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Active » Needs review
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
FileSize
2084455_checkArray_3.patch921 bytes

Better yet...

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Issue summary: View changes

Added link to test issue

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Component: system.module » base system
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Issue summary: View changes

better link to test

mgifford queued 3: 2084455_checkArray_3.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2084455_checkArray_3.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Issue summary: View changes
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs work » Needs review
FileSize
2084455_8_test_only.patch3.38 KB
2084455_8.patch4.01 KB

This patch combines the code change with a regression test to illustrate it.

I discovered that DateTimePlusTest::providerTestInvalidDateArrays() isn't hooked up to any test, so I added regression testing for this issue, as well as folding in the existing data.

I had to delete one of the invalid data arrays, because apparently the year 10,000 is a valid year in PHP now. :-)

Ignore the previous patches please. :-)

The last submitted patch, 8: 2084455_8_test_only.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented 
  1. +++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
@@ -61,6 +61,23 @@ public function testDateArrays($input, $timezone, $expected) {
+    $this->assertNotNull(
+      DateTimePlus::createFromArray($input, $timezone)
+    );

    Is this assert needed? Is it just there to provide output about the failure? 

  2. +++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
@@ -336,28 +353,25 @@ public function providerTestInvalidDates() {
-      array(array('year' => 10000, 'month' => 7, 'day' => 8, 'hour' => 8, 'minute' => 0, 'second' => 0), 'America/Chicago', NULL, "array('year' => 10000, 'month' => 7, 'day' => 8, 'hour' => 8, 'minute' => 0, 'second' => 0) contains an invalid year and did not produce errors."),

    I don't feel strongly about it but this is could still sort of a valid test. The value should be 32768 though. 32767 is the documented upper limit of checkdate().

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
FileSize
2084455_11_test_only.patch3.81 KB
2084455_11.patch5.42 KB

Historically, 10,000 is outside the allowable date range for dates, leading to the year-10000 problem. PHP fixed this, so 10000 won't throw an exception.

Good catch on checkdate() though, and on the assertNotNull(). It should be assertInstanceOf() for DateTimePlus, which is a more specific expectation. If createFromArray() doesn't throw an exception then we get an immediate indication of what it did in the fail message.

The last submitted patch, 11: 2084455_11_test_only.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs review » Needs work

Sorry, a couple documentation nits I missed in the last review

  1. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -112,9 +112,18 @@ public static function createFromDateTime(\DateTime $datetime, $settings = array
+   *   PHP DateTimeZone object, string or NULL allowed.
+   *   Defaults to NULL.

    This could fit on a single line I'd wager. 

  2. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -112,9 +112,18 @@ public static function createFromDateTime(\DateTime $datetime, $settings = array
+   *   A keyed array for settings:
+   *   - langcode: (optional) String two letter language code used to control
+   *     the result of the format(). Defaults to NULL.
+   *   - debug: (optional) Boolean choice to leave debug values in the
+   *     date object for debugging purposes. Defaults to FALSE.

    I really dislike the settings array. If we're going to add documentation can we just say we pass it to the constructor? From what I've been able to piece together, debug isn't used anywhere ever and langcode is only used on the Core version of the class so that's the most accurate we can be here.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs work » Needs review
FileSize
2084455_14.patch6.1 KB
interdiff.txt2.06 KB

If we're going to add documentation can we just say we pass it to the constructor? From what I've been able to piece together, debug isn't used anywhere ever and langcode is only used on the Core version of the class so that's the most accurate we can be here.

That's not how documentation works. :-)

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 357b503 and pushed to 8.0.x. Thanks!

  • alexpott committed 357b503 on 8.0.x 
    Issue #2084455 by Mile23: Drupal\Component\Datetime\DateTimePlus::...

Status: Fixed » Closed (fixed)

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