Currently Drupal core doesn't provide much help for date handling, nor even a date field. I've been working on a collection of patches that could make Drupal's core date handling much more robust, provide better ways for contrib modules to extend it, and hopefully lead to a date field in core.

This is the first patch I'd like to propose. This is the basis for everything else I hope we can do.

PHP has had a DateTime class since PHP 5.2. Prior to that it had only some pretty anemic date() and time() functions. The DateTime class is better for lots of reasons. It can accept various kinds of strings and create dates from them, like '10AM today', using functionality similar to the PHP strtotime() function. It is smart about timezone handling. The date() functions have no way to control timezone, but the DateTime class lets you specify it. At the moment, Drupal core uses the DateTime class in just a couple of places, but mostly ignores it.

That class, however, is still lacking a number of things that would make it more useful. It has a format() function, but that function can only create English date strings. It has no way to create a date from an array of date values, which would be very useful in form handling. It doesn't have a __toString() method. It is very lenient about interpreting dates, too lenient if you want to do validation of date inputs. For instance, it will take a date like '2/31/2012' and instead of treating that as an invalid date, it will turn it into '3/2/2012'.

Formatting date output is another issue. Drupal provides translation of formatted output in the format_date() class, but that still has some limitations. There is a new PHP class called the IntlDateFormatter that allows you to specify locales and calendars for much better international date handling. Unfortunately, although that is available by default from 5.3 on, it is not enabled by default, and it seems to be unavailable in lots of fairly ordinary places. OSX doesn't have it enable, for instance. But it would be nice for sites that do have that class available to be able to use it, while still providing some sort of fallback for sites that don't.

We have a custom extension of the DateTime object that we've been using in the Date API to fix some of those problems. I've done a lot of work to clean it up and improve it for D8, and I'd like to propose adding it to core in D8.

The patch actually creates two classes. First there is the DateObject class, which is direct extension of the DateTime class. It adds in the ability to accept various kinds of input values -- a string with an unknown format (the default behavior), a string with a known format (useful to make sure the date will be interpreted as intended since the parse can make mistakes about things like negative years), an array of date parts (useful for form handling), a timestamp, or another Date object.

The class incorporates a way to use the IntlDateFormatter for the format() method, if desired, and otherwise falls back to using the normal format() method. It also tightens up the leniency of the created dates so we can more easily validate the input.

The second class in this patch is an extension of the DateObject class to create a DrupalDate class. This basically extends the DateObject to tie it into Drupal's translation system. I've separated them into two classes since the main class could be useful outside of Drupal, and the second one is Drupal-specific. Then in Drupal we would use the DrupalDate class everywhere.

I have a follow up patch for this one that then changes format_date() to include the same settings array so users could optionally pass in the information needed by the IntlDateFormatter. Then instead of formatting the date in the previous way, it passes that settings array on to the date object so it can use the new format() functionality. I'll post that patch as a separate issue if this one is accepted.

I haven't included in this patch any other changes to core to use these classes, but that can be another follow up patch.

I hope patch will accomplish several things:

- Provide a robust way of handling dates that can be further extended in contrib.
- Provide a basis for doing better date form handling, both in core and in contrib.
- Provide part of the foundation needed to get a date field into core.

So here's my patch.

