Follow-up to #2707109: Help block should not be displayed when hook_help() implementations return an empty string

Problem/Motivation

moduleInvokeAll merges the return values if they are arrays. So if two hooks return [#markup => 'something'] I'm not sure what is going to happen.

Proposed resolution

Call each hook individually

Remaining tasks

User interface changes

None

API changes

@todo

Data model changes

None

CommentFileSizeAuthor
#12 2707261-12.patch5.78 KBalexpott
#12 8-12-interdiff.txt950 bytesalexpott
#8 2707261-8.patch5.74 KBalexpott
#8 2707261-8.test-only.patch4.55 KBalexpott
#4 2707261-4.patch8.2 KBalexpott
#4 3-4-interdiff.txt1.19 KBalexpott
#3 2707261-3-do-not-test.patch4.56 KBalexpott
#3 2707261-3.test-only.patch7.87 KBalexpott
#3 2-3-interdiff.txt540 bytesalexpott
#2 2707261-2.test-only.patch7.85 KBalexpott
#2 2707261-2-do-not-test.patch4.27 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2707261-2-do-not-test.patch4.27 KB
2707261-2.test-only.patch7.85 KB

Patch attached is a failing test built on top of #2707109: Help block should not be displayed when hook_help() implementations return an empty string as that adds simple help block test coverage. The do-not-test.patch shows the relevant additions.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2-3-interdiff.txt540 bytes
2707261-3.test-only.patch7.87 KB
2707261-3-do-not-test.patch4.56 KB

Oops forget to enable the module - the test still fails as expected though...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
3-4-interdiff.txt1.19 KB
2707261-4.patch8.2 KB

Here's a patch with a fix.

The last submitted patch, 2: 2707261-2.test-only.patch, failed testing.

The last submitted patch, 3: 2707261-3.test-only.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Postponed

This looks great! Good catch also. I am ready to RTBC it, but it looks like we should wait for #2707109: Help block should not be displayed when hook_help() implementations return an empty string to be finished, at which point it will need a reroll because this patch includes the patch from that other issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Version: 8.2.x-dev » 8.1.x-dev
Status: Postponed » Needs review
FileSize
2707261-8.test-only.patch4.55 KB
2707261-8.patch5.74 KB

I think this fix is patch eligible. Rerolled now that #2707109: Help block should not be displayed when hook_help() implementations return an empty string has landed.

The last submitted patch, 8: 2707261-8.test-only.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

What's not to like? :) Good, clean code. With test. Fixes bug. Let's do it.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work

I think this should use https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Extension!ModuleH...

Also see #2616814: Delegate all hook invocations to ModuleHandler.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
8-12-interdiff.txt950 bytes
2707261-12.patch5.78 KB

@catch - good catch.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Thanks!

  • catch committed 1003490 on 8.2.x 
    Issue #2707261 by alexpott, jhodgdon: Calling moduleInvokeAll in Help...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Status: Fixed » Closed (fixed)

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