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.
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.
Comment | File | Size | Author |
---|---|---|---|
#40 | drupal8.system-module.1987856-40.patch | 1.77 KB | disasm |
#40 | interdiff.txt | 1.04 KB | disasm |
#39 | drupal8.system-module.1987856-39.patch | 1.68 KB | disasm |
#39 | interdiff.txt | 609 bytes | disasm |
#33 | drupal8.system-module.1987856-33.patch | 1.69 KB | mparker17 |
Comments
Comment #1
sidharthapComment #2
dawehnerLet's move it to the Controller directory.
Needs better docs.
Comment #3
mparker17Going to help out as part of the code sprint at DrupalCon Portland 2013.
Comment #4
mparker17Try this new patch.
Comment #5
mparker17Here is an interdiff. Sorry this wasn't uploaded in the last comment :P
Comment #7
mparker17Hmm... 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?
Comment #8
mparker17#4: drupal-system-timezone-1987856-4.patch queued for re-testing.
Comment #10
vijaycs85Re-rolling...
Comment #12
vijaycs85#10: 1987856-timezone-controller-10.patch queued for re-testing.
Comment #13
dawehnerThis seems perfect beside this small points.
Needs docs.
Let's add a new line at the end.
Comment #14
vijaycs85Thanks @dawehner. Needed a re-roll. Updating comment (credit: http://php.net/manual/en/function.timezone-name-from-abbr.php) as well.
Comment #16
vijaycs85#14: 1987856-timezone-controller-14.patch queued for re-testing.
Comment #18
vijaycs85#14: 1987856-timezone-controller-14.patch queued for re-testing.
Comment #19
dawehnerThis is hopefully the last one :(
offset is probably an int?
This is a bad documentation wrapping.
Let's use the full namespace and maybe a short description
Comment #20
sidharthapComment #21
sidharthapComment #22
vijaycs85Updating review comments from #19.
Comment #23
dawehnerThis is perfect now.
Comment #24
catchCommitted/pushed to 8.x, thanks!
Comment #26
ianthomas_ukI 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!
Comment #27
googletorp CreditAttribution: googletorp commented#26: 1987856-timezone-controller-26.patch queued for re-testing.
Comment #28
dawehnerOooh, I guess we need test coverage for that.
Comment #29
pfrenssenChanging status since this needs tests.
Comment #30
mparker17Hmm... try this.
Note this is my second attempt at writing a SimpleTest, so I don't know if it'll work.
Comment #31
dawehnerI guess a starting "/" won't work. Have you tried that?
Comment #33
mparker17Sounds good... let's try it!
Comment #34
mparker17Whoops...
Comment #36
mparker17Intriguing.
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 be3600
.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.
Comment #37
ianthomas_ukThat 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.
Comment #38
disasm CreditAttribution: disasm commentedThe 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.
Comment #39
disasm CreditAttribution: disasm commentedThis 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.
Comment #40
disasm CreditAttribution: disasm commentedfixes test. Need to pass query string in an options array, otherwise urlGenerator escapes the ?date= part.
Comment #41
ianthomas_ukThe test looks good, and the patch still works with defaults in the YAML, so RTBC.
Comment #42
ianthomas_ukRemove tag
Comment #43
dawehner+1
Comment #44
mparker17Awesome, thanks @disasm! I didn't know you had to pass query parameters like that. :P
Comment #45
catchCommitted/pushed to 8.x, thanks!