Simplify TimeZoneFormHelper

1. I played around with array_multisort() but it's a mystery how it works
2. Agree! I think this is a much better API. Added a patch that does this.

so maybe better to provide the 1-1 for now.

By this I assume you mean having both methods as in the last patch?

I think if we get this in before 10.1.x is released, we won't need a deprecation either way, can just update the CR from the other issue.

Yep agreed.

So just need more reviews now?

+++ b/core/modules/system/system.module
@@ -1113,7 +1113,7 @@ function system_mail($key, &$message, $params) {
   @trigger_error(__METHOD__ . '() is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. This function is no longer used in Drupal core. Use \Drupal\Core\Datetime\TimeZoneFormHelper::getOptionsList() or \DateTimeZone::listIdentifiers() instead. See https://www.drupal.org/node/3023528', E_USER_DEPRECATED);

We should update this deprecation notice to add a reference to getGroupedOptionsList().

Not sure it is worth deprecating getOptionsList() given that getGroupedOptionsList() needs to call it and contrib appears to use it, it is a nice separation of concerns as it is.

Fixes deprecation message.

Will a change record be needed for getGroupedOptionsList()?

Shouldn't grouped be deprecated also?

@smustgrave as this was only added recently in 10.1.x which is not yet released, it makes sense to add this extra method to the existing change record at https://www.drupal.org/node/3023528

Not sure what you mean by deprecating grouped, it is used in many places in core. The ungrouped version is not, but it is a nice separation of concerns.

The deprecation message is updated and there is nothing else to do here, so this is RTBC.

Tagging for change record update.

That’s correct sorry for not specifying I meant parameter

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Re-roll of #9

Back to RTBC

Random test fail

Another random failure.

Surprise! Random failure.

This looks like a really nice idea.

+++ b/core/lib/Drupal/Core/Datetime/TimeZoneFormHelper.php
@@ -34,29 +32,44 @@ public static function getOptionsList(bool $blank = FALSE, bool $grouped = FALSE
+  public static function getGroupedOptionsList(bool $blank = FALSE): array {

I think we can name this method a bit better. How about getOptionsListByRegion() or getOptionsListGroupedByRegion(). This would lead to more informative code. Discussed with @catch. We have a preference for the first option because it is more concise but maybe you'd expect that method to have a region argument. Maybe people have a better suggestion.

Ha ha. I think the current getGroupedOptionsList() is easier to understand, but not a hill worth dying on.

Here's a patch that renames to getOptionsListByRegion().

Committed c312cc1 and pushed to 10.1.x. Thanks!

Sorry if not the right place for this question but I am trying to modify the TZ list in D9.4. From the looks of things this is only possible by hacking core (like an alter function in the system_time_zones() function?). Is the work here going to make this any easier to accomplish in D10?

Hmm, thought i would be smart and hack the system.module to add an alter to allow modifying the $zones. All i do in my alter is remove all the non-America optgroups. It works perfectly but form submit complains it is not a valid option. This isn't standard Drupal form processing is it. I should be able to remove items from a select list without issue.

This is tagged for change record updates which still need to be done.

This issue should be added to the list of issues in the existing CR. And I see that the deprecation messages lists two alternate methods to use but only one is mentioned in the CR. The CR should have both and explain when to use one or the other. And examples are always useful.

Setting back to NW for the change record work.

Updated the CR.

@FeyP, Thanks! That is much more informative and easy to read.

I forgot to remove the tag.

Since I didn't have luck asking in Slack...

Shouldn't the CR be published? The documentation on writing CRs says

(change records are published when the change is committed)

so I would have expected this to happen on commit or in this case after the review. However maybe I'm missing something and we usually wait until release?

@FeyP, yes it usually happens on commit but sometimes a committer forgets or gets called away. You are welcome to publish a Draft CR if the issue is fixed and there are not outstanding items. As always, add a comment to the issue when doing so. Which I will do now. :-)

I have published the CR.

@quietone Thanks! I thought that it was just an oversight, but wasn't entirely sure. Will do in the future, if needed :)