Deprecate and remove usages of datetime_date_default_time().

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

?

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

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.

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

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?

This should pass now.

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

Working on this, but found a buglet that crept in via #2786583: Create a base class for DateTimeFieldTest and DateRangeFieldTest and move common code into it. It will likely cause a tiny merge conflict.

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.

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

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

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

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

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

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

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.

?

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

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

OK, here is new approach w/ tests.

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

Yup. Patch still applies, so should be a good novice task for someone.

One of the earlier patches accidentally added the old DateTimeTest back, so removing that file from the patch takes care of #44.

Added the CR to the deprecation message per #45.

Did another usage report and search in core for datetime_date_default_time and am not finding any new usages. Think this is good.