Join language and timezone settings in one single fieldset

I like this change.

(It makes you wonder though -- should the per-user timezone settings live in locale.module? I don't think so, but it's probably good to raise the question as people might find that more intuitive ...)

Features such as this will not go into the stable Drupal 5.x.

The patch no longer applies.

Didn't apply anymore. Updated this patch to HEAD, although it's a bit late in D6 for this.

Oops, forgot to set to CNR...

Dries said:

It makes you wonder though -- should the per-user timezone settings live in locale.module? I don't think so, but it's probably good to raise the question as people might find that more intuitive ...)

This should be discussed further as soon as we're in the D7 dev cycle.

Joining the two fieldsets on the user page would anyway make sense. So if someone could test and rtbc this patch...?

Is this is an active need?

Please find an updated patch attached for review.

In Drupal 8.x, language fieldset is provided by the user module and locale is provided by the system module. The user module comes first so I changed it's language fieldset to be called locale with a title of 'Localization'.

System module then uses this fieldset.

I'll look at updating the tests.

On reflection, why does the system module add the timezone selector to the user edit form. Although system module provides the timezone api functions, it should be the user module which adds the options to the user edit form. Since user and system are both core required modules for all Drupal sites I see no issue in moving all the options to the user module.

Next patch will include this.

See attached screenshot of what the combined fieldset looks like.

New patch attached for review. The patch moves the form elements for timezone out of the system module and puts it in the user module. The user module language form elements are placed inside a localization fieldset with the timezone element. This also corrects the openid module tests.

+++ b/core/modules/user/lib/Drupal/user/AccountFormController.php
@@ -178,18 +178,20 @@ public function form(array $form, array &$form_state, EntityInterface $account)
+    $localization_options_available = drupal_container()->get('module_handler')->moduleExists('language') || config('system.timezone')->get('user.configurable');

Can use $this->moduleHandler now. And should use Drupal::config().

#14: language-timezone-one-user-field-134209-7255592.patch queued for re-testing.

Wow, this is old :)

I guess this won't get into 8.1 and definitely not 8.0. Still would like to get rid of all those system/user form alters and hooks. Also pulling in some changes from #2051067: Move user timezone code from system.module form alters to user form controllers.

Also not sure if we need to keep system_user_timezone() and @deprecate it. I guess so, technically although nobody is hopefully calling it.

No interdiff since most of it didn't apply anymore.

Actually, that extra fields change makes no sense, we need to rename the existing one and just drop timezone.

