hook_help() should allow render arrays for return values [#2665672]

Because HelpController::helpPage() does this:

  $temp = $this->moduleHandler()->invoke($name, 'help', array("help.page.$name", $this->routeMatch));
...
  $build['top']['#markup'] = $temp;

it is implicitly assuming that the return value of hook_help() is a string, not a render array. This is documented on the hook_help() documentation in help.api.php. However, it would be way better if it accepted a render array.

All I think we'd need to do is something like

if (is_array($temp)) {
   $build['top'] = $temp;
}
else {
   $build['top']['#markup'] = $temp;
}

Also, HelpBlock::getActiveHelp() would need a fix for this, because it currently does:

 $help = $this->moduleHandler->invokeAll('help', array($this->routeMatch->getRouteName(), $this->routeMatch));
  return $help ? implode("\n", $help) : '';

and instead it would be better if it did something like...

$build = [];
foreach ($help as $item) {
  if (is_array($item)) {
    $build[] = $item;
  }
  else {
    $build[] = ['#markup' => $item];
 }
}
return $build;

return $help;

on that second line and the array

Comment	File	Size	Author
#7	interdiff.txt	462 bytes	jhodgdon
#7	2665672-7.patch	4.38 KB	jhodgdon
#7	8.1.x: PHP 5.5 & MySQL 5.5 18,779 pass
#4	2665672-4.patch	4.38 KB	jhodgdon
#4	8.1.x: PHP 5.5 & MySQL 5.5 CI error
#2	2665672.patch	3.64 KB	jhodgdon
#2	8.1.x: PHP 5.5 & MySQL 5.5 18,769 pass, 28 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

9 February 2016 at 21:35

jhodgdon created an issue. See original summary.

Comment #2

jhodgdon

she/her

English

CreditAttribution: jhodgdon at Poplar ProductivityWare for Drupal Association commented 15 February 2016 at 23:03

Status:

Active

» Needs review

File	Size
2665672.patch	3.64 KB
8.1.x: PHP 5.5 & MySQL 5.5 18,769 pass, 28 fail

Let's see how many tests this patch breaks. I haven't actually tested it, but there are existing tests that do verify that the main Help page at admin/help and various admin/help/[module] at least work and have some specific bits of text on them. This patch changes the help_help() implementation of hook_help() to return render arrays for the help block that goes on admin/help and the admin/help/help page to both return render arrays. And it leaves the other hook_help() implementations alone.

So if the existing tests pass, then this patch would seem to have worked.

Comment #3

15 February 2016 at 23:37

Status:

Needs review

» Needs work

The last submitted patch, 2: 2665672.patch, failed testing.

Comment #4

jhodgdon

she/her

English

CreditAttribution: jhodgdon at Poplar ProductivityWare for Drupal Association commented 16 February 2016 at 00:01

Status:

Needs work

» Needs review

File	Size
2665672-4.patch	4.38 KB
8.1.x: PHP 5.5 & MySQL 5.5 CI error

1 file was hidden/shown/deleted

File	Size
2665672.patch	3.64 KB
8.1.x: PHP 5.5 & MySQL 5.5 18,769 pass, 28 fail

Whoops. Try this one. Not bothering with interdiff since no one is following this issue yet except me.

Comment #5

16 February 2016 at 00:03

Status:

Needs review

» Needs work

The last submitted patch, 4: 2665672-4.patch, failed testing.

Comment #6

tic2000 CreditAttribution: tic2000 at Intellix commented 16 February 2016 at 11:38

+    elseif is_string($help) {

() are missing.

Comment #7

jhodgdon

she/her

English

CreditAttribution: jhodgdon at Poplar ProductivityWare for Drupal Association commented 16 February 2016 at 16:24

Status:

Needs work

» Needs review

File	Size
2665672-7.patch	4.38 KB
8.1.x: PHP 5.5 & MySQL 5.5 18,779 pass
interdiff.txt	462 bytes

1 file was hidden/shown/deleted

File	Size
2665672-4.patch	4.38 KB
8.1.x: PHP 5.5 & MySQL 5.5 CI error

Doh. Thanks!

Comment #8

tic2000 CreditAttribution: tic2000 at Intellix commented 16 February 2016 at 19:34

-        $build['top']['#markup'] = $this->t('No help is available for module %module.', array('%module' => $module_name));
+        $build['top'] = ['#markup' => $this->t('No help is available for module %module.', array('%module' => $module_name))];

Why the need for this change?

Beside that everything looks fine to me. I also tested the patch and the changed pages also as some unchanged and all render as they should.

Comment #9

jhodgdon

she/her

English

CreditAttribution: jhodgdon at Poplar ProductivityWare for Drupal Association commented 16 February 2016 at 19:41

Yeah, it's not strictly necessary. I just did it to make the code there a bit more consistent, but I can take it out of the patch if it's bad.

Thanks for testing!

Comment #10

tic2000 CreditAttribution: tic2000 at Intellix commented 16 February 2016 at 19:47

Status:

Needs review

» Reviewed & tested by the community

It's not bad, but technically it's not the exact same. It makes no difference now cause $build['top'] was not set before.

I RTBC. If someone feels code should not be touched just for consistency purpose they can change to NW for that.

Comment #11

jhodgdon

she/her

English

CreditAttribution: jhodgdon at Poplar ProductivityWare for Drupal Association commented 16 February 2016 at 19:54

Thanks! I added a short draft change notice to this.
https://www.drupal.org/node/2669988

Note that the existing help tests cover this patch, because some help blocks and help texts are returning strings, and the ones in module Help are now returning render arrays. Both types are visited in tests and verified to still work.

Comment #12

17 February 2016 at 03:59

catch committed c7af20e on 8.1.x

Issue #2665672 by jhodgdon: hook_help() should allow render arrays for...

Comment #13

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 17 February 2016 at 03:59

Status:

Reviewed & tested by the community

» Fixed

Makes sense to me. Committed/pushed to 8.1.x, thanks!

Comment #14

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 17 February 2016 at 10:22

YAY! This also means help hook implementations can finally provide cacheability metadata.

I think it'd be nice if the CR explicitly mentioned this:

+++ b/core/modules/help/help.api.php
@@ -40,8 +40,9 @@
- * @return string
- *   A localized string containing the help text.
+ * @return string|array
+ *   A render array, localized string, or object that can be rendered into
+ *   a string, containing the help text.

Comment #15

jhodgdon

she/her

English

CreditAttribution: jhodgdon at Poplar ProductivityWare for Drupal Association commented 17 February 2016 at 16:26

Do you mean you think that the hook_help() API docs should mention this, or just the change record?

Comment #16

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 17 February 2016 at 17:10

I was talking just about the change record.

Comment #17

jhodgdon

she/her

English

CreditAttribution: jhodgdon at Poplar ProductivityWare for Drupal Association commented 17 February 2016 at 17:18

OK. I put a small note in there, but you can edit it to say more if you think that's a good idea? I am not necessarily convinced that we need one specially there, as opposed to generically "people need to realize about cache metadata in their render arrays", but it can't hurt. ;)