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

  1. Merge system.timezone into system.date, under a new top-level 'timezone' key.
Files: 
CommentFileSizeAuthor
#40 1934712.system.timezone.40.patch18.68 KBpplantinga
PASSED: [[SimpleTest]]: [MySQL] 57,584 pass(es). View
#38 1934712.system.timezone.38.patch18.6 KBpplantinga
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1934712.system.timezone.38.patch. Unable to apply patch. See the log in the details link for more information. View
#36 1934712.system.timezone.36.patch0 bytespplantinga
PASSED: [[SimpleTest]]: [MySQL] 57,528 pass(es). View
#33 system.timezone_merge.1934712.33.patch18.6 KBGaelan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system.timezone_merge.1934712.33.patch. Unable to apply patch. See the log in the details link for more information. View
#30 1934712.system.timezone.30.patch18.6 KBdlu
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1934712.system.timezone.30.patch. Unable to apply patch. See the log in the details link for more information. View
#28 1934712.system.timezone.28.patch18.63 KBdlu
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1934712.system.timezone.28.patch. Unable to apply patch. See the log in the details link for more information. View
#28 1934712.27-28.interdiff.txt1.49 KBdlu
#27 1934712.system.timezone.27.patch18.94 KBdlu
PASSED: [[SimpleTest]]: [MySQL] 57,178 pass(es). View
#25 1934712.system.timezone.25.patch18.93 KBdlu
FAILED: [[SimpleTest]]: [MySQL] 56,127 pass(es), 37 fail(s), and 9 exception(s). View
#25 1934712.24-25.interdiff.txt4.25 KBdlu
#24 1934712.system.timezone.24.patch16.5 KBdlu
FAILED: [[SimpleTest]]: [MySQL] 57,177 pass(es), 1 fail(s), and 0 exception(s). View
#19 1934712.system.timezone.19.patch16.43 KBs_leu
FAILED: [[SimpleTest]]: [MySQL] 58,010 pass(es), 1 fail(s), and 0 exception(s). View
#11 1934712.system.timezone.merge_.11.patch18.59 KBrbayliss
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View
#8 1934712.system.timezone.merge_.8.patch18.33 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1934712.system.timezone.merge_.8.patch. Unable to apply patch. See the log in the details link for more information. View
#7 1934712.plus_.1934964.system.timezone.7.patch19.99 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 52,802 pass(es). View
#4 1-4-interdiff.txt2.05 KBalexpott
#4 1934712.plus_.1934964.system.timezone.4.patch19.98 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1934712.plus_.1934964.system.timezone.4.patch. Unable to apply patch. See the log in the details link for more information. View
#1 1934712.system.timezone.1.patch17.92 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 52,459 pass(es), 3 fail(s), and 0 exception(s). View

Comments

alexpott’s picture

Category: task » bug
FileSize
17.92 KB
FAILED: [[SimpleTest]]: [MySQL] 52,459 pass(es), 3 fail(s), and 0 exception(s). View

A 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 !!!

alexpott’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1934712.system.timezone.1.patch, failed testing.

alexpott’s picture

Status: Needs work » Postponed
FileSize
19.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1934712.plus_.1934964.system.timezone.4.patch. Unable to apply patch. See the log in the details link for more information. View
2.05 KB

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

alexpott’s picture

Status: Postponed » Needs review

Let's get testbot's opinion on the patch :)

Status: Needs review » Needs work

The last submitted patch, 1934712.plus_.1934964.system.timezone.4.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
19.99 KB
PASSED: [[SimpleTest]]: [MySQL] 52,802 pass(es). View
alexpott’s picture

Priority: Normal » Major
FileSize
18.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1934712.system.timezone.merge_.8.patch. Unable to apply patch. See the log in the details link for more information. View

This is actually a major bug as system_user_timezone() is using config('system.date')->get('timezone.default') which is not configurable in UI at the moment.

benjifisher’s picture

Status: Needs review » Needs work
Issue tags: +Configuration system, +Sunrise Sanity Cruise

The last submitted patch, 1934712.system.timezone.merge_.8.patch, failed testing.

rbayliss’s picture

