Updated: Comment #N

Problem/Motivation

Throughout this cycle we've been working to replace uses of path with route_name to make hot-swapping paths a snap.

The signature of hook_help() is still:

hook_help($path, $arg)

The $path argument is used as follows:

a) The special path "admin/help#modulename" is passed in to retrieve the module overview page accessible from admin/help. This version is invoked from a number of places.

b) Other paths are passed in, when a given page is being rendered/built, to see if a module has any help to add to the top of the page. This version is invoked from SystemHelpBlock::getActiveHelp(). However, it does not work in D8 for pages whose paths contain wildcards, such as:
- node/%/outline
- node/%/edit

For case (b), we should definitely be using routes and not paths.

Proposed resolution

Replace $path and $arg with $request and $route_name in the parameters.

Remaining tasks

N/A

User interface changes

None, except hopefully pages whose help is currently broken will get their help back.

API changes

The signature of hook_help() changes

Files: 
CommentFileSizeAuthor
#67 interdiff.txt967 byteseffulgentsia
#67 help-2183113-67.patch125.7 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,490 pass(es).
[ View ]
#61 interdiff.txt3.45 KBeffulgentsia
#61 help-2183113-61.patch125.32 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,389 pass(es).
[ View ]
#60 help-2183113-60.patch124.9 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,430 pass(es).
[ View ]
#59 help-2183113-54-reroll.patch124.9 KBjhodgdon
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch help-2183113-54-reroll.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#54 help-2183113-54.patch124.9 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch help-2183113-54.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#54 interdiff.txt651 bytestim.plunkett

Comments

jhodgdon’s picture

This may be a duplicate of #244090: Tie help into menu router ... at least it's related?

larowlan’s picture

Ooh neat

larowlan’s picture

Issue summary:View changes
Status:Active» Needs review
StatusFileSize
new108.27 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.module.
[ View ]

More reasons why this is needed:

List of help-paths that are no longer working in HEAD:

node/%/outline (book module)
admin/config/development/configuration/sync (no such path)
admin/structure/block/manage/%
admin/structure/block/add/%/%
admin/structure/types/manage/%/form-display
admin/structure/types/manage/%/display
node/%/revisions
node/%/edit
admin/config/user-interface/shortcut/%
admin/structure/block/manage for powered-by block
admin/people/search - can't find this in user.routing.yml - assume its gone

hook_help() implementations made so -much cleaner

block_help()
image_help()
node_help()
taxonomy_help()

Note this patch *doesn't* break BC, it retains $path and $arg, but appends the $route_name and $request - and then updates all core hook_help() implementations to use the new (in my opinion) superior arguments.

Also search_help() is a weird one :)

Status:Needs review» Needs work

The last submitted patch, 3: help-path-2183113.1.patch, failed testing.

The last submitted patch, 3: help-path-2183113.1.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new108.32 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
new662 bytes

one too many use statements, I blame phpstorm it did the import for me

Status:Needs review» Needs work

The last submitted patch, 6: help-path-2183113.2.patch, failed testing.

larowlan’s picture

6: help-path-2183113.2.patch queued for re-testing.

The last submitted patch, 6: help-path-2183113.2.patch, failed testing.

larowlan’s picture

I can install and enable simpletest locally.
Any ideas?

herom’s picture

Status:Needs work» Needs review
StatusFileSize
new2.56 KB
new109.7 KB
PASSED: [[SimpleTest]]: [MySQL] 63,326 pass(es).
[ View ]

I could produce an error by enabling the help module! Let's see if this is related to the simpletest problem.

jhodgdon’s picture

Why does this patch need

+    case 'help.page.action':
+    case 'help.action':

I don't get why there should be two cases for the main module help in every hook_help implementation? That just seems wrong.

Also... The new signature for hook_help() makes zero sense to me. I cannot see a reason why hook_help() should ever need to know more than the route name and the route parameters to figure out what help to display. So rather than removing the existing path/arg parameters that were there before, you've left those in and added route name and $request? Why? This is way more complicated than I think it needs to be.

