Add a Date component to core

Just to make sure testbot is happy (so far).

Here's a related #51043: Add a "datetime" Form API element

Nice!

1. I don't know for sure, but I suspect we are no longer taking patches that add variable_* function calls.
2. Personally, I'd like to see one implementation go in with this patch (besides tests). Not a deal-breaker though.

I am very glad to see this proposal and agree with #4.2 that one implementation would help make acceptance easier.

There's a thread here about accessibility for date formats:
http://groups.drupal.org/node/19118

Also a number of existing issues for the date module (not sure if any code is reused, but still useful to consider):
http://drupal.org/project/issues/date?text=accessibility&status=All&prio...

Other related issues:
#1638078: [meta] Simplify complex form elements where possible
#1168246: Freedom For Fieldsets! Long Live The DETAILS.

The last patch looks like it almost got in, save:
Undefined property: Drupal\node\Node::$date

First review:
1) Exception handling seems needs discussions
2) Locale and $langcode - not sure I understand a difference but please clarify
3) A few minor code-style related issues

+++ b/core/lib/Drupal/Component/Datetime/DateObject.phpundefined
@@ -0,0 +1,703 @@
+   *   - string $locale
+   *     A locale name, using the pattern specified by the
+   *     intlDateFormatter class. Used to control the result of the
+   *     format() method if that class is available. Defaults to NULL.

It seems that $locale is a different thing then $langcode all over core now. Is it possible to specify difference somewhere?

+++ b/core/lib/Drupal/Component/Datetime/DateObject.phpundefined
@@ -0,0 +1,703 @@
+   *   - string $calendar
+   *     A calendar to use for the date, Defaults to NULL.

Maybe Gregorian should be set by default in case emty parameter?

+++ b/core/lib/Drupal/Component/Datetime/DateObject.phpundefined
@@ -0,0 +1,703 @@
+      $timezone_name = !empty($system_timezone) ? $system_timezone: 'UTC';

space needed after $system_timezone

