Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Category: task » bug
FileSize
17.92 KB

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
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
alexpott’s picture

Priority: Normal » Major
FileSize
18.33 KB

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

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.

star-szr’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

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.

star-szr’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

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

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

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

dlu’s picture

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

#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

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

Reroll of #30.

dlu’s picture

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

pplantinga’s picture

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

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.