I also think that several of the updates that are using $request will not work the way they are supposed to, but that will need to be tested... Can we try to have a sane API signature for the hook, and see if it will work?

larowlan’s picture

There is a distinction between help.page.{module} and help.main because of help module.
As it has two different sets of help text for the help landing page (admin/help) and the admin/help/help page.
No other module does, but could.
I agree $route_name and $route_params should be enough.
I left the existing parameters to be sympathetic and not cause an API break.
If we decide we want to remove them, no worries.
I used request instead of $route_params because there might be other cases (outside core) that need to be more specific. But happy to drop that back to just $route_params.
Thanks herom, I indeed missed that hunk and should have tried enabling simpletest from the UI, I was certain the bot used drush to enable simpletest, so I only tested that.

jhodgdon’s picture

If you are going to improve the API, improve the API. Keeping path/args in there is not going to improve the API. It just added a lot of code and didn't clean up much.

And regarding that help.page.(module) vs. help.(module)... This is really rather silly. The help module is currently, for some unknown reason, providing "help" for the top of the Help landing page (admin/help) via hook_help(). But that could just be moved to the Controller that generates the page -- there is not really a good reason it is inside hook_help().

But that still doesn't explain why the Actions module has

+    case 'help.page.action':
+    case 'help.action':

It seems like the first is really the route called "help.page" with a route argument of name=action, and I really have no idea at all what the second route is. I don't believe that there is a route called "help.action", and this is a switch statement on routes, so ... I don't get why that is there at all?

larowlan’s picture

Yes you are right, action should be help.main and help.page.action

larowlan’s picture

Will refactor as suggested, thanks for the feedback!

jhodgdon’s picture

Um... Action shouldn't be adding anything to help.main -- that is only the Help module itself, which needs to provide the text at the top of admin/help. Right? If every module does this, I think if you go to admin/help you are going to see all of the modules' help there, right?

sun’s picture

Assigned:Unassigned» catch

Is there any particular reason for why we have to keep the $path + $arg arguments of hook_help(), aside from backwards-compatibility?

If it's just for BC, then I think we should remove them as part of this patch, because the resulting "double signature", as visible in the patch, does not make much sense to me... :-)

Of course, this suggestion needs maintainer approval, so assigning to @catch for direction.

jhodgdon’s picture

Status:Needs review» Needs work

My previous review in #14 still applies as well.

And I completely agree with sun. We should either use routes or paths but not both. Routes are better. This is a sane change in line with the other changes we've done in 8 for routes.

catch’s picture

Assigned:catch» Unassigned

Yes I think it's fine to change that as part of overall router/route conversion.

jhodgdon’s picture

Issue tags:+Approved API change
effulgentsia’s picture

Priority:Normal» Major

Yay for this issue. Raising to Major since #2239009: Remove public direct usage of the '_system_path' request attribute is currently critical. In fact, maybe this issue should be a critical beta blocker? Not sure though.

I agree that using $route_name is much better than $path. But, are we sure we want to retain hook_help() at all? What about adding a _help callable in the *.routing.yml files, so each route can point to, for example, a method on the controller class that can return the help text?

jhodgdon’s picture

I like the suggestion in #23.

I think we would in that case want to keep hook_help() for its main use -- for modules to return their main module help text (the text that appears on the admin/help page), and get rid of the arguments.

Then we would use the _help in routing.yml files (and dynamic routes too presumably?) to provide help to appear at the top of particular pages.

larowlan’s picture

I think that is outside the scope of this issue.
I appreciate that work is being done elsewhere to stop accessing $request->attributes, but there is no reason why we can't use the RouteMatch concept here too.
RouteMatch->getAllArguments() and RouteMatch->getRouteName() are all hook help will need with the new signature

effulgentsia’s picture

$request->attributes aside, this issue is an approved API change to break all hook_help() implementations per #20. So, let's change it to what we think is best. If we think a giant switch/case construct in hook_help() implementations is best, then ok, but if we think a _help callable attached to routes is more consistent with D8 coding style, then let's do that in this issue, rather than breaking hook_help() API twice.

