Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sidharthap’s picture

Status: Active » Needs review
FileSize
2.8 KB
dawehner’s picture

+++ b/core/modules/system/lib/Drupal/system/TimezoneController.phpundefined
@@ -0,0 +1,27 @@
+ * Contains \Drupal\system\TimezoneController.

Let's move it to the Controller directory.

+++ b/core/modules/system/lib/Drupal/system/TimezoneController.phpundefined
@@ -0,0 +1,27 @@
+/**
+ * Controller for system timezone.
+ */
+class TimezoneController {
+  /**
+   * Menu callback; Retrieve a JSON object containing a suggested time zone name.

Needs better docs.

mparker17’s picture

Assigned: Unassigned » mparker17

Going to help out as part of the code sprint at DrupalCon Portland 2013.

mparker17’s picture

Assigned: mparker17 » Unassigned
FileSize
2.92 KB

Try this new patch.

mparker17’s picture

FileSize
1.36 KB

Here is an interdiff. Sorry this wasn't uploaded in the last comment :P

Status: Needs review » Needs work

The last submitted patch, drupal-system-timezone-1987856-4.patch, failed testing.

mparker17’s picture

Hmm... that's interesting... it failed because "PHP's Intl extension needs to be installed and enabled.". Is that a testing environment problem or did I make a mistake somewhere?

mparker17’s picture

Status: Needs work » Needs review
Issue tags: -WSCCI-conversion

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, drupal-system-timezone-1987856-4.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.93 KB

Re-rolling...

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, 1987856-timezone-controller-10.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion
dawehner’s picture

This seems perfect beside this small points.

+++ b/core/modules/system/lib/Drupal/system/Controller/TimezoneController.phpundefined
@@ -0,0 +1,29 @@
+   *
+   */
+  public function getTimezone($abbreviation = '', $offset = -1, $is_daylight_saving_time = NULL) {

Needs docs.

+++ b/core/modules/system/system.routing.ymlundefined
@@ -143,3 +143,10 @@ system_admin_index:
\ No newline at end of file

Let's add a new line at the end.

vijaycs85’s picture

Thanks @dawehner. Needed a re-roll. Updating comment (credit: http://php.net/manual/en/function.timezone-name-from-abbr.php) as well.

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, 1987856-timezone-controller-14.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1987856-timezone-controller-14.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion
dawehner’s picture

Status: Needs review » Needs work

This is hopefully the last one :(

+++ b/core/modules/system/lib/Drupal/system/Controller/TimezoneController.phpundefined
@@ -0,0 +1,42 @@
+   * @param $offset

offset is probably an int?

+++ b/core/modules/system/lib/Drupal/system/Controller/TimezoneController.phpundefined
@@ -0,0 +1,42 @@
+   *   Offset from GMT in seconds. Defaults to -1 which means that first found
+   *   time zone corresponding to abbr is returned.
+   *   Otherwise exact offset is searched and only if not found then the first
+   *   time zone with any offset is returned.
...
+   *   Daylight saving time indicator. If abbr does not
+   *   exist then the time zone is searched solely by
+   *   offset and isdst.

This is a bad documentation wrapping.

+++ b/core/modules/system/lib/Drupal/system/Controller/TimezoneController.phpundefined
@@ -0,0 +1,42 @@
+   * @return JsonResponse

Let's use the full namespace and maybe a short description

sidharthap’s picture

Assigned: Unassigned » sidharthap
sidharthap’s picture

Status: Needs work » Needs review
FileSize
3.02 KB
vijaycs85’s picture

Updating review comments from #19.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This is perfect now.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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

ianthomas_uk’s picture

Status: Closed (fixed) » Needs review
FileSize
641 bytes

I noticed an ajax request at the end of the install process returning a 404 which I this is a regression caused by this conversion. The URL requested was
/system/timezone/BST/3600/1?date=Tue+Sep+17+2013+21%3A35%3A31+GMT%2B0100+(BST)#

I'm still new to the routing system, but it looks like #22 supports /system/timezone, but not this longer format, presumably because you didn't need to explictly specify these parameters in D7 syntax.

Here's a patch that adds the parameters. I did try adding the defaults from the function signature to the YAML file, but that seemed to break it, so I've used null instead which seems to work. Hopefully someone can confirm that this is the correct thing to do!

googletorp’s picture

dawehner’s picture

Issue tags: +Needs tests

Oooh, I guess we need test coverage for that.

pfrenssen’s picture

Status: Needs review » Needs work

Changing status since this needs tests.

mparker17’s picture

Assigned: sidharthap » Unassigned
Status: Needs work » Needs review
FileSize
1.06 KB
1.69 KB

Hmm... try this.

Note this is my second attempt at writing a SimpleTest, so I don't know if it'll work.

dawehner’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Datetime/DrupalDateTimeTest.php
@@ -37,6 +37,15 @@ public function setUp() {
+    $response = $this->drupalGet('/system/timezone/BST/3600/1?date=Tue+Sep+17+2013+21%3A35%3A31+GMT%2B0100+(BST)#');

I guess a starting "/" won't work. Have you tried that?

Status: Needs review » Needs work

The last submitted patch, drupal8.system-module.1987856-30.patch, failed testing.

mparker17’s picture

Sounds good... let's try it!

mparker17’s picture

Status: Needs work » Needs review

Whoops...

Status: Needs review » Needs work

The last submitted patch, drupal8.system-module.1987856-33.patch, failed testing.

mparker17’s picture

Intriguing.

Testbot is getting a "A non well formed numeric value encountered" error on this line:
$timezone = timezone_name_from_abbr($abbreviation, intval($offset), $is_daylight_saving_time);

But the query string is $response = $this->drupalGet('system/timezone/BST/3600/1?date=Tue+Sep+17+2013+21%3A35%3A31+GMT%2B0100+(BST)#'); so $offset should be 3600.

Further, querying that exact same URL works fine on my environment, simplytest.me's environment, and presumably @ianmthomasuk's environment.

Does anyone know what version of PHP Testbot is running? The project page says 5.3.10 but the project page hasn't been updated since November 2011 which makes me suspect it's out of date.

ianthomas_uk’s picture

That error's being generated by timezone_name_from_abbr, because one of it's parameters is incorrect, right? The first parameter isn't an integer, the second is being passed through intval(), so the error must be with the third parameter.

Maybe try running the code locally passing various different values for $is_daylight_saving_time and see what it does/doesn't accept. Is that the correct syntax for passing a query string to drupalGet()? Is a query string even needed (perhaps it should be a separate test)?

The intval around $offset seems designed to surpress exactly this error message, but surely that means we're getting garbage in and therefore returning an invalid timezone. We should be checking the value of $offset and refusing to return any timezone if it isn't numeric.

disasm’s picture

The issue is the is_daylight_saving_time. Instead of 1, it's being set to 1?date=Tue+Sep+17+2013+21:35:31+GMT+0100+(BST)#. This is odd to me, because it symfony proper, I don't ever remember having this problem. Normally, the var would be set to 1 and you'd get the date from the request object if you wanted to use it.

disasm’s picture

Status: Needs work » Needs review
FileSize
609 bytes
1.68 KB

This won't resolve the issue, but here's a quick patch fixing the default values so they won't all be set to null if a user just hits system/timezone.

disasm’s picture

fixes test. Need to pass query string in an options array, otherwise urlGenerator escapes the ?date= part.

ianthomas_uk’s picture

Status: Needs review » Reviewed & tested by the community

The test looks good, and the patch still works with defaults in the YAML, so RTBC.

ianthomas_uk’s picture

Issue tags: -Needs tests

Remove tag

dawehner’s picture

+1

mparker17’s picture

Awesome, thanks @disasm! I didn't know you had to pass query parameters like that. :P

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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