Allow help text to be overridden from an .extra.hlp file

[colemanw]

Overview

Help text is written in .hlp smarty files. You can append to the text by adding your own .extra.hlp file with the same name. This allows you to also override it.

NOTE the pageRun hook is removed from CRM_Core_Page_Inline_Help in this PR. Per discussion - see #13488 (comment)

Before

.extra.hlp files could append but not override help text.

After

.extra.hlp files can be set to override on a per-item basis.

How-to

1. Create an extension (and enable it)

2. Add a file: templates/CRM/Activity/Form/Activity.extra.hlp

3. Add this to the file:

    {htxt id="assignee_contact_id" override=1}
      <p>I'm extra!</p>
    {/htxt}
   

4. Go to the "new activity" screen and click the help icon for "Assigned to"

5. Note that the default help text is gone and only the text "I'm extra!" prints.

6. If you remove the overrride=1 param, both the default help text and the new text appear, which was the previous behavior.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[monishdeb]

Tested working fine. Here's a screencast as 'How-to' steps:

On the other hand, I encountered a separate issue which is caused by pageRun hook. Suppose some extension called a $page->getVar() inside a pageRun hook then it throws a internal error when we click on hlp icon as:

<em class="placeholder">Error</em>: Call to undefined method CRM_Core_Page_Inline_Help::getVar() in <em class="placeholder">shoreditch_civicrm_pageRun()</em> (line <em class="placeholder">156</em> of <em class="placeholder">/Users/monish/src/civi-extensions/org.civicrm.shoreditch/shoreditch.php</em>).

In this above example the error is generted from here as CRM_Core_Page_Inline_Help don't extend CRM_Core_Page this it don't inherit getVar() as triggered from here

[colemanw]

Hmm. If the pageRun hook doesn't actually work (and wasn't running anyway 99% of the time) maybe we should just remove it.

[colemanw]

Looking at the commit history, that hook invocation was added in 6eea17d and @eileenmcnaughton references https://issues.civicrm.org/jira/browse/CRM-14021 in the commit msg. However, that issue description makes no mention of the hook, so I'm guessing she added it as an afterthought rather than because it was actually needed for something. As @monishdeb noted, the way it's invoked makes it very unlikely to be useful to anyone.

@eileenmcnaughton can you confirm it's safe to remove?

[monishdeb]

I agree with removing the Hook call in CRM_Core_Page_Inline_Help, but will wait for @eileenmcnaughton reply if theres any unwanted side effect to extensions, if we remove the hook call

[seamuslee001]

@monishdeb @colemanw I believe I am currently using the pageRun hook to manipulate the event info page. As such I would be against removing it. I would note that the hook doc iirc mentions it doesn’t run on forms only quickform pages

[colemanw]

@seamuslee001 we're not proposing to remove the entire hook. Just to stop invoking it in this one place when displaying help bubbles. This hook invocation is inconsistent and problematic because:

• The help page is not actually a CRM_Core_Page so the signature is broken.
• The hook only fires in the unusual circumstance of a .extra.hlp file existing.

I expect that removing it here will affect precisely no one. But will wait for @eileenmcnaughton to weigh in.

[seamuslee001]

Ah on that would be ok then. I miss interpreted Monish’s statement when he said he was ok removing the hook I thought that was completely removing not just one invocation.

[monishdeb]

@seamuslee001 I realized it and so I edited my comment soon after I saw your comment. Sorry about that :(

[colemanw]

@civicrm-builder retest this please

[eileenmcnaughton]

I can't recall anything about this!

I suspect the reason for the hook call is to allow extra variables to be assigned but I also think the discussion here is probably enough to warrant removing it but we should flag it in the hook change log & make sure the overview marks it such that it gets picked up as a change to highlight in release notes

[colemanw]

Thanks Eileen.
Marking this as merge-ready pending a green light from @lcdservices

[lcdservices]

@colemanw @eileenmcnaughton tested and this looks good. Implemented an extension to handle all inline help modifications.

[eileenmcnaughton]

@colemanw do you want to update the change log as we merge this?

[colemanw]

Thanks @eileenmcnaughton. Done: civicrm/civicrm-dev-docs@94160f6