jhodgdon’s picture

The dual purpose of hook_help() has always been a bit weird (to say the least) anyway -- you have to put an artificial "path" in there now to indicate "the module's main help". We'll either have to replicate that to convert it to route names, or (as #26/#23 suggests) actually make it sensible. Three cheers for #26!

Crell’s picture

A _help callable on the route definition is a no-go. We have put a great deal of time into ripping apart the muddled dumping ground that was hook_menu. There's now initiatives to try and avoid $request->attributes becoming a dumping ground (although I disagree with the degree to which that's going). We must not turn the route definition into yet another dumping ground. It already has a bit more in it than I'm comfortable with.

TBH I've considered hook_help as vestigial and useless for at least 3 core versions. Tour module, advanced help, and similar are far better approaches.

effulgentsia’s picture

I'm confused by #28. Routes already have a _title and _title_callback, because we recognize that per-route titles are a thing. Perhaps _help is a poor name, but if we made it _description and _description_callback, would that then not be legit for the same reason title is? Except in practice, all our descriptions are HTML, but we want t() to only run on the text portion, so we'd have many more _description_callback usages than _description. Also, I guess our hook_help() content is different than what's appropriate for <meta name="description">, since I think the latter needs to be plain text. Hm, is there an HTML 5 concept for a page description that's HTML?

jhodgdon’s picture

hook_help is not the same as a meta description. The idea of hook_help (for individual pages) is that if you have the Help module enabled, you will get some extra hints at the top of the page on what the page does and how to use it.

Maybe the best thing to do actually would be to make a way for a page controller to put the help text into the returned render array for the page, like in a #help array component at the top level? That makes way more sense, IMO, than having a separate function or something in the route YML file or returning all the help texts for a whole module in a hook.

effulgentsia’s picture

Maybe the best thing to do actually would be to make a way for a page controller to put the help text into the returned render array for the page, like in a #help array

The downside with this is it means the SystemHelp block would need to invoke the controller method to get back its render array, or else access a global variable that would compromise ESI. However, what if we defined a HelpProviderInterface with a getHelpHtml() method (we can improve those names if desired), and controller classes and forms would need to implement that interface if it wanted to offer help text?

Crell’s picture

#30/#31 makes about 10x more sense than a _help callable, and about 5x more sense than hook_help does today. It would also allow for alternate display of the help information than just "div at the top of the page". We want to convert HtmlFragment into an interface anyway (there's an issue somewhere), so an additional interface would fit in with that fine.

