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 design_test_followup-23.patch952 bytesParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,409 pass(es).
[ View ]
#22 design_test_followup.patch941 byteststoeckler
PASSED: [[SimpleTest]]: [MySQL] 55,886 pass(es).
[ View ]
#19 interdiff.txt697 bytesjibran
#19 1987688-19.patch4.92 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 56,583 pass(es).
[ View ]
#14 convert-design_test_category_page-to-controller-1987688-14.patch4.95 KBcbiggins
PASSED: [[SimpleTest]]: [MySQL] 56,316 pass(es).
[ View ]
#8 convert-design_test_category_page-to-controller-1987688-8.patch4.94 KBcbiggins
PASSED: [[SimpleTest]]: [MySQL] 55,923 pass(es).
[ View ]
#2 convert-design_test_category_page-to-controller-1987688-2.patch5.03 KBcbiggins
PASSED: [[SimpleTest]]: [MySQL] 55,594 pass(es).
[ View ]

Comments

cbiggins’s picture

Assigned:Unassigned» cbiggins
cbiggins’s picture

StatusFileSize
new5.03 KB
PASSED: [[SimpleTest]]: [MySQL] 55,594 pass(es).
[ View ]

Patch attached.

Its worth noting that while this callback has a '$categories' parameter - it isn't actually used in the callback. I've left the code as-is though.

cbiggins’s picture

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, convert-design_test_category_page-to-controller-1987688-2.patch, failed testing.

cbiggins’s picture

Status:Needs work» Needs review

Kicking the bot off again as local tests were all green.

cbiggins’s picture

dawehner’s picture

Just some nitpicking...

+++ b/core/modules/system/tests/modules/design_test/lib/Drupal/design_test/Controller/DesignTestController.phpundefined
@@ -0,0 +1,62 @@
+class DesignTestController implements ControllerInterface {
...
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();
+  }
+
+  /**
+   * Constructs a DesignTestController object.
+   */
+  public function __construct() {

No need for the interface if there is nothing to inject.

cbiggins’s picture

StatusFileSize
new4.94 KB
PASSED: [[SimpleTest]]: [MySQL] 55,923 pass(es).
[ View ]

Removed unused interface and rerolled against fresh 8.x (no changes)

larowlan’s picture

+++ b/core/modules/system/tests/modules/design_test/lib/Drupal/design_test/Controller/DesignTestController.phpundefined
@@ -0,0 +1,61 @@
+use Drupal\Core\ControllerInterface;

Not needed, no DI happening here.

+++ b/core/modules/system/tests/modules/design_test/lib/Drupal/design_test/Controller/DesignTestController.phpundefined
@@ -0,0 +1,61 @@
+class DesignTestController implements ControllerInterface {

no need for 'implements ControllerInterface' when no DI

+++ b/core/modules/system/tests/modules/design_test/lib/Drupal/design_test/Controller/DesignTestController.phpundefined
@@ -0,0 +1,61 @@
+  /**
+   * {@inheritdoc}
+   */
+  public static function create() {
+    return new static();
+  }

this can be removed, no DI going on here.

+++ b/core/modules/system/tests/modules/design_test/lib/Drupal/design_test/Controller/DesignTestController.phpundefined
@@ -0,0 +1,61 @@
+  /**
+   * Constructs a DesignTestController object.
+   */
+  public function __construct() {

not needed

cbiggins’s picture

Status:Needs review» Needs work

Thanks larowlan, will update (hopefully) tonight.

dawehner’s picture

Crell agreed that it actually makes sense to add all of them now already as all of them will need injection on the long run.

larowlan’s picture

Status:Needs work» Needs review

So in that case RTBC?

dawehner’s picture

Status:Needs review» Needs work
+++ b/core/modules/system/tests/modules/design_test/lib/Drupal/design_test/Controller/DesignTestController.phpundefined
@@ -0,0 +1,61 @@
+  }

/me hides ... missing empty line , the rest is fine.

cbiggins’s picture

StatusFileSize
new4.95 KB
PASSED: [[SimpleTest]]: [MySQL] 56,316 pass(es).
[ View ]

Line added. :)

cbiggins’s picture

Status:Needs work» Needs review
dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Great, thank you for your work!

alexpott’s picture

Very minor nit...

+++ b/core/modules/system/tests/modules/design_test/lib/Drupal/design_test/Controller/DesignTestController.phpundefined
@@ -0,0 +1,62 @@
+    $build = menu_tree_output($tree);
+    return $build;

This really should just be...
return menu_tree_output($tree);

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

See #17

jibran’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new4.92 KB
PASSED: [[SimpleTest]]: [MySQL] 56,583 pass(es).
[ View ]
new697 bytes

Fixed #17

tim.plunkett’s picture

+++ b/core/modules/system/tests/modules/design_test/design_test.moduleundefined
@@ -68,21 +68,19 @@ function design_test_menu() {
   foreach ($categories as $category) {
     $items["design_test/{$category}"] = array(
       'title' => drupal_ucfirst($category),
-      'page callback' => 'design_test_category_page',
-      'page arguments' => array(1),
-      'access callback' => TRUE,
+      'route_name' => 'design_test',

This shouldn't actually work, I thought. Most other foreach loops need a RouteSubscriber

EDIT: Nevermind, that only comes into play for access checks. Since these are all _access: 'TRUE', it doesn't matter.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed d84dd29 and pushed to 8.x. Thanks!

tstoeckler’s picture

Status:Fixed» Needs review
StatusFileSize
new941 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,886 pass(es).
[ View ]
+++ b/core/modules/system/tests/modules/design_test/lib/Drupal/design_test/Controller/DesignTestController.php
@@ -0,0 +1,61 @@
+  public static function create() {

This is not compatible with the parent declaration.

It seems design_test is a bit broken in general, but I don't know in how far that is related to this patch. This fixes the fatal, at least.

ParisLiakos’s picture

Title:Convert design_test_category_page() to a new style controller» [Followup] Convert design_test_category_page() to a new style controller
Assigned:cbiggins» Unassigned
Status:Needs review» Reviewed & tested by the community
StatusFileSize
new952 bytes
PASSED: [[SimpleTest]]: [MySQL] 56,409 pass(es).
[ View ]

straight reroll for ControllerInterface moving

YesCT’s picture

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

Status:Reviewed & tested by the community» Needs review

I don't understand how tests are currently passing...

Well scanning the code it appears that the test module design_test is not being used.

alexpott’s picture

alexpott’s picture

Status:Needs review» Fixed

I'm closing this issue in favour of #2037569: Remove design_test module - obviously we still need the followup fix from this issue but lets do that over there...

Adding tests is vital since the fact that they were missing is how we have ended up with a broken module.

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