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
#24 drupal-1979030-24.patch4.47 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,048 pass(es).
[ View ]
#22 drupal-1979030-22.patch4.48 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,843 pass(es).
[ View ]
#22 interdiff.txt2.47 KBdawehner
#14 help-controller-2.patch4.69 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 55,640 pass(es).
[ View ]
#12 help-controller.patch4.69 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 55,370 pass(es), 130 fail(s), and 97 exception(s).
[ View ]
#10 help-router.patch4.97 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch help-router.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#7 hel-controller.patch4.8 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 55,427 pass(es).
[ View ]
#2 convert-help-page.patch4.48 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 55,788 pass(es).
[ View ]

Comments

Assigned:Unassigned» marcingy

Going to look at this today

Status:Active» Needs review
StatusFileSize
new4.48 KB
PASSED: [[SimpleTest]]: [MySQL] 55,788 pass(es).
[ View ]

Passes help tests locally so lets see what the bot thinks

Status:Needs review» Needs work

The last submitted patch, convert-help-page.patch, failed testing.

Status:Needs work» Needs review

#2: convert-help-page.patch queued for re-testing.

Status:Needs review» Needs work

+++ b/core/modules/help/lib/Drupal/help/Controller/HelpController.phpundefined
@@ -0,0 +1,67 @@
+use Drupal\Core\ControllerInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
...
+class HelpController implements ControllerInterface {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();
+  }
+
+  /**
+   * Constructs a HelpController object.
+   */
+  public function __construct() {

If you're not going to inject anything, you don't need ControllerInterface

+++ b/core/modules/help/lib/Drupal/help/Controller/HelpController.phpundefined
@@ -0,0 +1,67 @@
+    $output = '';
...
+        $output .= t('No help is available for module %module.', array('%module' => $info[$name]['name']));
...
+        $output .= $temp;
...
+        $output .= theme('item_list', array('items' => $links, 'title' => t('@module administration pages', array('@module' => $info[$name]['name']))));
...
+    return $output;

Ideally $output would be a render array, and you would do $output[] = array('#theme' => 'item_list', ...

+++ b/core/modules/help/lib/Drupal/help/Controller/HelpController.phpundefined
@@ -0,0 +1,67 @@
+    if (module_hook($name, 'help')) {
...
+      $temp = module_invoke($name, 'help', "admin/help#$name", drupal_help_arg());

That's another place where the module handler could be used.

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

The issue with the render array approach is that hook_help returns strings which opens up a much wider issue and introduces so
many chance for bikesheding. Lets deal with any changes in this area in a follow up?

The module manager is now being injected

Status:Needs work» Needs review

Status:Needs review» Needs work

Need to fix up docs and should also have used an interface for module handler.

Status:Needs work» Needs review
StatusFileSize
new4.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch help-router.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixes up docs hopefully.

Status:Needs review» Needs work

The last submitted patch, help-router.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.69 KB
FAILED: [[SimpleTest]]: [MySQL] 55,370 pass(es), 130 fail(s), and 97 exception(s).
[ View ]

OK I forgot to pull.

Status:Needs review» Needs work

The last submitted patch, help-controller.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.69 KB
PASSED: [[SimpleTest]]: [MySQL] 55,640 pass(es).
[ View ]

Missed some changes in re-roll.

Status:Needs review» Needs work

+++ b/core/modules/help/lib/Drupal/help/Controller/HelpController.phpundefined
@@ -28,8 +28,8 @@ class HelpController implements ControllerInterface {
-  public function __construct(ModuleHandlerInterface $module_handler) {
-    $this->moduleHandler = $module_handler;
+  public function __construct(ModuleHandlerInterface $moduleHandler) {
+    $this->moduleHandler = $moduleHandler;

unneeded change

+++ b/core/modules/help/lib/Drupal/help/Controller/HelpController.phpundefined
@@ -87,4 +87,43 @@ protected function helpLinksAsList() {
+   * @return String
+   *   A string containing help page text.
...
+    $output = '';
...
+        $output .= t('No help is available for module %module.', array('%module' => $info[$name]['name']));
...
+        $output .= $temp;
...
+        $output .= theme('item_list', array('items' => $links, 'title' => t('@module administration pages', array('@module' => $info[$name]['name']))));

this should be turned to a render array

+++ b/core/modules/help/lib/Drupal/help/Controller/HelpController.phpundefined
@@ -87,4 +87,43 @@ protected function helpLinksAsList() {
+   * @see help_menu()

this should be removed

See #7 re render array changing it has much bigger implication as hook_help returns strings. Other parts I'll look to fix up.

Well the rest of the function can still use ['#markup'] for example.

It can't as it is concating a string and array potentially :)

Status:Needs work» Needs review

Manually testing this patch works for me.

Status:Needs review» Needs work

+++ b/core/modules/help/lib/Drupal/help/Controller/HelpController.phpundefined
@@ -28,8 +28,8 @@ class HelpController implements ControllerInterface {
-  public function __construct(ModuleHandlerInterface $module_handler) {
-    $this->moduleHandler = $module_handler;
+  public function __construct(ModuleHandlerInterface $moduleHandler) {
+    $this->moduleHandler = $moduleHandler;

This hunk should be reverted

+++ b/core/modules/help/lib/Drupal/help/Controller/HelpController.phpundefined
@@ -87,4 +87,43 @@ protected function helpLinksAsList() {
+   * @return String
+   *   A string containing help page text.

This should return a render array. Let's take the extra steps here to do that, please. (But if we don't, this should be lowercase "string")

+++ b/core/modules/help/lib/Drupal/help/Controller/HelpController.phpundefined
@@ -87,4 +87,43 @@ protected function helpLinksAsList() {
+        $output .= $temp;

$output[$name]['#markup'] = render($temp); or similar should suffice.
And add a @todo to have hook_help always return a render array.

+++ b/core/modules/help/lib/Drupal/help/Controller/HelpController.phpundefined
@@ -87,4 +87,43 @@ protected function helpLinksAsList() {
+      if (!empty($admin_tasks)) {
+        $links = array();
+        foreach ($admin_tasks as $task) {
+          $links[] = l($task['title'], $task['link_path'], $task['localized_options']);
+        }
+        $output .= theme('item_list', array('items' => $links, 'title' => t('@module administration pages', array('@module' => $info[$name]['name']))));

Yeah, let's switch this to '#theme' => 'links', here.

Assigned:marcingy» Unassigned

So in my eyes a half baked changed to a render array is pointless because well it is isn't an array that can truly be manipulated and of these render array changes should be a follow up.

Status:Needs work» Needs review
StatusFileSize
new2.47 KB
new4.48 KB
PASSED: [[SimpleTest]]: [MySQL] 55,843 pass(es).
[ View ]

I disagree that half baked changes are bad. Small improvements are also worth to be made.

agreed, thanks for pushing this!

+++ b/core/modules/help/lib/Drupal/help/Controller/HelpController.phpundefined
@@ -87,4 +87,53 @@ protected function helpLinksAsList() {
+   * @return String
+   *   A string containing help page text.

i guess its array now:)

+++ b/core/modules/help/lib/Drupal/help/Controller/HelpController.phpundefined
@@ -87,4 +87,53 @@ protected function helpLinksAsList() {
+   * @see help_menu()

needs removal

StatusFileSize
new4.47 KB
PASSED: [[SimpleTest]]: [MySQL] 57,048 pass(es).
[ View ]

Thanks for your review!

Fixed that.

Status:Needs review» Reviewed & tested by the community

looks good, and works as expected

Status:Reviewed & tested by the community» Fixed

Committed 5a31f62 and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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