Problem/Motivation

The datetime module has one function it it that isn't a hook: datetime_date_default_time(). This function should not exist.

Proposed resolution

At the very least we should @deprecated it, but we would also need to provide the proper alternative. Ideally, the field should just handle this own so that the widgets/formatters/etc don't need to worry about this.

Usages in tests should be refactored out into something on DateTestBase, like a massageTestDate($date) method, to DRY this up.

Remaining tasks

Discuss.

User interface changes

API changes

Data model changes

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

The hunk(s) in #2829848: Random test failure in DateRangeFieldTest to DateTimeComputed should probably be sufficient for the field/widget/formatter usages.

?

mpdonadio’s picture

Status: Active » Postponed

Postponing on #2829848: Random test failure in DateRangeFieldTest, once we figure out the full scope of that.

mpdonadio’s picture

Status: Postponed » Active
mpdonadio’s picture

Status: Active » Needs review
FileSize
8.46 KB

OK, let's remove all non-test uses and see what happens! I think we may have fails in the default date coverage in the widgets. If not, then I think we have missing test coverage.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

arunkumark’s picture

Hi,

I have re-rolled the patch to Drupal 8.4.x

mpdonadio’s picture

+++ b/core/modules/datetime/datetime.module
@@ -50,6 +50,9 @@ function datetime_help($route_name, RouteMatchInterface $route_match) {
+ *
+ * @deprecated in Drupal 8.3.0 and will be removed before Drupal 9.0.0. Use
+ *   (TBD) instead.
  */

Needs to be updated for 8.4.0.

mpdonadio’s picture

The #5 and #7 patches appear to be identical.

Here is more work on it.

Thoughts on next steps? Thinking we need to centralize this behavior somewhere so we can update the (TBD), but not sure where? Static public method on DateTimeComputed?

Status: Needs review » Needs work

The last submitted patch, 9: 2830094-09.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
13.2 KB
2.06 KB

This should pass now.

jhedstrom’s picture

I love this cleanup!

+++ b/core/modules/datetime/datetime.module
@@ -50,6 +50,9 @@ function datetime_help($route_name, RouteMatchInterface $route_match) {
+ * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Use
+ *   (TBD) instead.

Is the TBD to be decided here? I don't think we can deprecate w/o a clear replacement, or example of what to use instead...

mpdonadio’s picture

#12, that's what has me stumped. I did the change in three parallel places in the patch (was in item from another patch, added in default value, added to test base class). I don't know where we should centralize this. My only thought is a static method on DateTimeItem or DateTimeComputed (static so it can be called from procedural code / hooks when needed), but those don't sit well with me.

jhedstrom’s picture

If we must replace it, then a static method makes sense...if not, just documenting what time should be set in the deprecated notice might work...

While reviewing I found one lingering mention of the function in Drupal\datetime\DateTimeComputed::getValue():

        // @see datetime_date_default_time()
jhedstrom’s picture

Status: Needs review » Needs work
mpdonadio’s picture

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
10.79 KB

Ok, here is a redo of #11. It still has the TBD. I'm leaning towards something like

public static function DateTimeItem::setDefaultTime() ($date) {
  $date->setTime(12, 0, 0);
}

and then weaving this through. That will make it accessible from object oriented and procedural code that may not be dealing with fields/items directly.

mpdonadio’s picture

Re-roll. Tiny merge conflict in the old DateTestBase.

mpdonadio’s picture

OK, here is an option for how to handle the deprecation.

The last submitted patch, 18: 2830094-18.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 19: 2830094-19.patch, failed testing.

jhedstrom’s picture

The test fails are due to fatals, which are due to the phpunit tests thinking they have this new massageTestDate method (which is only added on the legacy test base class). Which brings me to the question, why can't the tests just call the new \Drupal\Core\Datetime\DrupalDateTime::setDefaultDateTime() method instead?

mpdonadio’s picture

#22, I think I had a comment get swallowed. I think that method fell out during a reroll. I think adding that back to the new BTB test base would be a good idea, and then have that call DrupalDateTime::setDefaultDateTime() is a good idea.

mpdonadio’s picture

Here is the missing hunk. Should be all green again.

jhedstrom’s picture

+++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeWidgetBase.php
@@ -77,4 +69,28 @@ public function massageFormValues(array $values, array $form, FormStateInterface
+    // The date was created and verified during field_load(), so it is safe to
+    // use without further inspection.
+    if ($this->getFieldSetting('datetime_type') === DateTimeItem::DATETIME_TYPE_DATE) {
+      $date->setTime(12, 0, 0);
+    }

Can this also use the new method?

mpdonadio’s picture

FileSize
16.24 KB
2.69 KB

Good idea. Adjusted a few other things. PhpStorm still says no usages left of `datetime_date_default_time()`.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

I think this is good to go now.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeItem.php
@@ -136,4 +136,13 @@ public function onChange($property_name, $notify = TRUE) {
+  /**
+   * Sets the default time for a DrupalDateTime object for a date-only field.
+   *
+   * @param \Drupal\Core\Datetime\DrupalDateTime $date
+   */
+  public static function setDefaultDateTime($date) {
+    $date->setTime(12, 0, 0);
+  }

I would have thought this should have a type hint. Also I think think should be a static method on \Drupal\Core\Datetime\DrupalDateTime and not here because reaching into a plugin like this for common functionality is a bit odd.

Also why are we not copying

 * The default time for a date without time can be anything, so long as it is
 * consistently applied. If we use noon, dates in most timezones will have the
 * same value for in both the local timezone and UTC.
mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
17.7 KB
9.45 KB

Patch; more words to follow when I can use DrEditor on it.

mpdonadio’s picture

Ok, since we are deprecating something, I'll draft a short CR about it.

+++ b/core/modules/datetime/datetime.module
@@ -50,7 +51,11 @@ function datetime_help($route_name, RouteMatchInterface $route_match) {
+ *
+ * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Use
+ *   \Drupal\Core\Datetime\DrupalDateTime::setDefaultDateTime() instead.
  */
 function datetime_date_default_time($date) {
-  $date->setTime(12, 0, 0);
+  @trigger_error('datetime_date_default_time() is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Datetime\DrupalDateTime::setDefaultDateTime() instead.', E_USER_DEPRECATED);
+  DrupalDateTime::setDefaultDateTime($date);
 }

Where to move this was a head scratcher. I don't disagree with putting the method on \Drupal/Core/Datetime/DrupalDateTime. My main thinking for the field item was that this is really a field-specific activity.

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
@@ -132,4 +132,17 @@ public function format($format, $settings = []) {
+  /**
+   * Sets the default time for an object built from date-only data.
+   *
+   * The default time for a date without time can be anything, so long as it is
+   * consistently applied. If we use noon, dates in most timezones will have the
+   * same value for in both the local timezone and UTC.
+   *
+   * @param \Drupal\Core\Datetime\DrupalDateTime $date
+   */
+  public static function setDefaultDateTime(DrupalDateTime $date) {
+    $date->setTime(12, 0, 0);
+  }
+

This part also has a tiny odor, but I don't know what to do about it. The old global function would work on a \DateTime, DateTimePlus, and DrupalDateTime. This still will allow that because the hint isn't enforced (yay PHP?), though PhpStorm will complain. However, we can't use a normal invokaction like $date->setDefaultDateTime() since we really do need this to be a static to allow usage on all three types.

After writing all that out and posting the patch, I am wondering if we should make `Drupal/Core/Datetime/DrupalDateTime::setDefaultDateTime` a normal class method (maybe on DateTimePlus?), and keep the `$date->setTime(12, 0, 0);` in `datetime_date_default_time()` to make this cleaner in the long run.

?

jhedstrom’s picture

This still will allow that because the hint isn't enforced (yay PHP?)

Won't php fatal error if a type not matching the hint is passed? Testing locally I get this:

I was just looking at the 3 classes, assuming DateTimePlus extended \DateTime, but it doesn't, even though it has this in the docblock:

/**
 * Wraps DateTime().
 *
 * This class wraps the PHP DateTime class with more flexible initialization
...

If this new method really should take all 3 types, should we add unit tests to ensure that it does? We might also want a follow-up to actually make DateTimePlus extend the php class it purports to (or correct the docblock).

mpdonadio’s picture

Assigned: Unassigned » mpdonadio
Status: Needs review » Needs work
Issue tags: +Needs tests

#31, apparently I am an idiot :) and PHP has enforced typehints in arguments for a while...

The docblock on DateTimePlus is essentially correct: DTP wraps \DateTime in the sense that it has a protected property, $dateTimeObject that is a \DateTime. While it doesn't extend it properly, it does behave like a \DateTime in most cases because of the __call() magic method. It just cases weird inheritance problems, and is why PhpStorm never autocompletes a lot of things for DDT and DPT.

Setting back to NW to ponder this some more, but thinking that

After writing all that out and posting the patch, I am wondering if we should make `Drupal/Core/Datetime/DrupalDateTime::setDefaultDateTime` a normal class method (maybe on DateTimePlus?), and keep the `$date->setTime(12, 0, 0);` in `datetime_date_default_time()` to make this cleaner in the long run.

may be the best option.

mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: +needs iss
FileSize
16.81 KB
9.6 KB

Think this should be fully green. Still want to add some test coverage somewhere. Probably the existing DateTimePlusTest and DrupalDateTimeTest unit tests. Not 100% sure if we have a good home to add a test for datetime_date_default_time().

Will write CR when we agree on final approach (and update the IS to match).

mpdonadio’s picture

OK, here is new approach w/ tests.

I'll update IS and CR if we think this is good.

jhedstrom’s picture

I like this approach better than the static replacement method. I think this is RTBC once the CR and IS are updated.