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.

CommentFileSizeAuthor
#1 aggregator_test_controller-1985550-1.patch6.27 KBplopesc

Comments

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Active » Needs review
StatusFileSize
new aggregator_test_controller-1985550-1.patch6.27 KB

Hello

Attaching patch that converts aggregator_test callbacks to a Controller.

Regards.

Status: Needs review » Needs work

The last submitted patch, aggregator_test_controller-1985550-1.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/aggregator/tests/aggregator_test.routing.ymlundefined
@@ -0,0 +1,13 @@
+    _permission: 'access content'
...
+    _permission: 'access content'

Just in general I'm wondering whether we should avoid using that basic permission if it's not actually used? It could be just replaced by '_access' : 'TRUE'

+++ b/core/modules/aggregator/tests/lib/Drupal/aggregator_test/Controller/AggregatorTestController.phpundefined
@@ -0,0 +1,70 @@
+   * @param $use_last_modified
...
+   * @param $use_etag

Should properly follow the coding standards, @see node 1354 :)

+++ b/core/modules/aggregator/tests/lib/Drupal/aggregator_test/Controller/AggregatorTestController.phpundefined
@@ -0,0 +1,70 @@
+      drupal_add_http_header('Last-Modified', gmdate(DATE_RFC1123, $last_modified));
...
+      drupal_add_http_header('ETag', $etag);
...
+      drupal_add_http_header('Status', '304 Not Modified');

I guess we should replace drupal_add_http_header with a changed value on a Response object?

+++ b/core/modules/aggregator/tests/lib/Drupal/aggregator_test/Controller/AggregatorTestController.phpundefined
@@ -0,0 +1,70 @@
+    print $feed;

Yeah that's also something which should be actually part of a response object.

+++ b/core/modules/aggregator/tests/lib/Drupal/aggregator_test/Controller/AggregatorTestController.phpundefined
@@ -0,0 +1,70 @@
+  function aggregatorTestRedirect() {

needs docs.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented

Hello

@dawehner, apart from your comments, I found a couple of issues when converting these callbacks to a Controller, detailed below:

  • Old hook_menu implementation allowed to add optional extra parameters aded to page callback function. However, in the new WSCCI conversion, I don't know exactly how it should be managed. Could it be declared as 3 different routing items using the same Controller? Are there a better option not explained in doc?
  • In aggregatorTestRedirect()method, tests does not work when reurning a RedirectResponse object, but works fine, when using drupal_goto

Regards

xjm’s picture

xjm
she/her
Primary language English
commented

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.

disasm’s picture

disasm commented
Status: Needs work » Closed (duplicate)

This is a duplicate of #1987598: Convert aggregator_test_feed() and aggregator_test_redirect() to a new style controller.