(Although I'd still favor leveraging tour more, as that may obviate the need for most hook_help instances in the first place.)

lostkangaroo’s picture

I would be hesitant to do anything that would stop a module from being able to notify an admin about what a module offers. Most ideas I see would still require a module to be installed/active state before anything is offered to accomplish this purpose. In this manner I am in favor of removing the hook_help all together for something that is always available to describe a module regardless of a module being installed or not. Possibly even a help description yml file indexed by routes with callbacks to functions returning text that the help module can access as part of a module's file tree.

Until this can be solved though we should continue with this issue and change path cases to routes, discussing alternatives in separate issues.

jhodgdon’s picture

Issue summary:View changes

RE #33, this seems only to apply to the "hook_help as overall module overview" help. This issue is about the "hook_help as top-of-page hints" help.

I agree that #31 is a good approach for the "hook_help as top of page hints" help.

For the "module overview" help, maybe we can retain hook_help and not have any arguments, for now, and then maybe file a separate issue about doing something about that?

lostkangaroo’s picture

#34 Thats a valid assumption but the main overview for a module would be included as one of the many callbacks indexed in the yml file. Possibly by indexing it in the yml as help.main so that Help.module could identify it. Help.module should also be able to determine if a module is enabled and therefore would be prevented from loading any additional callbacks for pages that a module would provide.

All callbacks could even be grouped together in a module.help.php file that only includes static functions that return the text for the routes named something like module_help_main for the main text and module_help_ for additional pages.

The key though is that any text returned would be indexed by a route that help.module could feed to a page if a module is enabled and still provide a basic overview text if a module is disabled.

tim.plunkett’s picture

@lostkangaroo

Modules cannot be enabled or disabled, only installed or uninstalled.
hook_help has never been able to introspect uninstalled modules
Please open a dedicated issue, this is about decoupling them from paths, not making every improvement available to us at once.

jhodgdon’s picture

Issue summary:View changes

I have added, hopefully, all of the proposals to the issue summary. Please correct and/or choose your favorite.

sun’s picture

what if we defined a HelpProviderInterface with a getHelpHtml() method (we can improve those names if desired), and controller classes and forms would need to implement that interface if it wanted to offer help text?

HelpProviderInterface is not available and not loadable unless Help module is installed. A solution that hard-codes the existence + architecture of the very dated Help module across all of core doesn't fly for me, because:

Page/controller/route-level help text is a UI/UX concept that doesn't make sense anymore today. Users should be informed before navigating somewhere. More complex controls and sections of user interfaces have to be explained contextually, not with a "read more" link in a page-level help text (which on its own explains nothing at all).

In essence, I agree with @Crell:

It makes more sense to ditch hook_help() altogether in favor of a Tour-alike contextual help. The only caveat is that (1) tours serve a very discrete and different purpose/use-case that is conceptually NOT to be mistaken with help, and (2) the DX of authoring tours is even worse than the DX of hook_help() (which is an achievement of its own).

jhodgdon’s picture

Issue summary:View changes

Added proposal 6 to the issue summary (ditch the page-level help entirely).

tim.plunkett’s picture

Can everything except proposal 2 be moved to a dedicated issue?
I don't understand why any of this is still being discussed.

effulgentsia’s picture

Re #40: per https://gist.github.com/webchickenator/4409685, hook_help() is the 7th most commonly implemented hook by contrib. I'd like to avoid breaking it twice if we don't have to. Are there other issues blocked on this that require proposal 2 to be committed separately from whatever other proposal people end up liking? If not, can we give it a few days to see if one of the other proposals emerges as a winner and then do that one in this issue?

jhodgdon’s picture

I don't think this issue is blocking anything, and I agree with #41 about only doing this once.

I'm kind of in favor of proposal 6, with proposal 2 being my second choice.

Crell’s picture

6 is good for me. 2 is acceptable. The question is then whether the "module level help" belongs in a hook or if we should just move that paragraph to the info.yaml file. (No sense parsing it all the time if it's just a string.)

tim.plunkett’s picture

Issue tags:-Approved API change

We're actively working to replace hardcoded paths with route names.
That's why this issue exists.

A larger "let's completely reconsider how to fix hook_help in more drastic ways" is a separate issue.

If we don't choose proposal #2, this is *not* an approved API change.
Just because an issue has that tag for a proposed approach doesn't mean you can just hijack it and make completely unrelated changes and sneak it in as "approved".

jhodgdon’s picture

Several modules have very extensive module-level help, which has sections, HTML tags, etc. YML file would not work well.

tim.plunkett’s picture

Issue summary:View changes
Status:Needs work» Needs review
Issue tags:+Approved API change
StatusFileSize
new122.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,667 pass(es), 491 fail(s), and 160 exception(s).
[ View ]

Tried to reroll, needed to rewrite from scratch.
I moved the other discussion to #2245813: Improve hook_help().

This patch incorporates the change made in #2245783: Regression: Help blocks display on 403/404 page, since it touches the same lines.

Adding back the approved tag, since this is the approach that was originally approved by catch.

Also, there is no need for 'help.main' in every implementation, only hook_help() should be using that. Otherwise, all of the module specific help shows up above the jump links, and that's not what's supposed to happen.

This was *NOT* fun to reroll, let's please get it in quickly.

Status:Needs review» Needs work

The last submitted patch, 46: help-2183113-46.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new123.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,124 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
new2.4 KB

Nice try, search module.

Status:Needs review» Needs work

The last submitted patch, 48: help-2183113-48.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new125.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,128 pass(es).
[ View ]
new2.04 KB

Missed a part in system.module

jhodgdon’s picture

Status:Needs review» Needs work
Issue tags:+Needs tests, +Needs manual testing

Wow. Thanks Tim.

So... I think the docs for hook_help() need to have a piece explaining how to provide the module overview page (which is detecting the special "route name" of help.page.(short name of the module). This needs to go into the help.api.php file. [setting to "needs work" for this change]

And... Since the help text on the admin pages is not (as far as I know) generally covered by our automated tests, I guess we will need to either (a) add automated tests for each page or (b) manually test that each page has the help that it should. Actually I'm sure the help text on many admin pages is not covered by automated tests, because quite a number of the help texts were not working before this patch (and should hopefully be working now?).

I'm not sure which approach is best, so added both tags for the moment. Probably an automated test would be best? Maybe centralized in the Help module? Really not sure. Maybe we don't need these tests, but if we had had them, I think we would have fixed the problems with wildcard paths in help long ago...

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new124.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,669 pass(es).
[ View ]

Ok, sorry for derailing this issue. Thanks for opening #2245813: Improve hook_help(). Alpha11 just came out yesterday, so if we get this one in quickly, we then have 2-3 weeks in which to do #2245813: Improve hook_help() without breaking the same API twice for contrib authors who chase alphas rather than HEAD.

Here's a reroll of #50. CNR for bot.

jhodgdon’s picture

Status:Needs review» Needs work

Still needs work for #51.

tim.plunkett’s picture

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs tests
StatusFileSize
new651 bytes
new124.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch help-2183113-54.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

If we want to invent a new system of automated testing, that is follow-up fodder.
While writing this patch I manually tested it, which actually flushed out a few bugs. But a spot check by someone else couldn't hurt.

Here is a more clear docblock for $route_name.

tim.plunkett’s picture

Status:Reviewed & tested by the community» Needs review

Oops :)

jhodgdon’s picture

I am going to give this patch a full review and manual test after lunch today.

Status:Needs review» Needs work

The last submitted patch, 54: help-2183113-54.patch, failed testing.

jhodgdon’s picture

I turned on every single Drupal Core module, and I have carefully reviewed and tested pretty much every line of this patch to make sure the help text appears on the pages it should be appearing on. Phew!

A few things I didn't test:
- There was one page in the Book module (path /book) that didn't work (I filed an issue), so I couldn't verify the help on it.
- I didn't test the message being set in node_help() about permissions needing rebuild. Why is that in hook_help() anyway???

I also wrote up a draft change record for the issue.

And don't think I didn't notice about the search "no results" help. We need to do something better about that, but moving it out of hook_help() is a good start.

Anyway, I think that the patch is 99.9% good. The code changes look good, and nearly everything works fine. Glitches:

a) language.module

-    case 'admin/structure/block/manage/%':
-    case 'admin/structure/block/add/%/%':
-      if ($arg[4] == 'language' && $arg[5] == 'language_interface') {
+    case 'block.admin_edit':
+    case 'block.admin_add':
+      if ($request->attributes->get('plugin_id') == 'language:language_interface') {
         return '<p>' . t('With multiple languages enabled, registered users can select their preferred language and authors can assign a specific language to content.') . '</p>';
       }
       break;

I cannot find this help being displayed. Looking at locale_help() from D7, I think this is supposed to be displayed when someone is configuring the Language Switcher block. But it isn't being displayed. One confounding factor is that from the Block Layouts page, if you configure a block, you get a pop-up (and I'm not sure the Help block would be included?), but even if you open the URL in its own page, you don't see this help.

So... I think I should see this help on:
admin/structure/block/add/language_block%3Alanguage_interface/bartik
or after I've added this block, on:
admin/structure/block/manage/languageswitcher
but I don't get it on either page.

[Note: The code in system.module that does this for the Powered by Drupal block does work, so that might help?]

b) node.module - I am not getting help on content type Form display or Manage display pages, such as
admin/structure/types/manage/article/display
admin/structure/types/manage/article/form-display