+++ b/core/lib/Drupal/Component/Datetime/DateObject.phpundefined
@@ -0,0 +1,703 @@
+        throw new \Exception('The array contains invalid values.');
...
+        throw new \Exception('The date cannot be created from a format.');
...
+            throw new \Exception('The created date does not match the input value.');
...
+    if (!empty($errors['warnings']) && in_array('The parsed date was invalid', $errors['warnings'])) {
+      $this->errors[] = 'The date is invalid.';

+++ b/core/lib/Drupal/Core/TypedData/Type/Date.phpundefined
@@ -31,18 +32,17 @@ class Date extends TypedData implements TypedDataInterface {
+        throw new InvalidArgumentException("Invalid date format given.");

Why do not use t() here or just named exception?

+++ b/core/lib/Drupal/Core/Datetime/DrupalDate.phpundefined
@@ -0,0 +1,169 @@
+    // Attempt to translate the error messages.
+    foreach ($this->errors as &$error) {
+      $error = t($error);
...
+      $this->errors[] = t($e->getMessage());

Not sure it's a good idea to use t() this way

Fixed broken test and moved DrupalDate.php to Core/Component

moved DrupalDate.php to Core/Component

"DrupalDate" is not compatible with Drupal\Component. Make sure to read the Drupal\Component\README.txt.

You probably want a Drupal\Core\Datetime\Date that extends Drupal\Component\Datetime\Date.

Note that I intentionally removed "Object". In case "Date" is a reserved keyword, then I'd suggest to use "DateTime", which might be cleaner anyway as it self-documents what this class is about and what it extends from; i.e.:

\DateTime
  - Drupal\Component\DateTime\DateTime
    - Drupal\Core\DateTime\DateTime

@sun DateTime is reserved class name that we extending, so better do not use same names to avoid collisions with php namespace. Can we agree on current common practice to add 'Base' suffix:

Drupal\Component\DateTime\DateBase
Drupal\Core\DateTime\DrupalDate

EDIT I think better proceed with DateTimeBase and DrupalDateTime

Let's please not add "Drupal" to any class name, if we can help it; it's already implied. :)

Hm. While what I said in #16 is generally true (see http://drupal.org/node/608152), andypost explained that what we're doing here is very similar to what we do in drupal_strtolower and like functions, which is basically "native PHP thing but without all the suck." "DrupalDateTime" is similar; it's taking the native PHP DateTime class and including localization and proper error handling.

So, ignore me, and let's go with DrupalDateTime.

Patch with rename:
DateObject => DateTimeBase
DrupalDate => DrupalDateTime
- I think it's better because it self-documenting purpose and parents

Drupal\Component\DateTime\DateTimeBase
Drupal\Core\DateTime\DrupalDateTime

Also fix same minor code-style issues

Small nit... In this patch, $calendartype has been changed to $calendar_type.

Does it also make sense to do similar renaming for variables $datetype and $timetype so that there is consistency? I would think so from afar.

@Lars I think better follow param names from intldateformatter
And make it $calendar all over

Using $calendar all over makes sense @andypost.

Would it also make sense to document somewhere why $datetype and $timetype were used instead of $date_type and $time_type? As I understand the Drupal coding conventions, $date_type is the preferred variable name, yet it is not being used in this patch.

I am simply trying to make sure that we are consistent when someone else in D9+ comes back to look at this code.

@Lars it looks debatable because of #20, the IntlDateFormatter::create() parameters are $datatype and $timetype
So it looks strange if we pass $settings with our kind of parameters and then use PHP function with without underscore

So changed all local variables to use underscore
Suppose no reason to change them in $settings because they are mimic PHP's variable names

@KarenS Thanx a lot for deep clarification!
About class naming we can get never-endless bikesheding as always but the main reason to get this in. Suppose we can rename the class after freeze.

I think patch ready except error handling translatoin, so pushing it into D8MI team

@KarenS You could use git diff -M25% to get interdiff for renamed files, so here interdiff

Upfront, I'm sorry for only commenting on the class name instead of doing a proper/in-depth review, but don't have time for that today. ;)

DateTimePlus sounds similarly odd like DateTimeBase. Is there any reason we can't simply name it DateTime? (as in #14)

Code that is using this class certainly wants to use the user-space extension instead of the native/core class, so I don't think there is any problematic ambiguity involved (and all code is still able to explicitly reference \DateTime anyway).

@sun As I commented above having same class name could lead to unpredictable troubles for example when some code needs both classes. Also it's a common practice to give different name to overloaded class

What kind of values need translation here? Can you provide examples? It is not clear from the comments above.

Examples of exceptions:

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -0,0 +1,719 @@
+    try {
+      parent::__construct('', $this->input_timezone_adjusted);
+      $this->setTimestamp($this->input_time_adjusted);
+    }
+    catch (\Exception $e) {

This could generate a exception but exception message would be in server's locale

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -0,0 +1,719 @@
+      else {
+        throw new \Exception('The array contains invalid values.');
+      }
+    }
+    catch (\Exception $e) {
+      $this->errors[] = $e->getMessage();

This kind o fmessages we can translate but probably better to have own set on exceptions

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -0,0 +1,719 @@
+    try {
+      @parent::__construct($this->input_time_adjusted, $this->input_timezone_adjusted);
+    }
+    catch (\Exception $e) {

The resulting exception message language is unpredictable

If its in the server's locale first and foremost, then although we cannot 100% say it is English, I think most servers are set up with English default locale, no? (Honestly have not seen many server consoles in Estonia or Japan, just guessing :).

because we make no output of errors we should not translate then

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -0,0 +1,719 @@
+    if (!empty($errors['warnings']) && in_array('The parsed date was invalid', $errors['warnings'])) {
+      $this->errors[] = 'The date is invalid.';
+    }

this is really bad

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.phpundefined
@@ -0,0 +1,170 @@
+    // Attempt to translate the error messages.
+    foreach ($this->errors as &$error) {
+      $error = t($error);
...
+    catch (\Exception $e) {
+      $this->errors[] = t($e->getMessage());

And this places still should not use t()

The errors should be translated depending on where they go. If they go to watchdog/syslog, then they should be English as much as possible (despite we cannot affect the server locale). If they are displayed, then they should be translated to the display language needed. If it is displayed via the watchdog log later for example on screen, then it will be t()-ed then. It needs to be marked for the translation system though so it ends up localize.drupal.org. We do not have a marker outside of t()/format_plural() that you can use, but you can put t() strings at a no-op section (eg. in a method after a return, etc). Read #1542144: Mark strings as localizable/translatable (new t()-alike string function that isn't t(), only for potx) especially http://drupal.org/node/1542144#comment-5904790 for more background.

@KarenS so only #42 is needs to be fixed, I very dislike in_array() pattern here

@KarenS because this message depends on server's locale and could fail if one is different from English

There's a suggestion about exceptions #817160: Don't translate exceptions

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -0,0 +1,719 @@
+  const FORMAT   = 'Y-m-d H:i:s';
+  const CALENDAR = 'gregorian';
+  const PHP      = 'php';
+  const INTL     = 'intl';
...
+    $this->calendar = !empty($settings['calendar']) ? $settings['calendar'] : self::CALENDAR;
...
+    $format = self::FORMAT;
...
+      $this->input_time_adjusted = $this->input_time_adjusted->format(self::FORMAT);
...
+      $this->input_time_adjusted = self::prepareArray($this->input_time_adjusted, TRUE);
+      if (self::checkArray($this->input_time_adjusted)) {
...
+        $this->input_time_adjusted = self::arrayToISO($this->input_time_adjusted);
...
+    $array = self::prepareArray($array, $force_valid_date);
...
+      $input_time_adjusted = self::datePad(intval($array['year']), 4);
...
+        $input_time_adjusted .= '-' . self::datePad(intval($array['month']));
...
+          $input_time_adjusted .= '-' . self::datePad(intval($array['day']));
...
+      $input_time_adjusted .= self::datePad(intval($array['hour']));
...
+        $input_time_adjusted .= ':' . self::datePad(intval($array['minute']));
...
+          $input_time_adjusted .= ':' . self::datePad(intval($array['second']));
...
+  public static function prepareArray($array, $force_valid_date = FALSE) {
+    if ($force_valid_date) {
+      $array += array(
+                 'year'   => 0,
+                 'month'  => 1,
+                 'day'    => 1,
+                 'hour'   => 0,
+                 'minute' => 0,
+                 'second' => 0,
+                );
+    }
+    else {
+      $array += array(
+                 'year'   => '',
+                 'month'  => '',
+                 'day'    => '',
+                 'hour'   => '',
+                 'minute' => '',
+                 'second' => '',
+                );
+    }
+    return $array;
...
+  public static function datePad($value, $size = 2) {
+    return sprintf("%0" . $size . "d", $value);
...
+  function canUseIntl() {
+    return class_exists('IntlDateFormatter') && !empty($this->calendar) && !empty($this->langcode) && !empty($this->country);
...
+    $format_string_type = isset($settings['format_string_type']) ? $settings['format_string_type'] : self::PHP;
...
+      if ($this->canUseIntl() && $format_string_type == self::INTL) {
...
+        // If we have information about a calendar, add it.
+        if (!empty($calendar) && $calendar != self::CALENDAR) {
...
+        if ($calendar != SELF::CALENDAR) {

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.phpundefined
@@ -0,0 +1,170 @@
+    $format_string_type = isset($settings['format_string_type']) ? $settings['format_string_type'] : self::PHP;

Probably this should be static:: let's get PHP 5.3 late static binding

We should add some tests for 24h timezones. I have not seen any test for an EU / German time like 16:00 = 4:00 pm and this could cause validators to fail.

I'll take a shot at this test on Friday night.

if the provided timezone name matches the offset or abbreviation passed in the string. If not, we should flag this as an error.

And if timezone was reformed? For example, in Russia there are no more Daylight Saving Time since 2011. What about such cases? As far as Date field represented dates in UTC internally, it was OK - but will it be for now?

Sorry.

As to #58, I'm trying to understand the question.

Sorry I wasn't clear enough from the very beginning. I meant that because of reforms pair of timezone name - timezone offset can be false as of today standards. I.E. when particular entity was created, timezone XXX was GMT+3, and now that timezone is GMT+4. So I was interested, will we see correct date in such a case. If I understood #63 right, the answer is "yes", with some additional conditions.

Well done. Just a few nits for the next reroll.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -0,0 +1,739 @@
+   * @TODO
+   *   Potentially there will be additional ways to take advantage
+   *   of calendar in date handling in the future.
+   */

This seems vague enough that it can be removed

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -0,0 +1,739 @@
+      dsm('HERE');

Cowboy debugging!

+++ b/core/modules/system/lib/Drupal/system/Tests/Datetime/DateTimePlusTest.php
@@ -0,0 +1,436 @@
+
+use Drupal\simpletest\WebTestBase;
+use Drupal\Component\Datetime\DateTimePlus;
+use DateTimeZone;
+
+class DateTimePlusTest extends WebTestBase {

Looks to me like we can and should use UnitTestbase instead of WebTestBase

Cowboy debugging!

:)

Is there a reason we can't use UnitTestbase instead of WebTestBase? WebTestBase is much slower since it has to setup a new Drupal install. maybe pollution from the host environment?

@Karens - is there a new patch with UnitTestBase?

@KarenS - This is a huge patch. Really appreciate the work you've put into this.

I've applied the patch (git applied it nicely), but I don't know how to see if it is working or not.

Where is the UI changing? I had expected to add a date/time field to a content type, but couldn't see it.

Thanks.

@mgifford: The code here only adds the api to core.
Adding a date/time field to core is being handled in this issue: #501428: Date and time field type in core.

@mgifford - thats a follow-up patch thats in progress at #501428: Date and time field type in core. this one has ni UI changes. I reviewed the tests and am happy with this now.

No UI

Well, if there's no UI, it makes it hard to test.. :)

Thanks @moshe for sending me in the right direction & marking this RTBC. Date support is pretty critical.

Btw, this is only my opinion, but I think DateTimePlus can be merged with the drupal date time object.

Separating out the Drupal bits from the non-Drupal bits is a good practice, so this split is good. There's nearly no runtime cost, as inherited methods are basically free.

That said, there's still some bugs in the code:

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -0,0 +1,730 @@
+  /**
+   * The value of the time value passed to the constructor.
+   */
+  public $inputTimeRaw = '';

Drupal coding standards specify protected is preferred over public. Why are these public?

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -0,0 +1,730 @@
+    // We are finally certain that we have a usable timezone.
+    $this->input_timezone_adjusted = $timezone_adjusted;
+  }

There is no such property. It's been renamed to inputTimeAdjusted. Looks like there's a missing test if that isn't being caught.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -0,0 +1,730 @@
+  public function prepareFormat($format) {
+    $this->input_format_adjusted = $format;
+  }

Ibid.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -0,0 +1,730 @@
+  public function inputIsObject() {
+    return $this->input_time_adjusted instanceOf \DateTime;
+  }

Ibid.

I think because all three of these are wrong in the same way, it's passing tests because it's storing to a dynamic public property instead of a defined property above. That's why tests aren't catching it.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -0,0 +1,730 @@
+  public function inputIsTimestamp() {
+    return is_numeric($this->input_time_adjusted) && (empty($this->input_format_adjusted) || $this->input_format_adjusted == 'U');
+  }

Looks like this property is wrong, too. I won't mention more, but there are more of these in the patch.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -0,0 +1,730 @@
+  function hasErrors() {
+    if (count($this->errors)) {
+      return TRUE;
+    }
+
+    return FALSE;
+  }

This could also just return (boolean)count($this->errors); Doesn't have to change, but a nice minor cleanup.

+++ b/core/lib/Drupal/Core/TypedData/Type/Date.php
@@ -50,7 +50,7 @@ public function setValue($value) {
-    return (string) $this->getValue()->format(DateTime::ISO8601);
+    return (string) $this->getValue()->__toString();

This is redundant. Just casting to a string is sufficient.

The DrupalDateTime does not adds that much (only 2 methods and a different constructor) thus there is maybe no need for further fragmentation.

Also, I think you can also make DrupalDateTimeTest extending from UnitTest by manually setting the global conf array. Just a though, are we still using variable_get() at this point ?

Back to RTBC

Srry for spoiling the party. This needs a doc cleanup first.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -0,0 +1,733 @@
+  /**
+   * Implementation of __toString() for dates. The base DateTime
+   * class does not implement this.
+   *

Docs should start with a 3th person verb. And the summary shouldn't be longer than 80 chars. Underneath the summary you can write longer docs if needed.

Same goes for most of the docs.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -0,0 +1,733 @@
+   * @see https://bugs.php.net/bug.php?id=62911 and
+   * http://www.serverphorums.com/read.php?7,555645
+   */

I'm not sure we should use @see in this case. And if we use @see it should be two @see's and not one with an 'and' in between.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -0,0 +1,733 @@
+   * Create a date object from an array of date parts.

Like I said before this should be "CreateS". Please check the other fucntions as well.

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.phpundefined
@@ -0,0 +1,169 @@
+   *   @see http://us3.php.net/manual/en/datetime.createfromformat.php

Don't use @see in the middle of a docblock. In that case a normal "See" is more appropriate. @see elements should be placed on the bottom of the doc block. See node/1354 for more information.

KarenS you made my day :). NICE!
Based on the previous RTBC I'm moving it back to that state.

I couldn't find much to complain about, this looks great. I agree about the class split, nice to have something else in components. it's a shame user_get_timezone() isn't anywhere near close to injectable but that's going to require the $_SESSION changes and a bunch of other stuff so we can leave it here.

A few niggles follow:

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -0,0 +1,734 @@
+    // Create a date as a clone of an input DateTime object.
+    if ($this->inputIsObject()) {
+      $this->constructFromObject();
+    }
+
+    // Create date from array of date parts.
+    elseif ($this->inputIsArray()) {
+      $this->constructFromArray();
+    }
+
+    // Create a date from a Unix timestamp.
+    elseif ($this->inputIsTimestamp()) {
+      $this->constructFromTimestamp();
+    }
+
+    // Create a date from a time string and an expected format.
+    elseif ($this->inputIsFormat()) {
+      $this->constructFromFormat();
+    }
+
+    // Create a date from any other input.
+    else {
+      $this->constructFallback();

Would it be possible to figure out which of these is likely to happen most often? i.e. is there anywhere in the critical path that we're using this or are likely to, then we could put that check arbitrarily first so the others get skipped.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -0,0 +1,734 @@
+
+      if (empty($this->errors)) {
+        @parent::__construct($this->inputTimeAdjusted, $this->inputTimeZoneAdjusted);

Why the @ to suppress errors when we're also catching the exception?

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -0,0 +1,734 @@
+  public static function prepareArray($array, $force_valid_date = FALSE) {
+    if ($force_valid_date) {
+      $now = new \DateTime();
+      $array += array(
+                 'year'   => $now->format('Y'),
+                 'month'  => 1,
+                 'day'    => 1,
+                 'hour'   => 0,
+                 'minute' => 0,
+                 'second' => 0,
+                );

Weird indentation here.

+++ b/core/modules/node/node.moduleundefined
@@ -1025,7 +1026,8 @@ function node_submit(Node $node) {
-  $node->created = !empty($node->date) ? strtotime($node->date) : REQUEST_TIME;
+  $node_created = new DrupalDateTime(!empty($node->date) ? $node->date : REQUEST_TIME);

Do we really need to create a new DrupalDataTime() object with a timestamp, then call getTimestamp() on it?

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -0,0 +1,734 @@
+  public static function prepareArray($array, $force_valid_date = FALSE) {
+    if ($force_valid_date) {
+      $now = new \DateTime();
+      $array += array(
+                 'year'   => $now->format('Y'),
+                 'month'  => 1,
+                 'day'    => 1,
+                 'hour'   => 0,
+                 'minute' => 0,
+                 'second' => 0,
+                );

I think we usually do this:

  public static function prepareArray($array, $force_valid_date = FALSE) {
    if ($force_valid_date) {
      $now = new \DateTime();
      $array += array(
        'year'   => $now->format('Y'),
        'month'  => 1,
        'day'    => 1,
        'hour'   => 0,
        'minute' => 0,
        'second' => 0,
      );

So it's just one level deeper, and doesn't depend on where array( starts.

Edit: Doube post.

Edit2: And a crosspost too. Looks correct in the patch.

This is an alternative to using strtotime(), which is a much less robust way to do things. The strtotime() function can not interpret dates nearly as well as the DateTime class, especially dates that are very old or far in the future. The DateTime class can do everything strtotime() can do and much more (and more reliably). Are you suggesting going back to strtotime()?

Sorry I didn't mean going back to strotime(), the patch currently does this:

-  $node->created = !empty($node->date) ? strtotime($node->date) : REQUEST_TIME;
+  $node_created = new DrupalDateTime(!empty($node->date) ? $node->date : REQUEST_TIME);
+  $node->created = $node_created->getTimestamp();

This means we have this possible code path:

$node_created = new DrupalDateTime(REQUEST_TIME);
$node_created = $foo->getTimestamp();

REQUEST_TIME is already a timestamp, so we don't need to do that, it can be:

if (!empty($node->date)) {
  $node_created = new DrupalDateTime($node->date);
}
else {
  $node->created = REQUEST_TIME;
}

With the @ call, please add some docs to explain why it's necessary, ideally we'd also have a link to the PHP bug report if there is one. We have #1247666: Replace @function calls with try/catch ErrorException blocks open which might allow the error to be caught but depends how fatal it is I suppose and that patch isn't in yet.

I think that addresses catch's concerns.

Removing the @ given that there are no known cases in which this can happen sounds good to me.

Just wanted to add a note that I'm aware that if this patch gets in I'll have to rework parts of #1571632: Convert regional settings to configuration system, but in my view this component approach is much preferred.

We previously setup the system.date.yml config file to allow for the persistence of multiple date format patterns. Given how DateTimePlus->format() depends on a proper $settings array, we'll just need to modify the regional CMI patch to populate the settings appropriately.

If there's somewhere in this patch where you're already doing that please point me to it. I think we're really close here.

Whoops, didn't mean to change the status. I can mark #1571632: Convert regional settings to configuration system as postponed if that helps get this one in.

This looks great to me. Excellent work, @KarenS! :)

Would be great to move forward here.

Attached is a patch that cleans up the documentation and inline code comments. An interdiff is also included.

Much of this included re-wrapping text to better use 80 character limit.

Where I was unsure of what needed or expected, I used the text string '???'.

+++ b/core/includes/common.incundefined
@@ -5,8 +5,8 @@
-use Drupal\Core\Datetime\DrupalDateTime;
 use Drupal\Core\Database\Database;
+use Drupal\Core\Datetime\DrupalDateTime;

Why did you change these lines? Please stop trying to enforce your personal coding standards on every patch in the queue.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -2,49 +2,66 @@
+ * Contains Drupal\Component\Datetime\DateTimePlus

This is STILL not correct according to the coding standards. But, that's coding standards not docs.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -2,49 +2,66 @@
+   * ???
...
+   * ???
...
+   * ???
...
+   * ???

That doesn't really help much. But these are the only parts that need to get fixed before this goes in, the rest is a follow-up.

According to the work on-going in #1446366: [meta] Multiple web test classes mislabeled as unit tests. simpletest ([Web|Unit|DrupalUnit]TestBase), DateTimePlusTest.php should be renamed to DateTimePlusUnitTest.php.

@Lars Toomre: Can you remove all new lines that contain "???" from #104, please?

Otherwise, let's go ahead and commit #97 and apply/commit the interdiff of #104 (or any newer/corrected) afterwards.

Back to RTBC for that.

AWESOME work!!

Looks like catch's concerns have been addressed. We are above thresholds by a couple of major bugs, but I think it's strategically important to get this in, so we can continue to build off of it until feature freeze.

Committed and pushed to 8.x. Thanks!

Karens++
Karens++
Karens++
Karens++
Karens++
Karens++
Karens++
Karens++

We need a change notice for this. Also yay!

I've created a related follow-up, please see #1844956: Optimize date formatting performance.

I don't know if this has been dealt with (I haven;t had a chance to read through all the comments) but the example given in an early comment, that the validation isn't or wasn't catching an invalid date such as 2/21/2014 misses the point that this is a VALID European date.

I was really hoping the codesprint in Kiev this weekend would solve the problems preventing me from creating a select by date range interface for my users, but it doesn't look like they even got to it. So I'm putting in a plea here: PLEASE, yes, a very robust date function is NECESSARY in core for D8!