Files: 
CommentFileSizeAuthor
#104 1802278-104-date-component.patch64.92 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 47,660 pass(es).
[ View ]
#104 interdiff-1802278-97-104.txt47.27 KBLars Toomre
#97 1802278-date-component-97.patch63.14 KBKarenS
PASSED: [[SimpleTest]]: [MySQL] 47,667 pass(es).
[ View ]
#97 interdiff.txt1.22 KBKarenS
#92 1802278-date-component-92.patch63.08 KBKarenS
PASSED: [[SimpleTest]]: [MySQL] 46,951 pass(es).
[ View ]
#88 1802278-date-component-88.patch63.41 KBKarenS
PASSED: [[SimpleTest]]: [MySQL] 46,498 pass(es).
[ View ]
#88 component-interdiff.txt24.48 KBKarenS
#83 1802278-date-component-83.patch62.99 KBKarenS
PASSED: [[SimpleTest]]: [MySQL] 46,452 pass(es).
[ View ]
#69 1802278-core-datetime-69.patch63.17 KBKarenS
PASSED: [[SimpleTest]]: [MySQL] 46,384 pass(es).
[ View ]
#69 interdiff.txt6.24 KBKarenS
#66 1802278-core-datetime.patch63.83 KBKarenS
PASSED: [[SimpleTest]]: [MySQL] 42,880 pass(es).
[ View ]
#56 datetime2.patch66.32 KBKarenS
FAILED: [[SimpleTest]]: [MySQL] 42,618 pass(es), 14 fail(s), and 1 exception(s).
[ View ]
#56 interdiff.txt16.01 KBKarenS
#52 1802278-core-datetime-52.patch55.09 KBKarenS
PASSED: [[SimpleTest]]: [MySQL] 42,176 pass(es).
[ View ]
#52 interdiff.txt8.26 KBKarenS
#33 1802278-interdiff-23vs32.txt29.55 KBandypost
#32 1802278-core-datetime-32.patch55.1 KBKarenS
PASSED: [[SimpleTest]]: [MySQL] 42,360 pass(es).
[ View ]
#30 1802278-core-datetime-29.patch8.43 KBKarenS
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#28 1802278-core-datetime-28.patch8.43 KBKarenS
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#23 1802278-core-datetime-23.patch53.87 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 42,348 pass(es).
[ View ]
#23 1802278-interdiff-18vs23.txt1.78 KBandypost
#18 1802278-core-datetime-18.patch53.87 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 42,346 pass(es).
[ View ]
#18 1802278-interdiff-8vs18.txt24.6 KBandypost
#13 1802278-code-date-17.patch53.73 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 42,182 pass(es).
[ View ]
#13 1802278-interdiff-8vs17.txt5.93 KBandypost
#8 dateobject.patch53.65 KBKarenS
FAILED: [[SimpleTest]]: [MySQL] 42,176 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#2 dateobject.patch45.23 KBKarenS
PASSED: [[SimpleTest]]: [MySQL] 42,166 pass(es).
[ View ]
dateobject.patch45.9 KBKarenS
PASSED: [[SimpleTest]]: [MySQL] 42,169 pass(es).
[ View ]

Comments

arlinsandbulte’s picture

Status:Needs work» Needs review

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

KarenS’s picture

StatusFileSize
new45.23 KB
PASSED: [[SimpleTest]]: [MySQL] 42,166 pass(es).
[ View ]

Oops, had some typos in the php docs. Try again.

andypost’s picture

moshe weitzman’s picture

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.
KarenS’s picture

It doesn't add a variable, it uses a core variable that hasn't been converted yet. I can add in the conversion of format_date to use this, but that adds a bunch of code.

Lars Toomre’s picture

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

KarenS’s picture

OK, I'll work up a patch that has an implementation and post it soon.

KarenS’s picture

StatusFileSize
new53.65 KB
FAILED: [[SimpleTest]]: [MySQL] 42,176 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

OK, new patch with implementations. The object allows us to simplify the TypedData date type and format_date(). It also provides the error handling required by things like the node form date validation.

Status:Needs review» Needs work

The last submitted patch, dateobject.patch, failed testing.

mgifford’s picture

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

KarenS’s picture

I don't think there are any accessibility considerations with this, it is just the api for creating dates. As far as I can tell, all the linked issues are specific to implementation of the object in a date field, which is not what this patch is for. When we start creating date fields with this object we need to look at those. This is the raw object only, and some usage of that in core.

I don't have time right now to fix the broken test, but can try to do that later.

andypost’s picture

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

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new5.93 KB
new53.73 KB
PASSED: [[SimpleTest]]: [MySQL] 42,182 pass(es).
[ View ]

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

sun’s picture

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

@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

webchick’s picture

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

webchick’s picture

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.

andypost’s picture

StatusFileSize
new24.6 KB
new53.87 KB
PASSED: [[SimpleTest]]: [MySQL] 42,346 pass(es).
[ View ]

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

Lars Toomre’s picture

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.

andypost’s picture

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

Lars Toomre’s picture

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.

andypost’s picture

@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