-    case 'admin/structure/types/manage/%/form-display':
-      $type = entity_load('node_type', $arg[4]);
+    case 'field_ui.form_display_overview_form_mode_node':
+      $type = $request->attributes->get('node_type');
...
-    case 'admin/structure/types/manage/%/display':
-      $type =  entity_load('node_type', $arg[4]);
+    case 'field_ui.display_overview_view_mode_node':
+      $type = $request->attributes->get('node_type');

[Note: The corresponding pages in User module help are working, such as admin/config/people/accounts/display]

c) node.module - also not getting help on a node revisions page like
node/1/revisions

-    case 'node/%/revisions':
+    case 'node.revision.overview':

d) node.module - not getting the content type description on the add/edit node pages such as
node/add/page
node/1/edit

-    case 'node/%/edit':
-      $node = node_load($arg[1]);
-      $type = node_type_load($node->bundle());
+    case 'node.page_edit':
+      $node = $request->attributes->get('node');
+      $type = $node->getType();
...
       return (!empty($type->help) ? Xss::filterAdmin($type->help) : '');
-  }

-  if ($arg[0] == 'node' && $arg[1] == 'add' && $arg[2]) {
-    $type = node_type_load($arg[2]);
-    return (!empty($type->help) ? Xss::filterAdmin($type->help) : '');
+    case 'node.add':
+      $type = $request->attributes->get('node_type');
+      return (!empty($type->help) ? Xss::filterAdmin($type->help) : '');

e) system.module - no help on admin/modules

