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
#23 drupal8.aggregator-module.1987598-23.patch6.94 KBmparker17
PASSED: [[SimpleTest]]: [MySQL] 59,124 pass(es).
[ View ]
#23 interdiff.txt2.13 KBmparker17
#20 drupal8.aggregator-module.1987598-20.patch6.51 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,878 pass(es).
[ View ]
#20 interdiff.txt867 bytesdisasm
#18 drupal8.aggregator-module.1987598-18.patch6.54 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,844 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#18 interdiff.txt3.7 KBdisasm
#14 drupal8.aggregator-module.1987598-14.patch6.59 KBmparker17
FAILED: [[SimpleTest]]: [MySQL] 58,000 pass(es), 47 fail(s), and 5 exception(s).
[ View ]
#14 interdiff.txt2.61 KBmparker17
#10 drupal8.aggregator-module.1987598-10.patch6.55 KBrabellamy
FAILED: [[SimpleTest]]: [MySQL] 58,049 pass(es), 37 fail(s), and 0 exception(s).
[ View ]
#2 1987598-aggregator-test-feed-controller-1.patch5.12 KBNiklas Fiekas
PASSED: [[SimpleTest]]: [MySQL] 55,700 pass(es).
[ View ]

Comments

Assigned:Unassigned» Niklas Fiekas

Assigned:Niklas Fiekas» Unassigned
Status:Active» Needs review
StatusFileSize
new5.12 KB
PASSED: [[SimpleTest]]: [MySQL] 55,700 pass(es).
[ View ]

Go testbot, go!

(Later maybe we should consider #2004022: Move aggregator_test module into its own directory.)

+++ b/core/modules/aggregator/tests/aggregator_test.moduleundefined
@@ -24,49 +17,6 @@ function aggregator_test_menu() {
-  if ($use_last_modified) {
-    drupal_add_http_header('Last-Modified', gmdate(DATE_RFC1123, $last_modified));
-  }
...
-  drupal_add_http_header('Expires', 'Sun, 19 Nov 1978 05:00:00 GMT');
+++ b/core/modules/aggregator/tests/lib/Drupal/aggregator_test/Controller/AggregatorTestController.phpundefined
@@ -0,0 +1,59 @@
+    $last_modified = new \DateTime('Sun, 19 Nov 1978 05:00:00 GMT');
...
+    if ($use_last_modified) {
+      $response->setLastModified($last_modified);
+    }

Is it just me that this logic is not 100% the same? Dries birthday got added before all the time.

+++ b/core/modules/aggregator/tests/aggregator_test.routing.ymlundefined
@@ -0,0 +1,8 @@
+    use_etag: '0'
+    use_last_modified: '0'

can't we also use false here which then get converted to FALSE?

Intresting. Have a look:

Before the patch:

http://localhost/drupal-8/aggregator/test-feed/1/1
GET /drupal-8/aggregator/test-feed/1/1 HTTP/1.1
Host: localhost
User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:21.0) Gecko/20100101 Firefox/21.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: de-de,de;q=0.8,en-us;q=0.5,en;q=0.3
Accept-Encoding: gzip, deflate
Cookie: Drupal.toolbar.collapsed=0
Connection: keep-alive
Pragma: no-cache
Cache-Control: no-cache
HTTP/1.1 200 OK
Date: Tue, 28 May 2013 10:34:08 GMT
Server: Apache/2.2.22 (Ubuntu)
X-Powered-By: PHP/5.4.9-4ubuntu2
Last-Modified: Sun, 19 Nov 1978 05:00:00 +0000, Tue, 28 May 2013 10:34:08 +0000 <-- Notice two different Last-Modified
Etag: h__lfVU1SyY-w28Xq0pfxb8gcBNeuD-43cpt_zIe98w, "1369737248" <-- Notice two ETags
Expires: Sun, 19 Nov 1978 05:00:00 GMT, Sun, 19 Nov 1978 05:00:00 GMT <-- Notice the expires header twice
Cache-Control: must-revalidate, must-revalidate, no-cache, post-check=0, pre-check=0, private <-- Notice must-revalidate twice
X-UA-Compatible: IE=edge,chrome=1
Content-Language: en
Vary: Accept-Encoding
Content-Encoding: gzip
Keep-Alive: timeout=5, max=100
Connection: Keep-Alive
Transfer-Encoding: chunked
Content-Type: text/html; charset=UTF-8 <-- Notice the wrong content type

After the patch:

http://localhost/drupal-8/aggregator/test-feed/1/1
GET /drupal-8/aggregator/test-feed/1/1 HTTP/1.1
Host: localhost
User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:21.0) Gecko/20100101 Firefox/21.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: de-de,de;q=0.8,en-us;q=0.5,en;q=0.3
Accept-Encoding: gzip, deflate
Cookie: Drupal.toolbar.collapsed=0
Connection: keep-alive
Pragma: no-cache
Cache-Control: no-cache
HTTP/1.1 200 OK
Date: Tue, 28 May 2013 10:36:40 GMT
Server: Apache/2.2.22 (Ubuntu)
X-Powered-By: PHP/5.4.9-4ubuntu2
Cache-Control: must-revalidate, no-cache, post-check=0, pre-check=0, private
Last-Modified: Tue, 28 May 2013 10:36:40 +0000 <-- Why is this so?
Etag: "1369737400" <-- Is this the generated ETag?
X-UA-Compatible: IE=edge,chrome=1
Content-Language: en
Expires: Sun, 19 Nov 1978 05:00:00 GMT <-- Looks correct
Accept-Ranges: bytes
content-transfer-encoding: binary
Vary: Accept-Encoding
Content-Encoding: gzip
Keep-Alive: timeout=5, max=100
Connection: Keep-Alive
Transfer-Encoding: chunked
Content-Type: application/rss+xml; charset=utf-8