andypost’s picture

StatusFileSize
new1.78 KB
new53.87 KB
PASSED: [[SimpleTest]]: [MySQL] 42,348 pass(es).
[ View ]

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

I'm in agreement with most of these changes. I just want to clarify that the patch uses both $calendar and $calendar_type. The IntlDateFormatter has a 'calendar' argument, but it's not the name of the calendar, it's basically a boolean to indicate whether or not the Gregorian calendar is used. If you want to use a non-gregorian calendar, you include it in the $locale string.

But we need to get core ready for other kinds of calendar handling where you need to know the calendar name, and that boolean value is not useful to anything but the IntlDateFormatter. So I split this up so you can provide both the calendar name and the boolean setting for the IntlDateFormatter as separate values. Then I add some helpers that construct the locale that the IntlFormatter wants unless something else has been input. So the real world use case for most people would be to provide just a calendar name and a format string in the right format and the code will do everything else that that is needed for the IntlFormatter. In the meantime, we now have an accepted way to pass a calendar name into a date class, so contrib modules that want to do more things with alternate calendars can do so.

As I now explain it, I suppose we could not even ask for the calendar_type variable and just deduce it from the calendar name, which would get rid of one variable.

KarenS’s picture

And to the question about $locale and $langcode. $langcode is our familiar Drupal variable for the name of a language. In this case, $locale is a string required by the IntlDateFormatter. It will look like:

So it is composed of the language, the country, and optionally a calendar. We know the language using $langcode. We know the site country. If a calendar name is passed in, we can add that. So we can construct the locale if it is not provided. But someone may want to use some other value for the locale, so we have to retain a way for it to be passed in. This code should make it possible to do either.

It is even more confusing because we have our own Locale module. I don't know what else I can do to make it more clear what is going on, though.

KarenS’s picture

One other question. If we call the component 'DateTimeBase', it makes sense from Drupal's perspective -- we are extending that to add our own translation system to it. But the idea behind making it a component is that it could be useful outside of Drupal. And for that purpose the name makes less sense. It sounds like a DateTime class with less functionality than the PHP class has, when it has more. It is more like 'DateTimeExtended'. For instance, I think Symfony could use this component. I see a number places in the Symfony code we include in core where it would make sense.

andypost’s picture

Issue tags:+D8MI

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

StatusFileSize
new8.43 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

I took a stab at making this more clear. I changed the name of the base class to DateTimePlus and removed the $locale setting and instead ask for $langcode and $country. Then we construct the local string from those values. That should remove some WTF.

Status:Needs review» Needs work

The last submitted patch, 1802278-core-datetime-28.patch, failed testing.

KarenS’s picture

Status:Needs work» Needs review
StatusFileSize
new8.43 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Wrong patch.

KarenS’s picture

Status:Needs review» Needs work

Wrong patch again :(

KarenS’s picture

Status:Needs work» Needs review
StatusFileSize
new55.1 KB
PASSED: [[SimpleTest]]: [MySQL] 42,360 pass(es).
[ View ]

OK, finally, this should be the right patch. I tried an interdiff but it's not very readable since it is dropping all the old files and adding in the renamed ones.

andypost’s picture

StatusFileSize
new29.55 KB

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

sun’s picture

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

andypost’s picture

@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

KarenS’s picture

I'm not a fan of the idea of naming the extension using the same name as the parent. I don't know of any other place In Drupal that we've done that, or anywhere else this has been done, for that matter. The idea is that this is a component that will work anywhere, we can't make any assumptions about how the core class might be used along side it. For that matter, the Symfony code we include in Drupal is still using the php DateTime class, so there is some possible ambiguity just within the Drupal code if we use the same class name for both.

KarenS’s picture

Issue tags:+Datein8

On the translation question, this is similar to the translation problems of timezone names. I think we can just provide a list of the phrases that need translation for the parser.

Gábor Hojtsy’s picture

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

andypost’s picture

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

Gábor Hojtsy’s picture

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 :).

andypost’s picture

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()

Gábor Hojtsy’s picture

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

The base component knows nothing about translation, so it just creates an array of errors. The subclass is where they should get translated, if at all. We can provide a list of values wrapped in t() for the messages that need translation as a helper somewhere. We've done that before.

