Deprecate system_time_zones() and move to TimeZoneFormHelper

1. +++ b/core/lib/Drupal/Core/Datetime/DateHelper.php
   @@ -529,4 +529,48 @@ public static function dayOfWeekName($date = NULL, $abbr = TRUE) {
   +   * @param mixed $blank
   +   *   If evaluates true, prepend an empty time zone option to the array.
   ...
   +  public static function timeZones($blank = NULL, $grouped = FALSE) {
   

   Let's take the opportunity to make this the boolean that it is.

2. +++ b/core/lib/Drupal/Core/Datetime/DateHelper.php
   @@ -529,4 +529,48 @@ public static function dayOfWeekName($date = NULL, $abbr = TRUE) {
   +    $zones = $blank ? ['' => t('- None selected -')] : [];
   ...
   +      $zones[$zone] = t('@zone', ['@zone' => t(str_replace('_', ' ', $zone))]);
   
   +++ b/core/tests/Drupal/Tests/Core/Datetime/DateHelperTest.php
   @@ -128,4 +128,62 @@ public function providerTestWeekDaysOrdered() {
   +if (!function_exists('t')) {
   +
   +  function t($string, array $args = []) {
   +    return strtr($string, $args);
   +  }
   

   Let's not use t() we can use TranslatableMarkup objects.

3. +++ b/core/modules/system/system.module
   @@ -1262,8 +1263,15 @@ function system_mail($key, &$message, $params) {
    function system_time_zones($blank = NULL, $grouped = FALSE) {
   

   Let's call the new code here. So there are not two codebases. Also makes the lack of a deprecation test ok because we have no logic in this function then. It can cast the first param to a boolean.

I think moving system_time_zones() to DateHelper seems a bit odd. The docs for that class are:

 * Defines Gregorian Calendar date values.
 *
 * Lots of helpful functions for use in massaging dates, specific to the
 * Gregorian calendar system. The values include both translated and
 * untranslated values.

This method doesn't fit into that. How about introducing a TimeZoneHelper?

+++ b/core/modules/system/system.module
@@ -1262,8 +1263,15 @@ function system_mail($key, &$message, $params) {
+ * @see https://www.drupal.org/node/3020455
...
+  @trigger_error('system_time_zones() is deprecated in Drupal 8.7.0 will be removed before Drupal 9.0.0. This function is no longer used in Drupal core. Use \Drupal\Core\Datetime\DateHelper::timeZones() instead. See https://www.drupal.org/node/3020455', E_USER_DEPRECATED);

This needs to be a change record not an issue node ID

+++ b/core/lib/Drupal/Core/Datetime/TimeZoneHelper.php
@@ -0,0 +1,57 @@
+

Nit, extra space.

+++ b/core/modules/system/system.module
@@ -1262,42 +1263,16 @@ function system_mail($key, &$message, $params) {
-    if (preg_match('!^((Africa|America|Antarctica|Arctic|Asia|Atlantic|Australia|Europe|Indian|Pacific)/|UTC$)!', $zone)) {
-      $zones[$zone] = t('@zone', ['@zone' => t(str_replace('_', ' ', $zone))]);
-    }

It looks like the filtering is gone from TimeZoneHelper::getList() , and that this was essentially an unnecessary call since the goofy stuff is all in backzone now, and

+++ b/core/lib/Drupal/Core/Datetime/TimeZoneHelper.php
@@ -0,0 +1,57 @@
+   * Generate an array of time zones and their local time & date.

Since we are touch this, this comment is wrong. The function doesn't do anything with "their local time & date".

I'm wondering if is should read like

"Generate an array of time zones names.

This method retrieves the list of IANA time zones names that PHP is configured to use, for display to users. It does not return the backward compatible names (i.e., the ones defined in the backzone file)."

I'm also wondering if we should add a third parameter to allow people to get the ALL_BC list. This periodically comes up in issues.

Small nitpick:

+++ b/core/lib/Drupal/Core/Datetime/TimeZoneHelper.php
@@ -0,0 +1,63 @@
+   *   If TRUE, prepend an empty time zone option to the array.

Needs (optional) prefix.

One more thing...

+++ b/core/modules/system/system.module
@@ -1266,42 +1267,16 @@ function system_mail($key, &$message, $params) {
+  @trigger_error('system_time_zones() is deprecated in Drupal 8.7.0 will be removed before Drupal 9.0.0. This function is no longer used in Drupal core. Use \Drupal\Core\Datetime\TimeZoneHelper::getList() instead. See https://www.drupal.org/node/3023528', E_USER_DEPRECATED);

Needs to be 8.8.0 and format as per https://www.drupal.org/project/coding_standards/issues/3024461

Rather than adding a limited parameter only for ALL_BC, why not add $what = \DateTimeZone::ALL and $country = NULL as default parameters to the method so that any list that can be provided by \DateTimeZone::listIdentifiers() can be provided by this method?

Having another look at this issue...

+++ b/core/lib/Drupal/Core/Datetime/TimeZoneHelper.php
@@ -0,0 +1,63 @@
+  public static function getList($blank = FALSE, $grouped = FALSE, $all_bc = FALSE) {

Not a big fan of boolean flags for DX. I'd prefer explicit wrapper methods like getGrouped() getWithBc() or something like that. Thoughts?

Reconsidered my feedback in #17 and I think it's ok the way it is.

Marking as RTBC as this is a straight re-roll.

1. +++ b/core/lib/Drupal/Core/Datetime/TimeZoneHelper.php
   @@ -0,0 +1,63 @@
   +   * @param bool $blank
   +   *   (optional) If TRUE, prepend an empty time zone option to the array.
   ...
   +    $zones = $blank ? ['' => new TranslatableMarkup('- None selected -')] : [];
   
   +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -132,7 +133,7 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
   -      '#options' => ['' => $this->t('- Default site/user time zone -')] + system_time_zones(FALSE, TRUE),
   +      '#options' => ['' => $this->t('- Default site/user time zone -')] + TimeZoneHelper::getList(FALSE, TRUE),
   

   I wonder if we should consider ditching the $blank functionality here.

2. +++ b/core/modules/system/system.module
   @@ -876,7 +877,7 @@ function system_user_timezone(&$form, FormStateInterface $form_state) {
   -    '#options' => system_time_zones($account->id() != $user->id(), TRUE),
   +    '#options' => TimeZoneHelper::getList($account->id() != $user->id(), TRUE),
   

   I guess this is the bit that makes it useful... still it feels odd to be putting form considerations in this list. But then again it only really exists for forms. I think this is more of a TimeZoneFormHelper than a generic helper.

3. +++ b/core/lib/Drupal/Core/Datetime/TimeZoneHelper.php
   @@ -0,0 +1,63 @@
   +   * @param bool $all_bc
   +   *   (optional) Whether all timezones including backward compatible zones (
   +   *   \DateTimeZone::ALL_WITH_BC) should be returned.
   

   Why are we providing this option? It's not needed - and adds things to translate that never will be. I'm not convinced we should add this until necessary. The old function removed all of the BC timezones so not providing them is not a regression.

4. +++ b/core/modules/user/src/Entity/User.php
   @@ -573,7 +574,7 @@ protected function getRoleStorage() {
   -    return array_keys(system_time_zones());
   +    return array_keys(TimeZoneHelper::getList());
   

   See this is weird because we creating a load of TranslatableMarkup for no reason. This could be return \DateTimeZone::listIdentifiers();. I think the docs for the helper needs to point out that all the keys are valid timezone identifiers provided by \DateTimeZone::listIdentifiers().

1. Kept it for 2.
2. Renamed to \Drupal\Core\Datetime\TimeZoneFormHelper::getOptionsList() which I think makes more sense
3. Removed BC option, and removed test assertions
4. Replaced with \DateTimeZone::listIdentifiers(). Couldn't find any other instances.

1. +++ b/core/modules/system/system.module
   @@ -1286,42 +1287,16 @@ function system_mail($key, &$message, $params) {
   + * @deprecated in Drupal 8.8.0 and will be removed from Drupal 9.0.0. This
   + *   function is no longer used in Drupal core.
   + *   Use \Drupal\Core\Datetime\TimeZoneHelper::getList() instead.
   ...
   +  @trigger_error('system_time_zones() is deprecated in Drupal 8.8.0. It will be removed from Drupal 9.0.0. This function is no longer used in Drupal core. Use \Drupal\Core\Datetime\TimeZoneHelper::getList() instead. See https://www.drupal.org/node/3023528', E_USER_DEPRECATED);
   

   or \DateTimeZone::listIdentifiers() instead - I think the CR should tell people to use the builtin when only using the machine value. Also need to update the class name.

2. +++ b/core/modules/system/system.module
   @@ -1286,42 +1287,16 @@ function system_mail($key, &$message, $params) {
    function system_time_zones($blank = NULL, $grouped = FALSE) {
   

   Could do with a legacy test.

Why are we providing this option? It's not needed - and adds things to translate that never will be. I'm not convinced we should add this until necessary. The old function removed all of the BC timezones so not providing them is not a regression.

The BC list is really for systems where an old name is baked into existing config from a time when it was a valid name. They are not supposed to be used human facing.

Other than a little polish, looks good.

The BC list is really for systems where an old name is baked into existing config from a time when it was a valid name.

Yeah but

+++ b/core/modules/system/system.module
@@ -1286,42 +1287,16 @@ function system_mail($key, &$message, $params) {
-  $zonelist = timezone_identifiers_list();
-  $zones = $blank ? ['' => t('- None selected -')] : [];
-  foreach ($zonelist as $zone) {
-    // Because many time zones exist in PHP only for backward compatibility
-    // reasons and should not be used, the list is filtered by a regular
-    // expression.
-    if (preg_match('!^((Africa|America|Antarctica|Arctic|Asia|Atlantic|Australia|Europe|Indian|Pacific)/|UTC$)!', $zone)) {
-      $zones[$zone] = t('@zone', ['@zone' => t(str_replace('_', ' ', $zone))]);
-    }
-  }

This wasn't returning BC timezones anyway - right? So why do we need to add support for them? If you have them to deal with you need to write custom code atm - right?

The preg_match() has been doing absolutely nothing since PHP5.3 :D - see https://3v4l.org/nYJ2R

#23, sorry for not being clear. That was support your comment about not needing the BC flag. The backward/links discussion comes up every month or two on the TZ mailing list. The official docs more or less say don't use them directly anymore for new config: https://github.com/eggert/tz/blob/master/theory.html#L298 (search for "backward" not "backzone").

#24, yeah, did the same thing. I suspect I also checked this at #10 and there is a fiddle out there, lost for the ages.

Re: #21

1. Fixed
2. Updated CR

Created follow-up #3083441: Deprecate system_user_timezone()

#26 doesn't apply to 8.9.x. Re-rolling.

Minor merge conflict in the use section.

Patch no longer applies.

- Updated CR
- Rerolled latest patch in MR

@Spokje : Seems we were working simultaneously on this issue. Do Continue, I will pick another issue.

Thanks @Bhanu951, I wasn't aware you're working on it as well. Next time assign the issue to yourself, like I did, so I know somebody is already working on it.

RTBC+1 I worked on this issue too much to RTBC it myself.

Hiding old patch files to show that the MR is where the party is at.

move this logic either to DateHelper.

This doesn't seem to be in the MR

Left a small nit picky thing in the MR too.

Rebased on 10.1.x

Can the proposed solution be updated? Talks about moving to DateHelper but I don't see that?

Updated IS to refer to Drupal\Core\Datetime\TimeZoneFormHelper::getOptionsList

Thank you with sticking with me on this one. Everything looks good to me.

Committed/pushed to 10.1.x, thanks! I'll open the follow-up and link it from here.

Here's the follow-up #3343755: Simplify TimeZoneFormHelper.

How is this list overridden?

With the old system_time_zones() function i hacked in a patch to add an alter (which i could then use to alter the list). I assumed if this was being redone the ability to alter the list would be included; but i don't see it here. Am i correct that this class can not be overridden?

@liquidcms I suggest you create a new issue to add this feature.