Recorded with Firefox Live Http headers. I don't think I fully understand what's going on here.

Status:Needs review» Closed (won't fix)

Status:Closed (won't fix)» Active

Assigned:Unassigned» mparker17

I'll help!

Assigned:mparker17» Unassigned

Whoops... @rabellamy is working on these :P

Assigned:Unassigned» rabellamy

Title:Convert aggregator_test_feed() to a new style controllerConvert aggregator_test_feed() and aggregator_test_redirect() to a new style controller
Status:Active» Needs review
StatusFileSize
new6.55 KB
FAILED: [[SimpleTest]]: [MySQL] 58,049 pass(es), 37 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal8.aggregator-module.1987598-10.patch, failed testing.

+++ b/core/modules/aggregator/tests/modules/aggregator_test/lib/Drupal/aggregator_test/Controller/AggregatorTestRssController.php
@@ -0,0 +1,72 @@
+      drupal_add_http_header('Last-Modified', gmdate(DATE_RFC1123, $last_modified));
...
+      drupal_add_http_header('ETag', $etag);
...
+    drupal_add_http_header('Expires', 'Sun, 19 Nov 1978 05:00:00 GMT');
+    drupal_add_http_header('Cache-Control', 'must-revalidate');
+    drupal_add_http_header('Content-Type', 'application/rss+xml; charset=utf-8');

What you should do instead is to create a response object and set all this header onto it and finally just return it.

*sad trombone* I was going to help convert those drupal_add_http_headers but apparently Chicago Midway doesn't have Wi-Fi. Sorry :(

Status:Needs work» Needs review
StatusFileSize
new2.61 KB
new6.59 KB
FAILED: [[SimpleTest]]: [MySQL] 58,000 pass(es), 47 fail(s), and 5 exception(s).
[ View ]

Try this...

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

The last submitted patch, drupal8.aggregator-module.1987598-14.patch, failed testing.

Status:Needs work» Needs review

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

The last submitted patch, drupal8.aggregator-module.1987598-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.7 KB
new6.54 KB
FAILED: [[SimpleTest]]: [MySQL] 58,844 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Was missing params on the route for the optional parameters. Also, extending ControllerBase to get access to urlGenerator() and $this->redirect. Renamed methods to testFeed and testRedirect.

Status:Needs review» Needs work

The last submitted patch, drupal8.aggregator-module.1987598-18.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new867 bytes
new6.51 KB
PASSED: [[SimpleTest]]: [MySQL] 58,878 pass(es).
[ View ]

passing route to $this->redirect should resolve the last failure.

Status:Needs review» Needs work
  1. +++ w/core/modules/aggregator/tests/modules/aggregator_test/lib/Drupal/aggregator_test/Controller/AggregatorTestRssController.php
    @@ -0,0 +1,66 @@
    +
    +class AggregatorTestRssController extends ControllerBase {

    Can we have a little bit of docs on there?

  2. +++ w/core/modules/aggregator/tests/modules/aggregator_test/lib/Drupal/aggregator_test/Controller/AggregatorTestRssController.php
    @@ -0,0 +1,66 @@
    +   * @param $use_etag
    +   *   Set TRUE to send an etag.
    +   */
    +  public function testFeed($use_last_modified, $use_etag, Request $request) {
    ...
    +
    +  public function testRedirect() {
    +    return $this->redirect('aggregator_test_feed', array(), 301);
    +  }
    +

    Missing @return

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

StatusFileSize
new2.13 KB
new6.94 KB
PASSED: [[SimpleTest]]: [MySQL] 59,124 pass(es).
[ View ]

Try this...

Status:Needs work» Needs review

Whoops... paging Dr. Testbot...

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

The last submitted patch, drupal8.aggregator-module.1987598-23.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal8.aggregator-module.1987598-23.patch, failed testing.

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

Status:Needs review» Reviewed & tested by the community

A couple of random failures later ...

XD Thanks!

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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