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

Assigned:Unassigned» cbiggins

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.

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.

Status:Needs work» Needs review

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

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.

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

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

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

Status:Needs review» Needs work

Thanks larowlan, will update (hopefully) tonight.

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.

Status:Needs work» Needs review

So in that case RTBC?

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.

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

Line added. :)

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

Great, thank you for your work!

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);

Status:Reviewed & tested by the community» Needs work

See #17

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

Fixed #17

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

Status:Reviewed & tested by the community» Fixed

Committed d84dd29 and pushed to 8.x. Thanks!

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.

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

Issue tags:+RTBC July 1

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

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.

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.