Simplify TimeZoneFormHelper

Couple more thoughts:

1. in the grouped case, would it be worth trying to use array_multisort() on the resulting array and skipping the initial asort(), didn't try this yet, might not be easier to read.

2. Should we split the method into getOptionsList() and getGroupedOptionsList() or something? But then we'd probably want a protected method for the shared logic, which means three methods instead of one, maybe that's fine though.

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.

Looking at #4, we now don't have a single call to ::getOptionsList(), so do we even need both versions? But grepping contrib it looks like it's used a lot with the default arguments, so maybe better to provide the 1-1 for now.

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.

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?

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

I'm on the fence, but I think that means we could try to get this in and then open yet another follow-up to look at deprecating one if we want to.

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

Fixes deprecation message.

The last submitted patch, 9: 3343755-9.patch, failed testing. View results →

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.

I think @smustgrave is maybe referring to the @grouped parameter, but since the method was only added to 10.1 a week or so ago, we don't need to deprecate as long as this gets committed before 10.1.x beta1 since the change will only be from 10.0.x and earlier which doesn't have the method.

That’s correct sorry for not specifying I meant parameter

Updated the issue summary a bit.

Still small but slightly less small than when I opened the issue.

The last submitted patch, 20: 3343755-20.patch, failed testing. View results →

Random test fail

The last submitted patch, 20: 3343755-20.patch, failed testing. View results →

Another random failure.

The last submitted patch, 20: 3343755-20.patch, failed testing. View results →

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

#30 looks good. I don't think there's that much in it, but I do prefer it slightly over the other options.

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.

I also went to update the CR but cross-posed with FeyP - changes look better than mine so marking fixed.

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

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