These classes do not display the errors or do anything with them directly. The display of the errors is up to whatever code calls this object. In many cases they will never be displayed. The errors are mostly for debugging and so you can count to see if there are any errors for validation handling. So we can also just remove the remaining t()s completely.

KarenS’s picture

What I meant to say is that if there is any potential need for translation of the error messages supplied directly by this code, we can create a list of those exact messages wrapped in t() so they show up in the string translation system.

andypost’s picture

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

KarenS’s picture

I don't understand what you mean by disliking the in_array() pattern. This is because PHP reports a date like 2012-02-30 as a warning but not an error, and we need to flag that as an error. I do that by looking for the warning and basically making it an error instead.

andypost’s picture

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

andypost’s picture

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

KarenS’s picture

OK, I see. But we need to know if that warning was set somehow. Because it needs to be promoted to an error or we end up creating the wrong date with no warning that the created date does not match the entered one. Not sure how to handle that....

I think PHP's handling is wrong, but there is no automatic way to alter it. At the least the IntlDateFormatter has a setting for 'lenient', which wrongly defaults to lenient but can be changed.

andypost’s picture

Status:Needs review» Needs work
+++ 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

KarenS’s picture

Status:Needs work» Needs review
StatusFileSize
new8.26 KB
new55.09 KB
PASSED: [[SimpleTest]]: [MySQL] 42,176 pass(es).
[ View ]

OK, figured out something I hope will work correctly. It looks like the warning I am looking for is consistently returned using a key of 11, so I check for that warning number instead of the warning text. To clarify what is going on, I added a constant that identifies the warning number. I also removed the t()s and changed self:: to static::.

hass’s picture

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.

KarenS’s picture

If you have specific use cases you'd like to test I'll be glad to add the tests. I can't really figure out what you want to test here. If possible I'd like to move additional test coverage into a follow up issue and get this committed. I have a bunch of other work I'd like to get in that is blocked until this lands.

cosmicdreams’s picture

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

KarenS’s picture

StatusFileSize
new16.01 KB
new66.32 KB
FAILED: [[SimpleTest]]: [MySQL] 42,618 pass(es), 14 fail(s), and 1 exception(s).
[ View ]

Rerolling patch to catch up to head with a few changes:

- I added a bunch of additional timezone tests.
- I realized when I split out the new component that I didn't add back in a check for Drupal's site timezone or user timezone, so that is added to the DrupalDateTime class, and some tests to be sure this is working correctly.

The last change requires an explanation. There is one other type of timezone treatment that can be problematic when using the DateTime class. If you create a DateTime object using a string that contains either a timezone offset or timezone abbreviation, it will ignore any timezone name you try to supply and create a DateTime object with a timezone name of '+10:00' or 'EST'. Those values are ambiguous -- lots of timezones have the same offset or abbreviation. That means the resulting date can't really be relied on. The specific date you input may be correct, but if you try to use date_modify() it won't work right. Derick Rethans, the person behind most of the PHP DateTime functions, posted about this issue at http://derickrethans.nl/gmt-being-tricky.html. The best solution seems to be to see if the provided timezone name matches the offset or abbreviation passed in the string. If so, we can set the timezone name correctly. If not, we should flag this as an error. So I've added this functionality along with some tests for that.

Status:Needs review» Needs work

The last submitted patch, datetime2.patch, failed testing.

renat’s picture

Status:Needs work» Needs review

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?

renat’s picture

Status:Needs review» Needs work

Sorry.

KarenS’s picture

@renat, please see if this is actually a problem before changing the status. PHP is doing all the internal handling of dates. Nothing in this code should be creating a problem. If you can demonstrate that this code breaks some logical method of creating dates, please provide an example.

KarenS’s picture

Sorry, I see you didn't change the status, you just changed it back. But still, can you see if there is an actual problem?

KarenS’s picture

So the test failures are for places in core that we are creating dates without real timezones, like '1969-01-01 00:00:00 -0500'. The tests are fixed if we quit doing that and change it to something like '1969-01-01 00:00:00 America/Chicago'. Which IMO would be preferable anyway. I don't know how people feel about that change, but I can make that change or go back to allowing the broken offset-only dates that can't be safely modified.

