Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Files: 
CommentFileSizeAuthor
#40 drupal8.system-module.1987856-40.patch1.77 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,777 pass(es).
[ View ]
#40 interdiff.txt1.04 KBdisasm
#39 drupal8.system-module.1987856-39.patch1.68 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#39 interdiff.txt609 bytesdisasm
#33 drupal8.system-module.1987856-33.patch1.69 KBmparker17
FAILED: [[SimpleTest]]: [MySQL] 58,643 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#33 interdiff.txt904 bytesmparker17
#30 drupal8.system-module.1987856-30.patch1.69 KBmparker17
FAILED: [[SimpleTest]]: [MySQL] 58,549 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#30 interdiff.txt1.06 KBmparker17
#26 1987856-timezone-controller-26.patch641 bytesianthomas_uk
PASSED: [[SimpleTest]]: [MySQL] 58,723 pass(es).
[ View ]
#22 1987856-timezone-controller-22.patch3.49 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 57,930 pass(es).
[ View ]
#22 1987856-diff-14-22.txt1.68 KBvijaycs85
#21 1987856-timezone-controller-21.patch3.02 KBsidharthap
PASSED: [[SimpleTest]]: [MySQL] 57,960 pass(es).
[ View ]
#14 1987856-timezone-controller-14.patch3.44 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,077 pass(es).
[ View ]
#14 1987856-diff-10-14.txt1.21 KBvijaycs85
#10 1987856-timezone-controller-10.patch2.93 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,542 pass(es).
[ View ]
#5 interdiff.txt1.36 KBmparker17
#4 drupal-system-timezone-1987856-4.patch2.92 KBmparker17
FAILED: [[SimpleTest]]: [MySQL] 55,665 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#1 system-timezone-1987856-1.patch2.8 KBsidharthap
PASSED: [[SimpleTest]]: [MySQL] 55,627 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new2.8 KB
PASSED: [[SimpleTest]]: [MySQL] 55,627 pass(es).
[ View ]

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

Assigned:Unassigned» mparker17

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

Assigned:mparker17» Unassigned
StatusFileSize
new2.92 KB
FAILED: [[SimpleTest]]: [MySQL] 55,665 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Try this new patch.

StatusFileSize
new1.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.

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?

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.

Status:Needs work» Needs review
StatusFileSize
new2.93 KB
PASSED: [[SimpleTest]]: [MySQL] 58,542 pass(es).
[ View ]

Re-rolling...

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

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

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

#10: 1987856-timezone-controller-10.patch queued for re-testing.

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.

StatusFileSize
new1.21 KB
new3.44 KB
PASSED: [[SimpleTest]]: [MySQL] 58,077 pass(es).
[ View ]

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.

Status:Needs work» Needs review

#14: 1987856-timezone-controller-14.patch queued for re-testing.

Status:Needs review» Needs work

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

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

#14: 1987856-timezone-controller-14.patch queued for re-testing.

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

Assigned:Unassigned» sidharthap

Status:Needs work» Needs review
StatusFileSize
new3.02 KB
PASSED: [[SimpleTest]]: [MySQL] 57,960 pass(es).
[ View ]

StatusFileSize
new1.68 KB
new3.49 KB
PASSED: [[SimpleTest]]: [MySQL] 57,930 pass(es).
[ View ]

Updating review comments from #19.

Status:Needs review» Reviewed & tested by the community

This is perfect now.

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.

Status:Closed (fixed)» Needs review
StatusFileSize
new641 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,723 pass(es).
[ View ]

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!

#26: 1987856-timezone-controller-26.patch queued for re-testing.

Issue tags:+Needs tests

Oooh, I guess we need test coverage for that.

Status:Needs review» Needs work

Changing status since this needs tests.

Assigned:sidharthap» Unassigned
Status:Needs work» Needs review
StatusFileSize
new1.06 KB
new1.69 KB
FAILED: [[SimpleTest]]: [MySQL] 58,549 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Hmm... try this.

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

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

StatusFileSize
new904 bytes
new1.69 KB
FAILED: [[SimpleTest]]: [MySQL] 58,643 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Sounds good... let's try it!

Status:Needs work» Needs review

Whoops...

Status:Needs review» Needs work

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

Intriguing.

Testbot is getting a "A non well formed numeric value encountered" error on this line:

<?php
$timezone
= timezone_name_from_abbr($abbreviation, intval($offset), $is_daylight_saving_time);
?>

But the query string is

<?php
  $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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new609 bytes
new1.68 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

StatusFileSize
new1.04 KB
new1.77 KB
PASSED: [[SimpleTest]]: [MySQL] 58,777 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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

Issue tags:-Needs tests

Remove tag

+1

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

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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