Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Niklas Fiekas’s picture

Assigned: Unassigned » Niklas Fiekas
Niklas Fiekas’s picture

Assigned: Niklas Fiekas » Unassigned
Status: Active » Needs review
FileSize
5.12 KB

Go testbot, go!

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

dawehner’s picture

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

Niklas Fiekas’s picture

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.

ParisLiakos’s picture

Status: Needs review » Closed (won't fix)
ayelet_Cr’s picture

Status: Closed (won't fix) » Active
mparker17’s picture

Assigned: Unassigned » mparker17

I'll help!

mparker17’s picture

Assigned: mparker17 » Unassigned

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

rabellamy’s picture

Assigned: Unassigned » rabellamy
rabellamy’s picture

Title: Convert aggregator_test_feed() to a new style controller » Convert aggregator_test_feed() and aggregator_test_redirect() to a new style controller
Status: Active » Needs review
FileSize
6.55 KB

Status: Needs review » Needs work

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

dawehner’s picture

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

mparker17’s picture

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

mparker17’s picture

Status: Needs work » Needs review
FileSize
2.61 KB
6.59 KB

Try this...

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

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

mparker17’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
3.7 KB
6.54 KB

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
867 bytes
6.51 KB

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

dawehner’s picture

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

xjm’s picture

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.

mparker17’s picture

Try this...

mparker17’s picture

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.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

disasm’s picture

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

Status: Needs review » Reviewed & tested by the community

A couple of random failures later ...

mparker17’s picture

XD Thanks!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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