KarenS’s picture

As to #58, I'm trying to understand the question. If you're asking if PHP understands that daylight savings time changed in Russia, it does, and there should be no problem. However it will only get daylight savings time right if you give it a fully qualified timezone name instead of an offset.

renat’s picture

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.

KarenS’s picture

@renat, yes, this the reason why we need to quit using timezone offsets and switch to using timezone names everywhere, which is part of what this whole patch is about. Timezone offsets do not provide enough information to handle dates correctly, but if we use timezone names, PHP has a historical database that can tell what offset applies when.

KarenS’s picture

Status:Needs work» Needs review
StatusFileSize
new63.83 KB
PASSED: [[SimpleTest]]: [MySQL] 42,880 pass(es).
[ View ]

Latest reroll. I got rid of the code that worries about offsets vs full timezones just to simplify this. That test is needed if you want to manipulate the dates, so anyone who wants to do that can handle it themselves. I also found that my previous hope that the warning message number for the invalid date warning was consistent was incorrect. There are several different warning message numbers. But I did find that basically anything that creates a warning is something we want to consider an error -- somehow a date is created but it doesn't exactly match the input. So I switched to just treating all warnings as errors.

I now have two follow up patches to this one, one to consolidate translation of date labels into a plugable calendar system and another one I'm getting ready to post that uses these two patches as the basis for a date field in core.

But let's confirm that this one is now green. I think it should be.

moshe weitzman’s picture

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

Wim Leers’s picture

Cowboy debugging!

:)

KarenS’s picture

StatusFileSize
new6.24 KB
new63.17 KB
PASSED: [[SimpleTest]]: [MySQL] 46,384 pass(es).
[ View ]

Latest re-roll. Included fixes suggested above, plus comments posted on #501428: Date and time field type in core. Hoping we can get this in soon so I can focus on #1811912: Add pluggable calendar backend to core and centralize date translation and then finally #501428: Date and time field type in core.

The comments included in #501428: Date and time field type in core include more debate about whether to use 'Drupal' in the class name. I'd like to put off that discussion to a follow up patch. The name can easily be changed later, after feature freeze, if there is agreement that it needs to be changed. I think the name makes sense, it is the Drupal version of the DateTimePlus class, that incorporates Drupal's localization system.

moshe weitzman’s picture

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

I changed the tests for DateTimePlus to use UnitTestBase. The test for DrupalDateTime is testing language handling and requires session management to test the language switching, so I don't think that one can use UnitTestBase.

KarenS’s picture

I meant to say it is testing user timezone preferences, not language switching, but I borrowed the method used by language switching tests to test the switch of timezone preferences.

moshe weitzman’s picture

@Karens - is there a new patch with UnitTestBase?

KarenS’s picture

@moshe --The patch in #69 uses UnitTestBase for the DateTimePlus tests. As I noted above the other tests, for DrupalDateTime can't use that.

mgifford’s picture

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

arlinsandbulte’s picture

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

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

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

moshe weitzman’s picture

No UI

mgifford’s picture

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.

Sylvain Lecoy’s picture

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

KarenS’s picture

The point of keeping them separate is that DateTimePlus can stand alone as a component that doesn't require Drupal and can be used elsewhere. DrupalDateTime adds in Drupal-specific handling for language, etc. to make it useful to Drupal.

The idea is that we're pulling out re-usable components into Drupal/Component, and Drupal-specific classes into Drupal/Core/.

Crell’s picture

Status:Reviewed & tested by the community» Needs work

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.

KarenS’s picture

Status:Needs work» Needs review
StatusFileSize
new62.99 KB
PASSED: [[SimpleTest]]: [MySQL] 46,452 pass(es).
[ View ]

The changing of the variable names to camelcase was a total fail. I didn't upload the file with all those changes. The tests were OK I think because they were used consistently. I also protected the variables and a few of the methods.

Sylvain Lecoy’s picture

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 ?

KarenS’s picture

I want DateTimePlus to be a re-usable, stand-alone component, and it cannot have Drupal methods in it. For Drupal we need to add in the Drupal methods. So we need two classes.