-    case 'admin/modules':
+
+    case 'system.modules':

Phew!!! Pretty close...

jhodgdon’s picture

StatusFileSize
new124.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch help-2183113-54-reroll.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's a quick reroll of the patch in #54 so it will apply (conflict in update.module), but it still is NW for #58.

effulgentsia’s picture

Assigned:Unassigned» effulgentsia
Status:Needs work» Needs review
StatusFileSize
new124.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,430 pass(es).
[ View ]

Reroll. CNR for bot. Looking into #58.

effulgentsia’s picture

Assigned:effulgentsia» Unassigned
Issue tags:-Needs manual testing
StatusFileSize
new125.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,389 pass(es).
[ View ]
new3.45 KB

I turned on every single Drupal Core module, and I have carefully reviewed and tested pretty much every line of this patch to make sure the help text appears on the pages it should be appearing on.

Great! Removing the tag then.

This fixes #58's a, b, c, and e.

For #58.d, that's a pre-existing condition in HEAD: core/profiles/standard/config/install/node.type.*.yml has an empty string value for the help key.

jhodgdon’s picture

For #58-d, shouldn't this patch then remove the code (which doesn't work) that is trying to add this help text to those pages, and (preferably) also add it to the YML file, where it would presumably work?

For the others... I think we probably need to go back and manually test for #58, right?

And honestly, couldn't we have automated tests added to this patch that would turn on the Help module, visit the pages I visited manually, and just do an assertText to verify the help text appears on the page? I still do not know why the "needs tests" tag was removed above, but I do not wish to start a flame war by adding back tags that people removed... so leaving tags as they are. But I think we should either have a "needs tests" or "needs manual testing", and an automated test would be better.

tim.plunkett’s picture

The need for tests for hook_help predates this issue. Opened #2259311: Add automated tests for hook_help() to handle that, it is not related to the path/route_name switch.

jhodgdon’s picture

Fair enough, but can I just say that testing this was *tedious*?

effulgentsia’s picture

Issue tags:+Needs manual testing

For #58-d, shouldn't this patch then remove the code (which doesn't work) that is trying to add this help text to those pages, and (preferably) also add it to the YML file, where it would presumably work?

The code does work and is needed. It returns a sanitized version of whatever is in the $type->help variable, which is populated in the "Explanation or submission guidelines" textarea when editing a node type. From what I could tell in searching earlier Drupal versions, we've never shipped with default help text for node types installed by core. #2084611: Provide default instructions on node/add/* forms is an issue to do so.

