Properly deprecate DRUPAL_USER_TIMEZONE_* constants

This moves all the user timezone related code out of system.module and into user.module where it belongs.

The config is still owned by system.date but instead of simply migrating it to the user module I wonder if we should go further and deprecate timezone.user.configurable and UserInterface::TIMEZONE_SELECT entirely and rely on the form display configuration to allow users to hide or show the timezone field in the relevant contexts. As I discovered in testing even if you allow the timezone to be configured in this custom config, it is still not shown if you remove it from the form display.

I think a lot of this was already done in #2331763: Remove user hooks and form alters from system module but never found anyway that wanted to review this :)

Whoops, didn't see that. The default value approach is different here, I should probably take your method, but the rest of it is so out of date it needed a complete reroll anyway by the looks of it :)

Good that we got the same single test failure, but not sure I understand why...

Borrowed @Berdir's approach from #2331763: Remove user hooks and form alters from system module for the default value, and modified the D6 migration test to use assertEquals, which works around the "" vs null issue.

+++ b/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserTest.php
@@ -106,7 +106,7 @@ public function testUser() {
-      $this->assertSame($expected_timezone_name, $user->getTimeZone());
+      $this->assertEquals($expected_timezone_name, $user->getTimeZone());

I don't think this is the right call here. The interface \Drupal\Core\Session\AccountInterface::getTimeZone has an @return string as docblok, so it should always return a string. Other code that uses strict checking could start misbehaving when it gets a NULL-value instead of ''.

I discussed this with @Wim Leers at the sprint weekend, because I wasn't sure about this, he mentioned #3095922. That issue adds a default value to the comment language field.

I think we could also add a default value of '' to the user's timezone, so that it doesn't use NULL but it uses that instead.

Because this issue otherwise would not be just a deprecation but it would also introduce a behavior change.

We could also update the interface to say @return string|NULL - but I think that is an even bigger change.

I haven't validated if that this actually works though

Setting the default value of the timezone field in the User entity doesn't work, as it is still filtered by FieldItemList::preSave() as noted in #10, and I think this would also be considered a behaviour change?

One alternative that doesn't mean altering the test is overriding TimeZoneItem::isEmpty() to allow an empty string timezone which then doesn't get filtered, this passes the test but I am also not sure if this is considered a behaviour change.

So #14 is also a behaviour change in different ways, as it causes other tests to fail. Not sure what to do now, but it does feel like allowing both NULL and empty string for the timezone shouldn't be allowed, we should pick just one that means "unknown/not set" and stick to it.

+++ b/core/modules/user/user.module
@@ -1455,3 +1468,58 @@ function template_preprocess_user(&$variables) {
+function system_form_system_regional_settings_submit($form, FormStateInterface $form_state) {

it looks weird name for function in user's module

In #3111942: Remove all remaining @deprecated code from system module this issue was raised as one that would take care of DRUPAL_USER_TIMEZONE_* constants. It indeed seems to do as there does not seem to be any further actual use of them.

$ git grep DRUPAL_USER_TIMEZO
core/modules/system/src/Form/RegionalForm.php:        DRUPAL_USER_TIMEZONE_DEFAULT => t('Default time zone'),
core/modules/system/src/Form/RegionalForm.php:        DRUPAL_USER_TIMEZONE_EMPTY   => t('Empty time zone'),
core/modules/system/src/Form/RegionalForm.php:        DRUPAL_USER_TIMEZONE_SELECT  => t('Users may set their own time zone at registration'),
core/modules/system/system.module:const DRUPAL_USER_TIMEZONE_DEFAULT = 0;
core/modules/system/system.module:const DRUPAL_USER_TIMEZONE_EMPTY = 1;
core/modules/system/system.module:const DRUPAL_USER_TIMEZONE_SELECT = 2;
core/modules/system/system.module:  if ($config->get('timezone.user.configurable') && $config->get('timezone.user.default') == DRUPAL_USER_TIMEZONE_SELECT) {
core/modules/system/tests/src/Kernel/Migrate/d7/MigrateSystemConfigurationTest.php:          // DRUPAL_USER_TIMEZONE_SELECT (D7 API)

It is not possible to remove them here, but a 9.0.x-only issue is possible after this to remove them cleanly.

Marking as 9 beta requirement given we need to remove usages before we an remove the constant.

#19/20 look OK to me, we could add a @todo to user_user_presave() as we know we should clean it up in the future?

IMHO \Drupal\Tests\user\Kernel\Migrate\d6\MigrateUserTest::testUser() is just broken, having the timezone behavior set to setting a default but then expecting it to be '' just because it is a kernel test that didn't actually set a default timezone makes no sense :)

Also, content entity fields never have been type-safe and you can't expect that from them $node->id() could be an integer or a string, that's just how it is atm. Same for '' vs NULL IMHO.

But we can discuss that in the follow-up to unblock this.

Glad that we finally moved that piece to user.module, even though I don't think it would have been necessary to resolve this issue. system and user are both required modules, both depend on each other, before and after this change. And it's only half-done now with this, as the setting is still in system.date and its expected values are constants defined in user.module, so nothing really changed :) But maybe it's the first step to further cleanups ;)

+++ b/core/modules/user/user.module
@@ -184,6 +185,19 @@ function user_entity_extra_field_info() {
+ *
+ * @todo https://www.drupal.org/project/drupal/issues/3112704 Move to
+ *   \Drupal\user\Entity\User::preSave().
+ */
+function user_user_presave(UserInterface $account) {

Haven't seen this @todo format before (URL first), but we do seem to have 50 instances of that in core. I'm used to @todo Do xy in URL.

Funny enough, in 2014 I wrote this in the issue summary of #2331763: Remove user hooks and form alters from system module (which I just closed as duplicate)

> Because one required module altering the other makes so much sense.

And what we end up doing here is just inverting it by altering a system.module form in user.module for the settings UI ;)

This doesn't apply to 9.0.x.

+++ b/core/modules/system/system.module
@@ -808,79 +807,6 @@ function system_form_alter(&$form, FormStateInterface $form_state) {
-
-/**
- * Add the time zone field to the user edit and register forms.
- */
-function system_user_timezone(&$form, FormStateInterface $form_state) {
-  $user = \Drupal::currentUser();
-
-  $account = $form_state->getFormObject()->getEntity();
-  $form['timezone'] = [
-    '#type' => 'details',
-    '#title' => t('Locale settings'),
-    '#open' => TRUE,
-    '#weight' => 6,
-  ];
-  $form['timezone']['timezone'] = [
-    '#type' => 'select',
-    '#title' => t('Time zone'),
-    '#default_value' => $account->getTimezone() ? $account->getTimezone() : \Drupal::config('system.date')->get('timezone.default'),
-    '#options' => system_time_zones($account->id() != $user->id(), TRUE),
-    '#description' => t('Select the desired local time and time zone. Dates and times throughout this site will be displayed using this time zone.'),
-  ];
-  $user_input = $form_state->getUserInput();
-  if (!$account->getTimezone() && $account->id() == $user->id() && empty($user_input['timezone'])) {
-    $form['timezone']['#attached']['library'][] = 'core/drupal.timezone';
-    $form['timezone']['timezone']['#attributes'] = ['class' => ['timezone-detect']];
-  }
-}

So I don't think the 8.9.x patch should remove this. We could maybe mark it @internal explicitly?

This looks good and gets us one step closer to D9 beta1.

Superb, thanks all. The Drupal 8.9 patch did not apply but it was an easy fix on commit (the removed use statement's context changed).

Did we mean to publish the draft change record?

Published CR