Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
- system.date contains plenty of date related configuration settings already.
- Whenever system.timezone is needed, system.date is most likely needed, too. And vice-versa.
Proposed solution
- Merge system.timezone into system.date, under a new top-level 'timezone' key.
Comment | File | Size | Author |
---|---|---|---|
#40 | 1934712.system.timezone.40.patch | 18.68 KB | pplantinga |
#38 | 1934712.system.timezone.38.patch | 18.6 KB | pplantinga |
#36 | 1934712.system.timezone.36.patch | 0 bytes | pplantinga |
#33 | system.timezone_merge.1934712.33.patch | 18.6 KB | Gaelan |
#30 | 1934712.system.timezone.30.patch | 18.6 KB | dlu |
Comments
Comment #1
alexpottA patch to do this... and this also fixes a few bugs as there was already some confusion between system.timezone:default and a rogue system.date:timezone.default key!!!
see
system_update_8038()
... date_default_timezone get converted twice! And system_user_timezone() sets system.date:timezone.default... but everywhere else system.timezone:default !!!Comment #2
alexpottComment #4
alexpottThis issue needs to be postponed on #1934964: Locale override subscriber should re-init context to clear caches as this exposes the bug that that issue is trying to fix.
The patch attached proves this statement and fixes another issue with the patch in #1.
Comment #5
alexpottLet's get testbot's opinion on the patch :)
Comment #7
alexpottRerolled patch still includes #1934964: Locale override subscriber should re-init context to clear caches
Comment #8
alexpottThis is actually a major bug as
system_user_timezone()
is usingconfig('system.date')->get('timezone.default')
which is not configurable in UI at the moment.Comment #9
benjifisher#8: 1934712.system.timezone.merge_.8.patch queued for re-testing.
Comment #11
rbayliss CreditAttribution: rbayliss commentedRerolled patch.
Comment #12
rbayliss CreditAttribution: rbayliss commentedComment #13
rbayliss CreditAttribution: rbayliss commentedI couldn't get a useful interdiff, but the only difference from the previous patch is that the system_regional_settings form changes were moved into the RegionalForm.
Comment #15
rbayliss CreditAttribution: rbayliss commented#11: 1934712.system.timezone.merge_.11.patch queued for re-testing.
Comment #17
Gábor Hojtsy#2003892: Convert date formats to config entities actually removes most of system.date.yml but it does not remove the whole thing altogether. Sightly related.
Comment #18
star-szrThis will need a reroll, patch no longer applies.
Comment #19
s_leu CreditAttribution: s_leu commentedRerolled the patch of post #11. The openid module doesn't exist anymore, so the related changes aren't of course not included anymore.
Comment #21
star-szrThanks @s_leu! Needs another reroll now…
Comment #22
dlu CreditAttribution: dlu commentedRerolling.
Comment #23
alexpottWe need to change the system.schema.yml as well...
We need a new line at the end of this file.
Comment #24
dlu CreditAttribution: dlu commentedRerolled the patch from #19 – it fails one of the tests and needs the changes mentioned by Alex in #23 (consensus of ChX and YesCT was that it was better to reroll the patch without fixing additional problems, and then fix the problems in another patch).
Comment #25
dlu CreditAttribution: dlu commentedAfter rerolling the patch in 19 (to 24) there was one test failing. This patch addresses that. It also adds a missing newline at the end of system.date.yml and updates system.schema.yml.
The attached patch is the reroll plus the fixes in the interdiff.
Comment #27
dlu CreditAttribution: dlu commentedIt appears that YAML and I have worked out our differences – new patch attached. Ignore #25.
Comment #28
dlu CreditAttribution: dlu commentedSmall patch to correct a couple of issues that ChX found.
The interdiff is between this patch and the previous one. The patch is against HEAD as of 13:00 PDT today.
This line in core/modules/system/system.module was causing one of the tests to fail – the default timezone was not being used when new accounts were created. The fix I came up with is the line below.
'#default_value' => isset($account->timezone) ? $account->timezone : config('system.date')->get('timezone.default'),
I dropped the test that $account->id() == $user->id() and always use timezone.default if the account doesn't already have a timezone set. The original version of the tests for setting the default timezone is below.
//'#default_value' => isset($account->timezone) ? $account->timezone : ($account->id() == $user->id() ? config('system.date')->get('timezone.default') : ''),
Comment #30
dlu CreditAttribution: dlu commented#28 failed to apply due to a conflict in core/modules/system/config/schema/system.schema.yml between my changes to move the timezone into system.date and breaking date formats out of system.date. That is resolved and a new patch is attached. May the testbot be kind…
Comment #31
dlu CreditAttribution: dlu commented#30: 1934712.system.timezone.30.patch queued for re-testing.
Comment #33
Gaelan CreditAttribution: Gaelan commentedResult of git auto-merging:
Comment #34
pplantinga CreditAttribution: pplantinga commentedComment #36
pplantinga CreditAttribution: pplantinga commentedReroll of #30.
Comment #37
dlu CreditAttribution: dlu commentedThe patch reroll is empty. But the test bot liked it…
Comment #38
pplantinga CreditAttribution: pplantinga commentedGah. We should have test bot fail on empty patches.
Here's an actual patch.
Comment #40
pplantinga CreditAttribution: pplantinga commentedForgot to pull first... gah
Comment #41
dlu CreditAttribution: dlu commentedSeems to be working, have tested manually and the 'bot likes it. Per disasm, I'm marking it RTBC.
Comment #42
LinL CreditAttribution: LinL commentedNow that config() has been deprecated, this should probably use Drupal::config() instead.
See #2028149: The config() function should be deprecated in favour of Drupal::config()
Comment #43
pplantinga CreditAttribution: pplantinga commentedI think there should be a separate issue for that... converting instances of config() in this patch will leave half of the instances in the files converted and half unconverted...
Comment #44
dlu CreditAttribution: dlu commentedThis issue #1957142: Replace config() with Drupal::config() discusses the strategy for converting the config() calls. The consensus (or at least agreement) seemed to be to do it all in one whack, and there is a patch. So I think we can properly and safely leave that change out of this patch. I should have mentioned that this was the reason why I didn't make the change as part of the earlier patches.
So I'm moving it back to RTBC.
Comment #45
catchCommitted/pushed to 8.x, thanks!
Comment #46
damiankloip CreditAttribution: damiankloip commented