We still have variable_get for the items in the tests, they haven't been changed to CMI yet. When they are, we have to make that change here too, but until that we have to use the variables.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC

aspilicious’s picture

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new24.48 KB
new63.41 KB
PASSED: [[SimpleTest]]: [MySQL] 46,498 pass(es).
[ View ]

OK, updated docs.

aspilicious’s picture

Status:Needs review» Reviewed & tested by the community

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

catch’s picture

Status:Reviewed & tested by the community» Needs work

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?

KarenS’s picture

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.

Not really. The order of the testing is basically from things that are easiest to be sure of down to things we are less sure of, with a final fallback for everything else. We can easily tell if the input is a DateTime object or array, so we test that first and pull those possibilities out. If it isn't either of those, it might be a timestamp, and that's relatively easy to determine (is_numeric), so we check that next. If it's none of those but a format is provided, we can do this using the createFromFormat() method, so that's the next thing we can pull out. Everything else funnels into the parent class to see what it can make of things.

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

That's been in the code for quite a while. Basically if a date fails it is an ugly fatal failure that blows the page up even in a try/catch. We're checking for everything possible before trying that, but if it's broken at that point the error is ugly, so it's basically a last ditch effort to suppress any problems if the date can't be created. If I've succeeded in identifying every possible thing that can go wrong, it's not needed. But I'm not sure how to be sure of that.

Weird indentation here.

That's the indentation my code style sniffer wants. I didn't see any Drupal code standard that said differently, so that's the way I did it.

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

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()?

KarenS’s picture

Status:Needs work» Needs review
StatusFileSize
new63.08 KB
PASSED: [[SimpleTest]]: [MySQL] 46,951 pass(es).
[ View ]

How about this? I changed the indentation. I'm not sure there is anything else to fix.

Berdir’s picture

Status:Needs review» Needs work
+++ 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:

<?php
 
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.

Berdir’s picture

Status:Needs work» Needs review

Edit: Doube post.

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

KarenS’s picture

Status:Needs review» Reviewed & tested by the community

Marking this back to RTBC

catch’s picture

Status:Reviewed & tested by the community» Needs work

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:

<?php
$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:

<?php
$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:

<?php
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.

KarenS’s picture

Status:Needs work» Needs review
StatusFileSize
new1.22 KB
new63.14 KB
PASSED: [[SimpleTest]]: [MySQL] 47,667 pass(es).
[ View ]

OK, made those changes.

KarenS’s picture

Ugh, meant to say I changed the node creation item and I removed the @. I have added lots of additional ways to keep it from failing, and I can't find a way to break it right now, so maybe it's OK to leave it off now. If anyone does find a way to break it we can document that then.

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

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.

cosmicdreams’s picture

Status:Reviewed & tested by the community» Needs review

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.

KarenS’s picture

Status:Needs review» Reviewed & tested by the community

I think this is still RTBC, in spite of #100. If this can be committed I'll be glad to see what changes need to be made to the regional settings patch, if any. It would be much easier to get this in and then react to it in the other issue than to hold up both issues while we figure out how to patch them both simultaneously.

cosmicdreams’s picture

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.

sun’s picture

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

Would be great to move forward here.

Lars Toomre’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new47.27 KB
new64.92 KB
PASSED: [[SimpleTest]]: [MySQL] 47,660 pass(es).
[ View ]

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 '???'.

tim.plunkett’s picture

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

Lars Toomre’s picture

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.

sun’s picture

Status:Needs review» Reviewed & tested by the community

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

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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!

arlinsandbulte’s picture

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

catch’s picture

Title:Add a Date component to core» Change notice: Add a Date component to core
Category:feature» task
Priority:Normal» Critical
Status:Fixed» Active

We need a change notice for this. Also yay!

KarenS’s picture

I'll work on a change notice later today. Thanks everyone!!!

KarenS’s picture

Status:Active» Fixed

Change notice added: http://drupal.org/node/1834108.

Tor Arne Thune’s picture

Title:Change notice: Add a Date component to core» Add a Date component to core
Category:task» feature
Priority:Critical» Normal
fago’s picture

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

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

TonyScott’s picture

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!