Replace drupal_get_user_timezone with a date_default_timezone_get() and an event listener

Great work!

1. +++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
   @@ -71,7 +71,7 @@ public function __construct($time = 'now', $timezone = NULL, $settings = []) {
   +      $timezone = \Drupal::service('system.user_timezone_handler')->getUserTimeZone();
   

   sob, singleton access in a value object constructor. this is why we can't have nice things

   We almost need a factory service for date time objects (follow up) or a follow up to deprecate this codepath so that in d9 we can make the timezone required.

   In core the only place calling it without a timezone is in a test.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
   @@ -81,7 +81,7 @@ public function onKernelRequestAuthenticate(GetResponseEvent $event) {
   +      date_default_timezone_set(\Drupal::service('system.user_timezone_handler')->getUserTimeZone());
   
   +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
   @@ -49,7 +49,7 @@ public function setAccount(AccountInterface $account) {
   +    date_default_timezone_set(\Drupal::service('system.user_timezone_handler')->getUserTimeZone());
   
   +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
   @@ -171,7 +171,7 @@ protected function setTimeZone(DrupalDateTime $date) {
   +      $timezone = \Drupal::service('system.user_timezone_handler')->getUserTimeZone();
   
   +++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeWidgetBase.php
   @@ -22,7 +22,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      '#date_timezone' => \Drupal::service('system.user_timezone_handler')->getUserTimeZone()(),
   
   +++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
   @@ -151,7 +151,7 @@ protected function opSimple($field) {
   +      : \Drupal::service('system.user_timezone_handler')->getUserTimeZone();
   
   @@ -173,7 +173,7 @@ protected function getOffset($time, $timezone) {
   +      $origin_offset = $origin_offset + timezone_offset_get(new \DateTimeZone(\Drupal::service('system.user_timezone_handler')->getUserTimeZone()), new \DateTime($time, new \DateTimeZone($timezone)));
   
   +++ b/core/modules/datetime_range/src/Plugin/Field/FieldWidget/DateRangeWidgetBase.php
   @@ -62,7 +62,7 @@ public function massageFormValues(array $values, array $form, FormStateInterface
   +    $user_timezone = new \DateTimeZone(\Drupal::service('system.user_timezone_handler')->getUserTimeZone());
   

   we can inject these

3. +++ b/core/modules/system/tests/src/Kernel/UserTimezoneHandlerTest.php
   @@ -0,0 +1,40 @@
   +    // Uninstalling modules requires the users_data table to exist.
   

   c/p?

4. +++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
   @@ -224,10 +224,11 @@ public function getDateField($field, $string_date = FALSE, $calculate_offset = T
   +    return \Drupal::service('system.user_timezone_handler')->getUserTimeZone();
   

   and this one can be injected too

So its a bit of rats' nest you're unravelling here, and basically you've discovered a hidden dependency from core to the system module.

I'm thinking we'll need a default timezone handler in core namespace, that doesn't support configuration options and then a service provider in system module that swaps out the default implementation for the configurable one - thoughts?

Great work!

I'm +1 for the resolver approach, have asked @alexpott and @catch their thoughts too.

A few thoughts...

1. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
   @@ -49,7 +49,6 @@ public function setAccount(AccountInterface $account) {
   -    date_default_timezone_set(drupal_get_user_timezone());
   

   Removing this feels tricky. Is this responsible for setting the system timezone correctly?

2. +++ b/core/modules/system/tests/src/Kernel/UserTimeZoneDeprecationTest.php
   @@ -0,0 +1,31 @@
   +    // Avoid risky warnings.
   +    $this->assertNotNull(drupal_get_user_timezone());
   

   I think we set the timezone in tests so this can probably assert against an expected value rather than NotNull().

3. +++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
   @@ -47,6 +50,42 @@ abstract class QueryPluginBase extends PluginBase implements CacheableDependency
   +    $this->userTimeZoneHandler = $userTimeZoneHandler;
   

   Whilst views is probably dependent on user - should this be called timeZoneHandler?

4. +1 for the resolver approach too - much better than the service modification going on in the current patch.
5. I'm not 100% sure about the need for core and system resolvers. I would have thought a core and a user resolver are all that is necessary. The system / core config dependency thing is not really a problem. Nothing breaks - config returns nulls and code is often written to expect that. This is by far not the only case of this. I think the core / system divide is a fiction of our own making.

I don't think the core/system divide is a fiction of our own making, we're still in the process of removing direct, hard, circular dependencies between things like the extension system, installer, and system module itself.

https://api.drupal.org/api/drupal/core%21includes%21install.inc/function... (still a hard dependency)

https://api.drupal.org/api/drupal/core%21modules%21system%21system.modul... (only just deprecated in 8.6.x).

Having said that, configuration objects which might or might not exist and are handled gracefully if they aren't are a bit different to having to install a module in order to have a functional module installation system, so don't have a massively strong opinion on 2 vs. 3 services.

I also wonder though, why not move the configuration to core like config.extension.yml? i.e. core.timezone.yml or similar?

One thing which confuses me is that we still have the actual configuration in system module. Given the timezone seems essentially for booting up Drupal, couldn't we place the configuration file inside core itself, and system module just provides a UI for it? This way we would have to have one less service involved.

System module for me its just a excuse for things which could be part of core anyway.

The approach looks nice and reminds me how we did configurable language manager

Lets make sure we are profiling this, because we are adding quite a bit of stuff/services of something that happens pretty early in bootstrap on all (authenticated?) requests.

Yes, profiling this low-level stuff is tricky, the first link had huge differences (20%) but as you said, there was xdebug and also quite a few other changes that seem to be irrelevant, like different amount of queries. However, the second link doesn't have any difference at all, the amount of calls is identical, so that doesn't seem right either.

As discussed, that result still didn't show any call differences.

I did now try it on a extremely minimal route, I picked system.timezone for that, specifically this request: http://d8/system/timezone/nzdt/-1/1.

The blackfire extension somehow misbehaved on that page, probably because of the way my firefox shows the json data there. So I did the request on the console, as anon as well as with a cookie for uid 1. I had the page cache disabled on purpose.

In all cases, the patch was between 6-8% faster but we're talking about response times of 21-23ms, so the difference is 1-2ms.

https://blackfire.io/profiles/compare/65cf67d3-7012-4a10-876a-ea343fc6e6...

Things that we can see
* 3 more classes being loaded
* However, we also save a bunch of things, one event subscriber goes away and with it the call to drupal_get_user_timezone and its \Drupal::config() call.

+++ b/core/modules/system/src/ConfigurableTimeZoneProvider.php
@@ -0,0 +1,51 @@
+   */
+  public function getTimeZone() {
+    if ($this->currentUser && $this->dateConfig->get('timezone.user.configurable') && $this->currentUser->isAuthenticated()) {
+      return $this->currentUser->getTimeZone();
+    }
+    return $this->dateConfig->get('timezone.default');

this->currentUser is a required constructor argument, so I don't think it can ever be NULL?

However, I would suggest we move the is authenticated check first and also only inject the config factory and not already load the config.

The advantage is that we need the config load call only if the user is actually authenticated

The thing that I'm not fully convinced about is whether we really need this to be a tagged service. That is a pattern that is useful if we have to expect multiple service providers that have different weights and on different requests, different services that act on it. In this case, it seems very unlikely that contrib will add additional implementations, it's really just a pattern to decouple core and system and in this case, the pattern that the LanguageManager/ConfigurableLanguageManager are using seems like a better match. That would mean we'd just have a single service in core and we replace/override that in the system module, either by just redefining it in system.services.yml (still not sure how reliable that is) or using ServiceProvider.

Personally, I don't really think that we would even need that level of decoupling, I agree that there is a distinction between system.module and core, but 90% of that is for historical reasons, e.g. all the configuration in system.module is mostly just there because the ability to have config in the core namespace (and especially config entities) only came pretty late in the D8 development cycle if I remember correctly. And the long-term goal is IMHO to move things *out* of it and not adding more to it, but that's just me :) And I'm OK with this approach if it means everyone else is OK with it too.

> The advantage is that we need the config load call only if the user is actually authenticated

Correction: That is wrong as the else case also reads from configuration, so we do always need it.

There *would* be a way to avoid the config read at runtime for the non-configurable case (which I suppose is 90% of the sites?), if we would handle this in a similar way as LanguageServiceProvider does with some of the language configuration. Read it in the service provider and then set it there on the container/service, could be two constructor arguments for example, so at runtime, we'd only need to do something like this:

if ($this->isConfigurable && $this->currentUser->isAuthenticated()) {
  return $this->currentUser->getTimezone();
}
return $this->defaultTimezone;

I'm not quite sure about that, the downside is that we need a config listener that forces a container rebuild if either of those two settings change. Similar to the check in \Drupal\language\Entity\ConfigurableLanguage::postSave(). On the other side, doing that could actually even result in a performance *improvement* compared to HEAD (small, but every config we *don't* have to read during bootstrap is a good thing) which would be an additional argument for getting this committed compared to just being able deprecate an old include file function. And I guess that 99% of the sites out there never change that setting anymore after the initial configuration.

> I think the logic in \Drupal\Core\Datetime\TimeZoneResolver::resolveTimeZone() is the important design change. We're iterating through providers, calling setting date_default_timezone_set($timeZone). All other code is then using date_default_timezone_get() instead of drupal_get_user_timezone().

Yes, I get this part, my proposal would by to simply call this->getTimezone() or so and then system.module would replace the service with its own class that extends the default implementation and overrides getTimezone(). The advantage is that we don't need (yet another) service tag collection process, weight handling/sorting and calling out to other services.

Nothing would change with the events that class implements.

The tagged service approach has to instantiate 3 services (and load the interface), a simple service that's replaced by system.module would have to load just one (+ the interface).

@Berdir your implementation is what I supposed in #33 btw it will require to rebuild container on setting (config) change (like locale doing)
OTOH this change will affect caching - if no configurable time zones allowed we get less cache variations

+++ b/core/modules/datetime/tests/src/Kernel/Views/DateTimeHandlerTestBase.php
@@ -98,6 +98,7 @@ protected function setSiteTimezone($timezone) {
+    $this->container->get('system.timezone_resolver')->resolveTimeZone();

+++ b/core/modules/system/src/ConfigurableTimeZoneResolver.php
@@ -0,0 +1,66 @@
+    if ($this->dateConfig->get('timezone.user.configurable') && $this->currentUser->isAuthenticated()) {
+      date_default_timezone_set($this->currentUser->getTimeZone());
+    }

+++ b/core/modules/system/system.services.yml
@@ -43,3 +43,8 @@ services:
+    class: Drupal\system\ConfigurableTimeZoneResolver

Whilst there is no way to set it via the UI there is code to suggest that a NULL is possible and it is not a explicitly required base field. The original code check if $user->getTimeZone() returns a value. I think we should continue to do that here and open a follow-up to discuss the required-ness. Also see \Drupal\user\UserInterface::TIMEZONE_EMPTY (apparently dead code) - ah no - bad deprecated code :( and system_user_login()

1. +++ b/core/modules/system/src/ConfigurableTimeZoneResolver.php
   @@ -0,0 +1,66 @@
   +
   +/**
   + * A configurable time zone provider.
   + */
   +class ConfigurableTimeZoneResolver implements EventSubscriberInterface {
   

   Hm, so no we got rid of the interface completely, that did go one step further than I imagined :)

   Not sure if we should keep that or not. Also, now that we no longer have a non-configurable implementation, does it make sense to name this one "Configurable..."?

2. +++ b/core/modules/system/src/ConfigurableTimeZoneResolver.php
   @@ -0,0 +1,66 @@
   +   */
   +  public function resolveTimeZone() {
   +    if ($this->dateConfig->get('timezone.user.configurable') && $this->currentUser->isAuthenticated() && $this->currentUser->getTimezone()) {
   +      date_default_timezone_set($this->currentUser->getTimeZone());
   +    }
   +    elseif ($defaultTimeZone = $this->dateConfig->get('timezone.default')) {
   +      date_default_timezone_set($defaultTimeZone);
   +    }
   +  }
   

   Now you merged it together completely, I would still keep a separate protected method that returns the value, then it would still be an option to sublclass if someone wants to do something crazy.

   My (crazy) idea about keeping the config value as a pre-calculated service argument would still require a ServiceProvider, but we could still consider that in a second phase, it's probably too much for a single issue.

1. +++ b/core/modules/system/src/TimeZoneResolver.php
   @@ -41,14 +45,11 @@ public function __construct(AccountInterface $currentUser, ConfigFactoryInterfac
   +    if ($timeZone = $this->getTimeZone()) {
   +      date_default_timezone_set($timeZone);
        }
   

   I think our current coding standard still requires $time_zone for local variables?

2. +++ b/core/modules/system/src/TimeZoneResolver.php
   @@ -63,4 +64,20 @@ public static function getSubscribedEvents() {
   +  /**
   +   * Get the time zone based on site and user configuration.
   

   AFAIK the first word should be third-person, so Gets (or Returns).

   Also not sure about the Resolve on the resolveTimeZone()

   And while we're on naming, not sure if it should be TimeZone or Timezone, I think the second because that's what PHP and our own existing methods does as well: http://php.net/manual/de/datetime.gettimezone.php.

+++ b/core/modules/datetime/tests/src/Kernel/Views/DateTimeHandlerTestBase.php
@@ -98,6 +98,7 @@ protected function setSiteTimezone($timezone) {
       ->set('timezone.user.configurable', 0)
       ->set('timezone.default', $timezone)
       ->save();
+    $this->container->get('system.timezone_resolver')->setDefaultTimeZone();
   }
 

I think this looks great now, the only thing I'm not sure about is these necessary test changes.

It will only affect the current request, so it might be OK, but we could also implement/extend a config save event subscriber that would call out to setDefaultTimeZone() if one of those two values change. Since we already have \Drupal\system\SystemConfigSubscriber, I expect it would only require a few additional lines for that.

Setting to RTBC to get feedback from the committers about this.

Doing things in config listeners is great - it solves a whole class of bugs in config import and using drush or console to make changes so +1 to @Berdir's idea in #59

It will only affect the current request, so it might be OK, but we could also implement/extend a config save event subscriber that would call out to setDefaultTimeZone() if one of those two values change. Since we already have \Drupal\system\SystemConfigSubscriber, I expect it would only require a few additional lines for that.

I think we should do that, CNW for that change.

Personally, I don't really think that we would even need that level of decoupling, I agree that there is a distinction between system.module and core, but 90% of that is for historical reasons, e.g. all the configuration in system.module is mostly just there because the ability to have config in the core namespace (and especially config entities) only came pretty late in the D8 development cycle if I remember correctly. And the long-term goal is IMHO to move things *out* of it and not adding more to it

I agree with this - system.module should have as little code as possible. However in that case, why not move this configuration to somewhere in core* here too?

Wondering if those test fails are caused because we immediately read the new value *and* we do so on the inject config object. I suspect that still sees the old value? So if we really do this, we need to only read it directly from configFactory, to make sure we always have the latest information.

2. Not sure, but changing config is something we need to be very careful about. I've done things like that in pathauto and paragraphs recently and we got a lot of support requests/bug reports because people are using incorrect workflows and are immediately reverting that config change by re-importing old configuration after running updates. So I would be -1 on doing that just to make things a bit "nicer".

I'm so confused about these tests and how our date formatting works, we're converting so many times between timezone and not-timezone and timestamps and date objects, it's absolutely insane :)

But, as far as I can tell, this identified an actual, real bug.

Interestingly, the test only failed on plain and custom date formatter and not the default. They are almost identical, but not *quite*. They didn't get the change/bugfix in #2739290: UTC+12 is broken on date only fields.

So what's happening is that we are rendering a date as just a date, without time, so \Drupal\datetime\Plugin\Field\FieldFormatter\DateTimeFormatterBase::setTimeZone() skips the timezone and forces UTC. But then \Drupal\datetime\Plugin\Field\FieldFormatter\DateTimePlainFormatter::formatDate() calls format() with NULL as timezone, which makes it then fall back to the default. Which is Pacific/Kwajalein, which then adds 12h and makes it switch over to the next date.

The fun part is why we haven't seen this before. Unlike other places, \Drupal\Core\Datetime\DateFormatter::format() already called date_default_timezone_get() directly, and not through our wrapper that returns the configured timezone. So it did not *actually* see the timezone set by \Drupal\Tests\datetime\Functional\DateTestBase::setSiteTimezone() because before, we only set that again when changing the current user. So we've really always been testing that with the default sydney timezone. Now we automatically update it for the current process. It's also fun how that test loops over all the different timezones, but what we're really doing is just ensuring that we are enforcing UTC over and over again :)

@jhedstrom also pointed out there is an existing issue for this problem and I came up with the same fix. Except there we needed additional changes to the test to make it fail: #2861986: Plus one day on Date only fields with custom display widget for pacific/auckland timezone.

Cross-referencing stuff, see #2946826: Add a method to format a DrupalDateTime object to DateFormatter on ideas to make this whole date/timezone/timestamp converting easier to follow.

But, as far as I can tell, this identified an actual, real bug. […] The fun part is why we haven't seen this before.

This bug is very similar to the one reported at and fixed in #3002164: DateTimeIso8601::getDateTime() does not specify the storage timezone when constructing DrupalDateTime object. And I think that issue may be solving the same bug as #68's interdiff at a lower level: in a @DataType class rather than a formatter.

I don't have time to verify right now whether that is indeed solving the same problem at a lower level like I suspect, but we definitely should investigate it.

We discovered it from an API-First POV. You say that @jhedstrom discovered it through a widget. And you discovered the same thing here. So it seems like we're all discovering the same problems deep in our date handling…

@WimLeers, no, I don't think that will help.

The problem is the mentioned disconnect between our date fields/formatters, which uses DateTime objects, and the date formatting API which has a timestamp + timezone API only. So we have to deal with the timezone explicitly.

What's possible is that it would allow to simplify \Drupal\datetime\Plugin\Field\FieldFormatter\DateTimeFormatterBase::setTimeZone as the explicit call to setting the storage timezone then might not be needed anymore. But I that isn't related to this issue/problem.

I think

The problem is the mentioned disconnect between our date fields/formatters, which uses DateTime objects, and the date formatting API which has a timestamp + timezone API only. So we have to deal with the timezone explicitly.

is the correct assessment here. There is way, way too much shuffling amongst \DateTime, DrupalDateTime, DateTimePlus, ISO strings, and timestamps everywhere in core, but a lot of that is because of what is advertised on public APIs.

1. +++ b/core/modules/datetime/tests/src/Kernel/Views/DateTimeHandlerTestBase.php
   @@ -98,6 +98,7 @@ protected function setSiteTimezone($timezone) {
   +    $this->container->get('system.timezone_resolver')->setDefaultTimeZone();
   

   Should this happen on a config event? Ah it is already done... I guess this is not needed.

2. +++ b/core/modules/system/src/SystemConfigSubscriber.php
   @@ -42,6 +52,9 @@ public function onConfigSave(ConfigCrudEvent $event) {
   +    if ($saved_config->getName() == 'system.date' && ($event->isChanged('timezone.default') || $event->isChanged('timezone.user.configurable'))) {
   +      $this->timeZoneResolver->setDefaultTimeZone();
   +    }
   

   <3 ... however thinking about this some more it might be even neater to subscribe to the config event in TimeZoneResolver since that is an event subscriber too. It keeps everything nicely together.

+++ b/core/modules/system/src/TimeZoneResolver.php
@@ -0,0 +1,100 @@
+/**
+ * Event handler that resolves time zone based on site and user configuration.
+ *
+ * Sets the time zone using date_default_timezone_set().
+ *
+ * @see date_default_timezone_set()
+ */
+class TimeZoneResolver implements EventSubscriberInterface, TimeZoneResolverInterface {
+

Should we remove the interface again? With the config event change, we again have no calls to this except the evens.

Nice, I really like having this all contained in one place.

1. +++ b/core/includes/bootstrap.inc
   @@ -530,20 +530,15 @@ function drupal_get_messages($type = NULL, $clear_queue = TRUE) {
   + * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0.
   + *   Use date_default_timezone_get() instead.
   
   +++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
   @@ -71,7 +71,7 @@ public function __construct($time = 'now', $timezone = NULL, $settings = []) {
   -      $timezone = drupal_get_user_timezone();
   +      $timezone = date_default_timezone_get();
   

   🎉 Having worked on some datetime issues lately, anything that simplifies it is a good step forward!

2. +++ b/core/lib/Drupal/Core/Session/AccountSetEvent.php
   @@ -0,0 +1,39 @@
   + * Wraps a configuration event for event listeners.
   

   "Wraps a configuration event" is a copy/paste leftover.

3. +++ b/core/lib/Drupal/Core/Session/AccountSetEvent.php
   @@ -0,0 +1,39 @@
   +   * Gets the Account.
   ...
   +   *   The Account.
   

   s/A/a/

4. +++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeDefaultWidget.php
   @@ -3,8 +3,8 @@
   -use Drupal\Core\Field\FieldItemListInterface;
    use Drupal\Core\Field\FieldDefinitionInterface;
   +use Drupal\Core\Field\FieldItemListInterface;
   

   Out of scope change.

5. +++ b/core/modules/system/src/TimeZoneResolver.php
   @@ -0,0 +1,100 @@
   +   * @var \Drupal\user\Plugin\views\argument_default\CurrentUser
   

   Wrong FQCN.

6. +++ b/core/modules/system/src/TimeZoneResolver.php
   @@ -0,0 +1,100 @@
   +      return $defaultTimeZone;
   

   We only use camelCase for class members.

7. +++ b/core/tests/Drupal/Tests/Core/Session/AccountProxyTest.php
   @@ -36,20 +38,11 @@ public function testId() {
   -if (!function_exists('drupal_get_user_timezone')) {
   -
   -  function drupal_get_user_timezone() {
   -    return date_default_timezone_get();
   -  }
   -
   -}
   

   Nice!

+++ b/core/modules/system/tests/src/Functional/Datetime/DrupalDateTimeTest.php
@@ -54,6 +54,7 @@ public function testDateTimezone() {
+    $this->container->get('system.timezone_resolver')->setDefaultTimeZone();

@@ -71,6 +72,7 @@ public function testDateTimezone() {
+    $this->container->get('system.timezone_resolver')->setDefaultTimeZone();

@@ -80,6 +82,7 @@ public function testDateTimezone() {
+    $this->container->get('system.timezone_resolver')->setDefaultTimeZone();

Not needed - the listener does this.

Thanks to #80, we can go one step further: we can mark system.timezone_resolver private 😀

However … when I rolled a patch to do exactly that, I noticed that nothing in core is doing this…

No, our event subscriber implementation does not alllow private services.

Also, our coding standard documentation allows apparently camel case as long as it is consistently done in a file or so

No, our event subscriber implementation does not alllow private services.

But … making it private still allows tests to pass 🤔

Also, our coding standard documentation allows apparently camel case as long as it is consistently done in a file or so

TIL

@Wim Leers to make a service in our container private doing private: TRUE doesn't work. The only way to make a service private is to do public: false and if you do that tests fail.

1. +++ b/core/core.services.yml
   @@ -1530,6 +1530,7 @@ services:
      current_user:
        class: Drupal\Core\Session\AccountProxy
   +    arguments: ['@event_dispatcher']
   
   +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
   @@ -38,6 +43,23 @@ class AccountProxy implements AccountProxyInterface {
   +  /**
   +   * AccountProxy constructor.
   +   *
   +   * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $eventDispatcher
   +   *   Event dispatcher.
   +   */
   +  public function __construct(EventDispatcherInterface $eventDispatcher) {
   +    $this->eventDispatcher = $eventDispatcher;
   +  }
   

   So I pondered what this might break and I found http://grep.xnddx.ru/search?text=extends+AccountProxy&filename= which leads us to https://cgit.drupalcode.org/acquia_commercemanager/tree/modules/acm/src/... / https://www.drupal.org/project/acquia_commercemanager

   I'm not sure anything will really break because they override the constructor and use the new class in other services and don't override the real current user as far as I can tell. But I've not looked that hard. I've looked for test coverage of the user features it offers which appear to be the ability to log in from an external ecommerce system but I can't find any :(

   Not sure what to do here. \Drupal\Core\Session\AccountProxy does not itself feel like an extension point. Sure inject the service places and consume the current user but there are many more supported ways of adding new ways to log in to Drupal. Plus I have no idea of the security implications of doing things the way this module does.

2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
   @@ -55,7 +55,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   -    $timezone = $this->getSetting('timezone_override');
   +    $timezone = $this->getSetting('timezone_override') ?: $date->getTimezone()->getName();
   
   +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
   @@ -42,7 +42,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   -    $timezone = $this->getSetting('timezone_override');
   +    $timezone = $this->getSetting('timezone_override') ?: $date->getTimezone()->getName();
   

   Why is this change necessary? Ah this is explained in #68 - nice. Fixing bugs ftw.

1. Yeah, OK, that point goes to you on the final-discussion, overriding that service/class for alternative authentication mechanisms seems like a bad idea when we have a pluggable authentication system.

Since it didn't have a constructor before this, they couldn't call it, but I guess they could have still used a setter-based injection.

I guess the only thing we can do to avoid breaking that is to get the event dispatcher through a getEventDispatcher() method that does a @trigger_error() when it's not already injected?

@Berdir well they've also overridden setAccount() so that's not going to fire. And yeah stuff like this is why I think we need to use final more. We need to wrangle our extension points. It'll help everyone.

Yeah, but the question is, what exactly would have happened if AccountProxy would have been final? The *only* thing that does is prevent subclasses. What would have likely happened is that the module author copied the AccountProxy class as so much was changed compared to the original anyway. And the result would have been the same, we would not have the new extension point/event that we are introducing here and the timezone handling would be silently broken for sites using that module.

Which is exactly my argument against using final too much, that it leads to copying whole classes and resulting in maybe less frequent but much more subtle and harder-to-identify bugs than a missing constructor argument, which will either result in a deprecated message or even a hard error. I get how that is a deal-breaker for automated updates, but for anyone with a managed process and testing environments, the second is IMHO preferable.

But we're getting off track with that discussion here. So maybe we should on purpose leave it like it is, reach out to the module authors (which both seem to be fairly active, and the module only has 2 reported/active installs) and hope that not too many custom sites did something similar.

Re #68, "Interestingly, the test only failed on plain and custom date formatter and not the default.", I dug into this and it looks like the root cause goes all the way back to #2399211: Support all options from views fields in DateTime formatters.

This issue solves bug #2861986: Plus one day on Date only fields with custom display widget for pacific/auckland timezone

+++ b/core/lib/Drupal/Core/Session/AccountEvents.php
@@ -0,0 +1,28 @@
+final class AccountEvents {
...
+  const SET_USER = 'account.set';

+++ b/core/lib/Drupal/Core/Session/AccountSetEvent.php
@@ -0,0 +1,39 @@
+class AccountSetEvent extends Event {

Maybe instead of AccountEvents class just add const $eventName to To AccoutSetEvent class? Related #2825358: Event class constants for event names can cause fatal errors when a module is installed

Not sure about constant naming

That issue hasn't been updated in more than a year and I don't think there are any existing events that follow what you just did? the discussion there isn't to *move* the constant, it's to remove it completely.

I'd rather not hold up an issue that was RTBC for so long about something like that.

@Berdir I see no rush here to get it in, but downsides of extra API-addition & useless autoloader call

Meanwhile no reason to block this change on #2825358: Event class constants for event names can cause fatal errors when a module is installed

Nice

Back to rtbc #80 abd commented other issue - let's keep current way

Spurious from the not-correct patch. Reuploading #80 so it is the one tested.

1. +++ b/core/includes/bootstrap.inc
   @@ -530,20 +530,15 @@ function drupal_get_messages($type = NULL, $clear_queue = TRUE) {
    function drupal_get_user_timezone() {
   +  @trigger_error('drupal_get_user_timezone() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use date_default_timezone_get() instead. https://www.drupal.org/node/3009387.', E_USER_DEPRECATED);
   +  return date_default_timezone_get();
   

   So this previously fetched the user's configured timezone and fell back to the site configured timezone followed by the results of the PHP function if there was none configured. Is this correct? If so, is there a behavior change when the user's configured timezone or the site configured timezone does not match that result?

   If there is a behavior change, we need to at a minimum document what it is in the CR. Right now it just tells you to replace the Drupal function use with the PHP base function, without explaining what impacts this might have. It might also need to be mentioned in the release notes for that reason and so we should add a release notes snippet.

   There's also a change where @ was previously being used with the function when it was the fallback, and it is not anymore. I'm guessing this is related to:

   // Ignore PHP strict notice if time zone has not yet been set in the php.ini
   // configuration.
   

   So that's another thing that changes besides the behavior (notices no longer suppressed), and probably also worth mentioning.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
   @@ -80,8 +80,6 @@ public function onKernelRequestAuthenticate(GetResponseEvent $event) {
          }
   -      // No account has been set explicitly, initialize the timezone here.
   -      date_default_timezone_set(drupal_get_user_timezone());
        }
      }
   

   What behavior changes might result from the removal of this fallback? Is it now returning NULL in cases where it wouldn't previously? Also might need to be documented in the CR?

3. +++ b/core/lib/Drupal/Core/Session/AccountEvents.php
   @@ -0,0 +1,28 @@
   +final class AccountEvents {
   

   Not going to make a fuss, but I still don't think we should start peppering the codebase with final when there's disagreement about how we should use it.

4. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
   @@ -38,6 +43,23 @@ class AccountProxy implements AccountProxyInterface {
   +  public function __construct(EventDispatcherInterface $eventDispatcher) {
   +    $this->eventDispatcher = $eventDispatcher;
   +  }
   

   Is there a reason we didn't try to provide some BC for this as described in #87? And also this should perhaps be mentioned in the CR if we've spotted actual contrib modules that might have been affected (even if upon further inspection they weren't).

5. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
   @@ -55,7 +55,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   -    $timezone = $this->getSetting('timezone_override');
   +    $timezone = $this->getSetting('timezone_override') ?: $date->getTimezone()->getName();
   
   +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
   @@ -42,7 +42,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   -    $timezone = $this->getSetting('timezone_override');
   +    $timezone = $this->getSetting('timezone_override') ?: $date->getTimezone()->getName();
   

   Meanwhile this is changing behavior in he other direction. What impacts might that have and why is this one necessary?

All the event constant classes are already final in core. This would be an outlier. They don't contain functionality and extending or overriding them makes no sense.

On point 5, see #86 where alexpott asked the same and then he found #68 where I explained those changes in depth. In short, this makes timezone switching more reliable than before and that uncovered existing bugs where our test coverage didn't work because the timezone wasn't actually switched.

There was an existing bug about it that was closed as a duplicate because that issue was struggling to write a test for it and this has to fix it: #2861986: Plus one day on Date only fields with custom display widget for pacific/auckland timezone

Thanks, I've also added a note about the date/timezone bugfix that's included here to the issue sommary and also extended the API changes section with the AccountProxy stuff.

Back to RTBC, the feedback has been addressed.

I did ask in Slack if committing this to 8.7 is still an option, which would be nice as it does also fix bugs and isn't just a deprecation. But that is pretty unlikely as it contains the AccountProxy changes that will break at least one contrib module.

Reroll with version fix

EDIT: not related failure of tests

It would be nice to be able to pass in a user to get that user's timezone or default instead of the current user.

If you want the timezone of a user then you can simply call $user->getTimezone(). The service that is added here is *not* an API, you are not supposed to call it, ever.

Random fail?

1) Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest::testWidgetOEmbed
Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "Select Another video" not

I opened https://github.com/acquia/commerce-manager/pull/148/ for the commerce manager issue, their sub-class implementation is identical and therefore not needed

https://github.com/acquia/commerce-manager/pull/148 is in, so this has no contrib impact now

Rerolled #115

Down to uber-nits now

1. +++ b/core/lib/Drupal/Core/Session/AccountEvents.php
   @@ -0,0 +1,28 @@
   +class AccountEvents {
   

   as per #107 and other event classes in core, I think we should keep this as final as per earlier patches

2. +++ b/core/modules/system/src/TimeZoneResolver.php
   @@ -0,0 +1,100 @@
   +    if ($saved_config->getName() == 'system.date' && ($event->isChanged('timezone.default') || $event->isChanged('timezone.user.configurable'))) {
   

   nit ===

Rtbc++

Committed e47700f and pushed to 8.8.x. Thanks!

Published change record

This seems to have caused some fairly serious regressions in terms of form entry for datetimes. See #3087606: Datetime::getInfo() caches user's timezone causing unpredictable timestamps. (core) and #3085947: Entered date is stored using cached timezone from previous user (scheduler). In manual testing, the Authored on node form entry now only displays in whatever the server's date is, even though the date/time displays correctly when viewing a node.

As noted in #3087606-8: Datetime::getInfo() caches user's timezone causing unpredictable timestamps., I think this issue exacerbated an existing bug, rather than causing it outright.