Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

Assigned: Unassigned » marcingy

Going to look at this today

marcingy’s picture

Status: Active » Needs review
FileSize
4.48 KB

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.

marcingy’s picture

Status: Needs work » Needs review

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

tim.plunkett’s picture

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', ...

dawehner’s picture

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

marcingy’s picture

FileSize
4.8 KB

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

marcingy’s picture

Status: Needs work » Needs review
marcingy’s picture

Status: Needs review » Needs work

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

marcingy’s picture

Status: Needs work » Needs review
FileSize
4.97 KB

Fixes up docs hopefully.

Status: Needs review » Needs work

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

marcingy’s picture

Status: Needs work » Needs review
FileSize
4.69 KB

OK I forgot to pull.

Status: Needs review » Needs work

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

marcingy’s picture

Status: Needs work » Needs review
FileSize
4.69 KB

Missed some changes in re-roll.

ParisLiakos’s picture

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

marcingy’s picture

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

dawehner’s picture

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

marcingy’s picture

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

dawehner’s picture

Status: Needs work » Needs review

Manually testing this patch works for me.

tim.plunkett’s picture

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.

marcingy’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.47 KB
4.48 KB

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

ParisLiakos’s picture

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

dawehner’s picture

FileSize
4.47 KB

Thanks for your review!

Fixed that.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

looks good, and works as expected

alexpott’s picture

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.