For the others... I think we probably need to go back and manually test for #58, right?

True. Restoring tag. I manually tested as part of writing the #61 interdiff, but someone needs to verify that prior to this being RTBC'd.

jhodgdon’s picture

Status:Needs review» Needs work
Issue tags:-Needs manual testing

I retested the glitches noted in #58 with the patch in #61.

a) language switcher block - I am seeing the help text if I already have the language switcher block configured, and I edit/configure again, but not when I first add the language switcher block.

... Oh, that's a problem with the pop-up interface for block add. It doesn't display the Help. When I go to
http://wren/~d8git/admin/structure/block/add/language_block%3Alanguage_i...
in a new window/tab, I get the help. But if I'm on admin/structure/block and I click "language switcher" to add this block, I get a pop-up window and it doesn't display the help. So that is not a function of this issue.

Filed: [edit: added link to issue]
#2260095: Help not displayed within modals (like when adding a new block to layout)

So, (a) is OK.

b) node type form/display modes - works.

c) node revisions - works.

d) To test this, I first went to the Article content type, and added a "submission guidelines" text to it. Then I went to node/add/article. Explanation was shown. Works! Thanks for the explanation in #65.

e) Extend page - works

I also verified that the interdiff only touches those 5 places in the code. I had previously reviewed both the code and functionality of the rest of the patch. So ... I think this is ready to go in!

Uh oh... So I was busy filing an issue about (a) and I noticed that the help for the "Powered by Drupal" block does not show up when you add a new one, but only when you edit. Can we add code similar to the Language Switcher block fix that you put in this latest patch, to system.module for the Powered by Drupal block as well?

It looks like Powered by Drupal and Language Switcher are the only two Core blocks with help, by the way.

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new125.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,490 pass(es).
[ View ]
new967 bytes

Can we add code similar to the Language Switcher block fix that you put in this latest patch, to system.module for the Powered by Drupal block as well?

Sure.

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community

Excellent! IMO, this patch is great. I had previously reviewed the code and tested the functionality in #58. The few problems I found in #58 and #66 have all been addressed. It works fine.

tim.plunkett’s picture

+1 for RTBC, thanks.

The last submitted patch, 59: help-2183113-54-reroll.patch, failed testing.

jhodgdon’s picture

#70 is complaining about a retest of an old patch. The patch in #67 passed.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Well the diff on the doxygen of hook_help() looks immeasurably more sane. :) I was going to recommend also adding an example there of a dynamic hook_help() entry, but in grepping the patch for Request-> it seems like there's no real standard anywhere. It's all just PHP.

Going to get this in while it's hot, since it seems like the kind of thing that'll break all over the place otherwise. Change record looks good.

Committed and pushed to 8.x. Thanks!

  • Commit eef067b on 8.x by webchick:
    Issue #2183113 by jhodgdon, effulgentsia, tim.plunkett, herom, larowlan...
jhodgdon’s picture

Woot! I just published the change record too.

jhodgdon’s picture

I also just updated the hook_help() guidelines page: https://drupal.org/node/632280

YesCT’s picture

https://api.drupal.org/api/drupal/core%21modules%21help%21help.api.php/f...

doesn't have the new parameters.
Are the api pages cached .. or?

YesCT’s picture

+++ b/core/modules/action/action.module
@@ -5,12 +5,14 @@

+use Symfony\Component\HttpFoundation\Request;
+
/**
  * Implements hook_help().
  */
-function action_help($path, $arg) {
-  switch ($path) {
-    case 'admin/help#action':
+function action_help($route_name, Request $request) {
+  switch ($route_name) {
+    case 'help.page.action':
       $output = '';

actual change needed here includes adding the use for Request.

Updating the change record to include that.

jhodgdon’s picture

Thanks for the alert in #76! Turns out that the updates on api.drupal.org have been hung for 7 days. We're on the case...

And thanks for fixing the change record too!

Status:Fixed» Closed (fixed)

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