FileSize
18.59 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View

Rerolled patch.

rbayliss’s picture

Status: Needs work » Needs review
rbayliss’s picture

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

Status: Needs review » Needs work
Issue tags: -Configuration system, -Sunrise Sanity Cruise

The last submitted patch, 1934712.system.timezone.merge_.11.patch, failed testing.

rbayliss’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Configuration system, +Sunrise Sanity Cruise

The last submitted patch, 1934712.system.timezone.merge_.11.patch, failed testing.

Gábor Hojtsy’s picture

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

Cottser’s picture

Issue tags: +Needs reroll

This will need a reroll, patch no longer applies.

s_leu’s picture

Status: Needs work » Needs review
FileSize
16.43 KB
FAILED: [[SimpleTest]]: [MySQL] 58,010 pass(es), 1 fail(s), and 0 exception(s). View

Rerolled the patch of post #11. The openid module doesn't exist anymore, so the related changes aren't of course not included anymore.

Status: Needs review » Needs work

The last submitted patch, 1934712.system.timezone.19.patch, failed testing.

Cottser’s picture

Thanks @s_leu! Needs another reroll now…

dlu’s picture

Rerolling.

alexpott’s picture

We need to change the system.schema.yml as well...

+++ b/core/modules/system/config/system.date.ymlundefined
@@ -62,3 +62,9 @@ formats:
+timezone:
+  default: ''
+  user:
+    configurable: '1'
+    default: '0'
+    warn: '0'
\ No newline at end of file

We need a new line at the end of this file.

dlu’s picture

Issue tags: -Needs reroll
FileSize
16.5 KB
FAILED: [[SimpleTest]]: [MySQL] 57,177 pass(es), 1 fail(s), and 0 exception(s). View

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

dlu’s picture

Status: Needs work » Needs review
FileSize
4.25 KB
18.93 KB
FAILED: [[SimpleTest]]: [MySQL] 56,127 pass(es), 37 fail(s), and 9 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 1934712.system.timezone.25.patch, failed testing.

dlu’s picture

Status: Needs work » Needs review
FileSize
18.94 KB
PASSED: [[SimpleTest]]: [MySQL] 57,178 pass(es). View

It appears that YAML and I have worked out our differences – new patch attached. Ignore #25.

dlu’s picture

FileSize
1.49 KB
18.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1934712.system.timezone.28.patch. Unable to apply patch. See the log in the details link for more information. View

Small 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') : ''),

Status: Needs review » Needs work

The last submitted patch, 1934712.system.timezone.28.patch, failed testing.

dlu’s picture

Assigned: Unassigned » dlu
Status: Needs work » Needs review
FileSize
18.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1934712.system.timezone.30.patch. Unable to apply patch. See the log in the details link for more information. View

#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…

dlu’s picture

Status: Needs review » Needs work
Issue tags: +Configuration system, +Sunrise Sanity Cruise

The last submitted patch, 1934712.system.timezone.30.patch, failed testing.

Gaelan’s picture

FileSize
18.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system.timezone_merge.1934712.33.patch. Unable to apply patch. See the log in the details link for more information. View

Result of git auto-merging:

pplantinga’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, system.timezone_merge.1934712.33.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
0 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,528 pass(es). View

Reroll of #30.

dlu’s picture

The patch reroll is empty. But the test bot liked it…

pplantinga’s picture

FileSize
18.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1934712.system.timezone.38.patch. Unable to apply patch. See the log in the details link for more information. View

Gah. We should have test bot fail on empty patches.

Here's an actual patch.

Status: Needs review » Needs work

The last submitted patch, 1934712.system.timezone.38.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
18.68 KB
PASSED: [[SimpleTest]]: [MySQL] 57,584 pass(es). View

Forgot to pull first... gah

dlu’s picture

Status: Needs review » Reviewed & tested by the community

Seems to be working, have tested manually and the 'bot likes it. Per disasm, I'm marking it RTBC.

LinL’s picture

Status: Reviewed & tested by the community » Needs work

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

pplantinga’s picture

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

dlu’s picture

Status: Needs work » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

damiankloip’s picture

Assigned: dlu » Unassigned

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