+++ b/core/modules/user/user.module
@@ -141,18 +141,11 @@ function user_entity_extra_field_info() {
-  $fields['user']['user']['form']['language'] = array(
-    'label' => t('Language settings'),
+  $fields['user']['user']['form']['localization'] = array(
+    'label' => t('Localization settings'),
     'description' => t('User module form element.'),

Would this require a hook_update_N call? Can't you move around the extra fields?

Renamed the key back to language, makes this a smaller API change, also fixes the installer config test.

Improved the migrate test to actually set a default timezone which is always there in a non-kernel test. somehow ended up with a NULL vs. '' difference.

Rerolled so I could make a screenshot to compare with the related issue.

I closed #272397: Update "Locale settings" on the user edit page to "Time zone settings" as a duplicate of this. This patch makes more than to me than the other issue. Let's ask for a usability review here.

Regarding usability review: Could we get some details under the User Interface Changes section of the Issue Summary, specifically before and after screenshots, thanks.

Also, IS references Drupal 5, might want to update that as well ;)

@AaronMcHale, thank you for the instructions.

I've updated the IS and added screenshots. I am hoping this is ready for a usability review now.

Meant to set to NR.

+++ b/core/modules/user/src/AccountForm.php
@@ -279,29 +281,24 @@ public function form(array $form, FormStateInterface $form_state) {
     $system_date_config = \Drupal::config('system.date');
-    $form['timezone'] = [
-      '#type' => 'details',
-      '#title' => $this->t('Locale settings'),
-      '#open' => TRUE,
-      '#weight' => 6,
-      '#access' => $system_date_config->get('timezone.user.configurable'),
-    ];
     if ($self_register && $system_date_config->get('timezone.user.default') != UserInterface::TIMEZONE_SELECT) {

This means that we also need to remove the timezone key in user_entity_extra_field_info() and update all user form displays to remove that key from it.

This will break some customizations I suppose, but we did that before, form structures are not part of BC. That said, this being a separate extra field is slightly more complicated, as it might result in the timezone field showing up for sites that had hidden it through the form display but have language visible. As there is still the user configurable flag, I suppose that is only a very small amount of sites, but who knows.

Leaving at needs review as we can work on that after we have a +1 from a usability perspective.

Fix some screenshot links.

I am updating the issue summary, showing the screenshots instead of providing links.

Usability review

We discussed this issue at #3200771: Drupal Usability Meeting 2021-03-05. In short, YES!

It is kind of silly to have three fieldsets or details elements (or even one or two) with just one form element each. While that will still be the case for monolingual sites (two fieldsets, each with one form element) this issue will improve the situation for multilingual sites.

We tried, but we could not think of any drawbacks.

Back to NW because of #40.

@benjifisher and the UX team, thanks for the quick turnaround on this issue!

The next step is to do the work for #40.

This means that we also need to remove the timezone key in user_entity_extra_field_info() and update all user form displays to remove that key from it.

+++ b/core/modules/user/user.module
@@ -121,17 +121,11 @@ function user_entity_extra_field_info() {
-  if (\Drupal::config('system.date')->get('timezone.user.configurable')) {
-    $fields['user']['user']['form']['timezone'] = [
-      'label' => t('Timezone'),
-      'description' => t('System module form element.'),
-      'weight' => 6,
-    ];
-  }

This satisfies the first part, removing timezone fro user_entity_extra_field_info().

The second part is to update all user form displays to remove that key from it. How to do this? My knowledge of forms is pretty limited.

Adding Bug Smash tag because work on this started from the duplicate that was original a Bug Report.

I think we need to remove language key from the entity_form display modes.

@Berdir, I found two occurrences for the same and removed in current patch.
Please correct me if I am missing something.

Patch works as intended, while addressing #40. Since the propose is already revised I'm moving it to RTBC.

Edit: forgot to post image evidence. Adding below.

The second half of the issue title seems to be outdated, haven't reviewed the code properly yet but noticed no system.module changes.

1. +++ b/core/modules/user/src/Entity/User.php
   @@ -108,6 +108,11 @@ public function preSave(EntityStorageInterface $storage) {
        }
   +
   +    $config = \Drupal::config('system.date');
   +    if ($config->get('timezone.user.configurable') && !$this->getTimeZone() && !$config->get('timezone.user.default')) {
   +      $this->timezone = $config->get('timezone.default');
   +    }
      }
   

   Why is this necessary? Presumably not due to the form changes?

2. +++ b/core/modules/user/user.module
   @@ -121,17 +121,10 @@ function user_entity_extra_field_info() {
      ];
   -  if (\Drupal::config('system.date')->get('timezone.user.configurable')) {
   -    $fields['user']['user']['form']['timezone'] = [
   -      'label' => t('Timezone'),
   -      'description' => t('System module form element.'),
   -      'weight' => 6,
   -    ];
   -  }
   

   Will this require any form displays to be updated that could be referencing the extra field?

3. +++ b/core/modules/user/user.module
   @@ -463,6 +456,19 @@ function user_login_finalize(UserInterface $account) {
   +
   +  $config = \Drupal::config('system.date');
   +  // If the user has a NULL time zone, notify them to set a time zone.
   +  if (!$account->getTimezone() && $config->get('timezone.user.configurable') && $config->get('timezone.user.warn')) {
   +    \Drupal::messenger()
   +      ->addStatus(t('Configure your <a href=":user-edit">account time zone setting</a>.', [
   +        ':user-edit' => $account->url('edit-form', [
   +          'query' => \Drupal::destination()->getAsArray(),
   +          'fragment' => 'edit-timezone',
   +        ]),
   +      ]));
   +  }
   

   This looks like it's probably OK but again why is it done in this issue? Doesn't seem necessary for changing the fieldset.

4. +++ b/core/profiles/demo_umami/config/install/core.entity_form_display.user.user.default.yml
   @@ -21,9 +21,6 @@ content:
        region: content
   -  timezone:
   -    weight: 6
   -    region: content
      user_picture:
   

   OK yeah this confirms the question above - so I think we probably need an update to remove timezone from every core.entity_form_display.user.user.*

+++ b/core/modules/user/user.module
@@ -463,6 +456,19 @@ function user_login_finalize(UserInterface $account) {
   \Drupal::service('session')->set('uid', $account->id());
+
+  $config = \Drupal::config('system.date');
+  // If the user has a NULL time zone, notify them to set a time zone.
+  if (!$account->getTimezone() && $config->get('timezone.user.configurable') && $config->get('timezone.user.warn')) {
+    \Drupal::messenger()
+      ->addStatus(t('Configure your <a href=":user-edit">account time zone setting</a>.', [
+        ':user-edit' => $account->url('edit-form', [
+          'query' => \Drupal::destination()->getAsArray(),
+          'fragment' => 'edit-timezone',

this is likely a leftover from a reroll. Another issue moved this code from system.module to user.module and it added it to user_user_login(), so this would duplicate it.

And yeah, not sure about the migrate test changes.

- Fixing suggestions #1 and #3
- For suggestion 2 and 4, I spotted two core.entity_form_display.user.user.default.yml files only.
Both the files are updated in patch #46

+++ b/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserTest.php
@@ -34,6 +34,10 @@ protected function setUp(): void {
     $this->installEntitySchema('node');
     $this->installSchema('user', ['users_data']);
+    $this->installConfig(['system']);
+    $this->config('system.date')
+      ->set('timezone.default', 'Australia/Sydney')
+      ->save();
     // Make sure uid 1 is created.
     user_install();
 
@@ -105,7 +109,7 @@ public function testUser() {

@@ -105,7 +109,7 @@ public function testUser() {
       $this->assertSame($source->login, $user->getLastLoginTime());
       $is_blocked = $source->status == 0;
       $this->assertSame($is_blocked, $user->isBlocked());
-      $expected_timezone_name = $source->timezone_name ?: $this->config('system.date')->get('timezone.default');
+      $expected_timezone_name = $source->timezone_name ?: 'Australia/Sydney';
       $this->assertSame($expected_timezone_name, $user->getTimeZone());

I think this part is also safe to remove. It doesn't make any functional changes, I was able to remove any changes to the test and it still passes, so I think we should remove these changes too.

Removing additional test cases.

@Berdir, do you think we should add functional tests? I think we are just changing form structure, so I thought of not to add.
Let me know if it makes sense.

Hm, good question, but functionally, the wrappers have no effect as they have no #tree TRUE. So we are testing that each element works, just not that it visually appears within a given details element. But we don't have that for any existing element either I think, so I'd say that's not a requirement?

What we still do need is an update function to clean out remaining configuration. A post update that loads each user form display and removes the timezone component. See https://api.drupal.org/api/drupal/core%21modules%21system%21system.post_... for some examples that do stuff with form displays in updates.

I just checked, and the filled database dump has the timezone field enabled, so we just need to assert in a test then that it is no longer there. To see that, add var_dump(\Drupal::config('core.entity_form_display.user.user.default')->get()); to the end of \Drupal\Tests\system\Functional\UpdateSystem\UpdatePathTestBaseFilledTest, and then alter it to assert that the timezone key is no longer in there.

Also created a change record: https://www.drupal.org/node/3209248

@Berdir Implemented following changes:
- Added an update hook to remove timezone from the configuration
- An test case to test the update hook process.
Test cases are passing on local.

Verified and tested patch#58 on the drupal 9.3.x-dev version. Patch applied successfully and looks good to me.Adding screenshot for the reference.
Need +1 RTBC

+++ b/core/modules/user/src/AccountForm.php
@@ -279,29 +281,24 @@ public function form(array $form, FormStateInterface $form_state) {
     if ($self_register && $system_date_config->get('timezone.user.default') != UserInterface::TIMEZONE_SELECT) {
       $form['timezone']['#access'] = FALSE;
     }

$form['timezone'] no longer exists so this code needs updating or removing.

The logic around whether the fieldset is displayed or not and which type it is seems quite convoluted, are we sure that it is displayed correctly for all possible configurations?

+++ b/core/modules/user/src/AccountForm.php
@@ -279,29 +281,24 @@ public function form(array $form, FormStateInterface $form_state) {
-    ];
     if ($self_register && $system_date_config->get('timezone.user.default') != UserInterface::TIMEZONE_SELECT) {
       $form['timezone']['#access'] = FALSE;
     }
-    $form['timezone']['timezone'] = [
+    // Add the time zone field to the user edit and register forms.
+    $form['language']['timezone'] = [
       '#type' => 'select',
       '#title' => $this->t('Time zone'),
       '#default_value' => $account->getTimezone() ?: $system_date_config->get('timezone.default'),
       '#options' => system_time_zones($account->id() != $user->id(), TRUE),
       '#description' => $this->t('Select the desired local time and time zone. Dates and times throughout this site will be displayed using this time zone.'),
...
     ];

We did move the #access down. And the language field already depends on !$self_register.

But yes, we can remove that condition then.

Not 100% sure about about the new check vs. the explicit != TIMEZONE_SELECT check before. There is a third option, empty. I can actually not see any explicit usages of that, so not sure where that is used also confuses me as afaik we set a value then anyway in \user_user_presave. Anyway, I would propose to remove the if and keep the explicit == UserInterface::TIMEZONE_SELECT check then.