Make admin/help page more flexible, and list tours on it

And by the way, I'm working on a patch now...

Here's an initial patch, with tests, docs, etc.

Also here are screen shots of the new/old help pages (they are kind of large images so just linking not embedding in this comment, hope that is OK). Differences to note:
- Slight change in wording for "For more information" sentence in Getting Started.
- "Help topics" section on old becomes "Module overviews" (and description text line also changes accordingly). Pay no attention to the differences in the list of topics here though... The screen shots came from two different sites, and I had more modules enabled on the "New" site than on "Old". Sorry about that.
- New page also lists tours

Screen shots -- new:

Old:

Fixing test failures. I had to add the template to Stable, fix the NoHelp test (it was looking for the old test on the admin/help page) and the help.module file (it was skipping modules that didn't provide a main help topic, which violated the HelpTest test).

Note that the interdiff file doesn't include the added template in Stable -- it only has the other two changes. Hope that is OK.

Tests pass locally now; should be OK to review.

Dang, sorry, uploaded wrong patch file there, without my updates from the interdiff (forgot to git add -u them, oops).

Tests pass this time. ;)

Thanks for the review!

In regards to item 6, I did document in there I think that the problem is that some routes require parameters. So the try/catch is to find a route that doesn't. For instance, you might have a tour on the foo/add and foo/{foo}/edit pages. The edit page would exception out (it has a parameter) and the add page would work without parameters. Anyway I'll fix the comments so this is clearer.

Good idea on the plugin vs. hook. I'll work that up...

OK! I have implemented a plugin system instead of hook for the help page sections. I think all the review comments in #10 have been addressed (most were related to that). One note: what I decided to do for the Tours section permissions check was go ahead and list them but not make links if the user doesn't have permission to visit that page. This functionality is also tested in TourHelpPageTest. Note that all modules are shown in the Module Overview section too, even if the user doesn't have admin permissions on that module, so it is kind of parallel to list the tours but I agree that providing 403 links to the admin pages with the tours on them is probably not useful.

The screen shots are exactly the same as in #3. Only the underlying functionality has changed (extensively). Tests pass locally; let's see what the bot says.

I've included an interdiff file, although since it's nearly as large as the patch (almost everything changed), I am pretty sure it is not very useful. I recommend looking at the patch file instead, but whatever. ;)

Tests passed; let round 2 of reviews begin! (thanks in advance!!)

Thanks for the review in #14! Your points:

1. I wouldn't want the default to be 'access administration pages'. The default would want to be '', because the admin/help page already only is visible for 'access administration pages', so we don't need that permission. Anyway, I made that default and updated the docs.

2. We do not actually send the output of the page into an alter hook, beyond the generic theme/render hooks that all render arrays get passed to. hook_help_section_info_alter() is for the plugin definition arrays (which basically means it gets the information in the @HelpSection annotations).

3. Updated docs. This reminded me that the tours section should probably be given the user context as it is checking permissions, so added that too.

4. Hah. Updated docs. There's an edge case possible where a module adds hook_help() when it didn't have it before, but developers doing that would know they need to clear cache if so, and if it's an update they downloaded they should run update.php which also clears the cache. So. Took out 'hopefully'. :)

5. Hm. Deferring that (if necessary) for now. I see what you're saying, don't see a compelling reason to do it for the moment. I would not expect this plugin class to be instantiated except when building the admin/help page, and I don't see a compelling reason to do it one way vs. the other, since both pieces (cache and topics) will be called for, one after the other, in any case.

6. I didn't make this change. The reasoning I used:
- The static Link and Url calls return you objects that are lazy-evaluated at render time, unlike the URL and Link generator classes. In the HookHelpSection class, that is desirable, I think, to leave the links as late as possible to render.
- In the Tour class, the link is rendered immediately to verify there is not an exception, so we could use the link generator service. However, the URL object is also used -- we call the ->access() method on it. It would mean injecting at least one and maybe several other services to replace everything the URL object is doing, and I think the code would be much less clear, because the object returned by the URL service is not something you can pass into the Link generator (which is expecting a regular Url object, not what the URL service returns)... so I'm inclined to leave it as it is.
So the bottom line is that the code in these plugins is quite a bit clearer and cleaner using Url and Link, and I'm inclined to leave it as it is. Thoughts?

7. Typo, fixed.

Anyway, here's a new patch and interdiff. Also updated issue summary to reflect plugin vs. hook.

Thanks for the review in #16! Regarding your points...

1. The modifications here are fairly straightforward (making the page flexible and listing tours if available); if we get crazy with graphics and stuff (on another issue presumably) then we can think about splitting it up, but I don't see a point to having a Help module with a separate UI module at this point.

2. Updated to make it clearer this is a URL but I think saying "help index page" is confusing, as it is not called that anywhere else. The wording in this section is now parallel to the existing docs in hook_help() above in that same file.

3. I specifically used "hook_help" for this plugin because it is listing the pages provided by hook_help(). I'd prefer to leave it with this ID.

4. The permission annotation is documented on the Annotation class included in the patch.

5. There is no way to handle routes with parameters. I've made another try at updating the docs to make this clearer.

6. There are not many tours that exist currently in core. If you turn on the language/locale modules, you will have a few more showing. See #1921152: META: Start providing tour tips for other core modules., which hasn't been updated in a while.

Anyway, here's a patch with some updated docs...

Thanks for the review in #20, and what a pain to lose your comment! Ugh. Anyway, I hope the important points are all there.

Regarding your point #2, the fact that it is a 4-column list is hard-coded in the way the method works (it divides up the list into 4 chunks) and in the class added to the render array (layout-column--quarter); that said, it does degrade nicely to a 1 or 2 column list on a phone or other smaller device. This is NOT a change from the existing help page; I only separated out the code that made the 4 columns into a separate method (I needed to call it on each section of the page rather than just once). And I changed the code slightly -- used array_slice() instead of the slightly more convoluted process used in the previous code for dividing the array into 4 parts.

Anyway... since this is how the existing page works, I think fixing that would be a separate issue. There were also two previous issues dealing with these points, which is how it got from the even worse code it used to be to where it is now:
#2408481: Rewrite help.module component's inline with our CSS standards
#2337317: Replace help page layout CSS with reuseable layout classes

By the way, regarding the search feature for help mentioned in the review in #16, there is already an issue related to this at #102254: Add an administrative search feature.

The attached patch/interdiff I think fixes the other points in #20. Much better to use that interface, excellent idea! I decided that it merited making a base class now (as was suggested a few reviews ago by larowlan), since I think most implementers will have now 2-3 methods they will want to leave empty.

whoops, uploaded the patch twice instead of the interdiff. :(

Dang, can't do anything right today. I also forgot to add the new plugin base class into that patch. It's also not in the interdiff file.

One more patch - cleaning up some extraneous use statements, at least I hope they are extraneous. The tests are passing, and I think/hope I've addressed most/all of the review comments.

So... I really appreciate all of the reviews so far... Anyone have more suggestions for things that should be improved in this patch? I would like to get this patch finalized soon if possible so that I can work on #2351991: Add a config entity for a configurable, topic-based help system and hopefully make the 8.1 deadline at the end of the month (that issue is postponed on this one).

Thanks for the review in #26 @andypost!

1. All plugin managers have an "alter" hook to let modules alter the list of plugins, so I put one in here. It is possible that not all of the alter hooks we have are documented, but all/most plugin managers have that alter hook invoke in them... This is pretty much a standard plugin manager construct method (in HelpSectionManager class):

+  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler) {
+    parent::__construct('Plugin/HelpSection', $namespaces, $module_handler, 'Drupal\help\HelpSectionPluginInterface', 'Drupal\help\Annotation\HelpSection');
+
+    $this->alterInfo('help_section_info');
+    $this->setCacheBackend($cache_backend, 'help_section_plugins');
+  }

So this alter is being invoked once when you go to the admin/help page, when someone asks for the HelpSectionManager to list all the help page section plugins. It will only actually get invoked if the help section plugins list cache data has been invalidated or is missing. It is not affecting the help block, so it is not happening on every page, just when you go to admin/help, if someone has installed/uninstalled a module or cleared the cache or something.

2. The page is being cached based on user permissions already. So it seems to be a waste of time to calculate something that will never be displayed? I think it's better just to skip it if the user cannot see it anyway. (You brought up several performance issues -- why add to the problem?)

3. Fixed. Thanks!

4. Good idea. Fixed. Thanks! [Note that the interdiff doesn't show that the HelpSectionBase class file name changed, due to me not being great at creating interdiff files, but it is changed in the patch.]

5. Hm. That HookHelpSection plugin extends PluginBase, which uses StringTranslationTrait. So it seemed like a good idea to inject the string translation service, in case any methods on PluginBase used t()? Not sure. Is this unnecessary?

6. (implied) I'll get a change record drafted shortly and update the issue.

So... somewhat flawed interdiff file and new patch attached. I haven't tested the patch but it should be OK hopefully.

Change notice drafted:
https://www.drupal.org/node/2669966

Regarding #29, they can already omit the permission key. In fact, if you look at HookHelpSection, it does that:

+ * @HelpSection(
+ *   id = "hook_help",
+ *   header = @Translation("Module overviews"),
+ *   description = @Translation("Module overviews are provided by modules. Overviews available for your installed modules:"),
+ * )

which, according to the docs in the annotation, means:

+  /**
+   * The (optional) permission needed to view the help section.
+   *
+   * Only set if this section needs its own permission, beyond the generic
+   * 'access administration pages' permission needed to see the admin/help page
+   * itself.
+   *
+   * @var string
+   */
+  public $permission = '';

To clarify, the admin/help page is already checked for the 'access administration pages' permission due to the help.routing.yml entry:

help.main:
  path: '/admin/help'
  defaults:
    _controller: '\Drupal\help\Controller\HelpController::helpMain'
    _title: 'Help'
  requirements:
    _permission: 'access administration pages'

So if a HelpSection plugin has no permission beyond that, it just leaves 'permission' out of the annotation, and no further permission check is performed. There is no reason to default the permission in the annotation to 'access administration pages', and check that same permission again in the Controller when it decides whether to build each section. Right?

Actually, I don't agree about the tours and plain text vs. links. I put that in because I really do think it's useful to know that a tour exists for the "editing languages" or "editing a view" pages, even though you cannot go there directly with a link. The description text for the section explains this with:

The tours with links in this list are on user interface landing pages; the tours without links will show on individual pages (such as when editing a View using the Views UI module).

Eventually it would be nice if #1921152: META: Start providing tour tips for other core modules. got done so we had a sensible list of tours on the page in the first place, so that the section wouldn't be so empty...

And as a note, the Help controller does have some code that outputs some text if the list of topics is empty, but again I think having the section there is useful.

Thoughts?

RE #31/#32, we should let the Tour module maintainer decide this.

and/or Usability team.

The permission in the plugin annotation is the permission to display the entire section. Any module that provides a section plugin can define a permission like "View xyz type of help" and then that can be given to a role. A user without that permission won't see the section at all. For instance, if you don't have the tour viewing permission, you won't see the Tours section, because the Tour plugin has that permission in its annotation.

Then each individual plugin may also check permission or do whatever it wants when making the list of topic links (so it could hide certain links for certain permissions etc.). It can also put whatever it wants into the cache metadata for the section, using the methods on the plugin. For instance, the Tour module adds the entity list cache tag for Tour config entities for its section. The controller adds the user.permission cache tag to the page as a whole, so that if a user with different permissions views the page, it will be rebuilt for them.

And then presumably the standard Drupal route permission system will be used when someone clicks on a link to view a topic or see a tour or whatever. So for instance if the Tours section lists a tour for adding a language, and someone who doesn't have administer languages permission clicks on the link, they get a 403.

On the other hand, the core hook_help() pages don't do much about permissions -- this system was in place prior to this patch. You just need the standard "access administration pages" permission to view admin/help or any of the hook_help() module pages like admin/help/node. This is true whether or not you have any admin permissions for that particular module; for instance, you can read about how to add content on the admin/help/node page, even if you have no add content permissions. But that has been true forever in Core. This patch doesn't change it in any way.

So... I think the system is sufficiently flexible for whatever use case, and the tests in this patch do test the permissions stuff...

Thanks Wim! First, here's a reroll to merge with the changes from the other issue. And regarding creating tours, see that meta issue linked in #32.

I'll address/fix the rest in a separate patch because I have enough problems with interdiffs as it is without combining them with rerolls. ;)

So, regarding the review in #37 -

1. Fixed in two places in this file, plus a few other files. Some rewrapping required.

2. I do agree with you on this... I talked to joelpittet about this, and got it working. There's a new template for help sections for Classy (to override the default one in modules/help and Stable theme), and now HelpController does not output markup itself. Woot! Definitely an improvement.

3. Excellent, thanks for pointing that out!

4. I don't see any comments going over 80-character lines in this patch... ???

5. Fixed.

6. Got rid of the local variable entirely -- combined it with the next line of code.

7. Good idea, fixed.

8. Sure, also changed $entity to $tour.

9. Hm. So. In TourHelpSection, getCacheContexts() does this:

  public function getCacheContexts() {
    // The links are checked for user access, so we need the user
    // permissions context.
    return ['user.permissions'];
  }

I thought that this would take care of making sure that the page cache is not shared for people with different permissions to access routes?

If not, do you have a suggestion for what I need to do instead? Thanks!

Anyway. Here's a new patch and an interdiff. Tested manually and works fine; let's see if the bot agrees.

Note: The interdiff does not include the new core/themes/classy/templates/misc/help-section.html.twig template file, because I am lousy at creating interdiffs that contain both new files and changes to existing files. Sorry about that!

Whoops. Forgot that. Thanks!

Doh. Realized that in Stable/core template, since we are not outputting an item-list of links any more, I needed to add something to the template. Verified manually that the page works in Stark theme as well. It just makes a simple UL list of the topics.

Hm. I see what you are saying about the cacheability -- thanks for taking the time to explain it.

So, let's see. The TourHelpSection plugin is trying to make a link to any of the possibly several routes (usually admin pages) that the tour could be displayed on (for instance, a tour could apply to both the Add and Edit page for some config entity, which means it has 2 routes it applies to).

So TourHelpSection does a loop over the routes, where it checks if the route:

a) Can be created (without any special route parameters).
b) Can be accessed by the current user.

If the first route works, it makes that link. If it doesn't, it keeps trying the rest of the routes where that tour can be displayed, until it finds one and makes that link. If none of them works, it just displays the tour title as not a link. So this isn't quite like breadcrumbs, because they have definite URLs at least.

Yeah, I'm inclined to say that the TourHelpSection's list of links should not be cached.

Here's one more patch... I was going to fix the 80-char thing in HelpTest but with the comma, it hits 80 in my editor so I didn't.

Thanks Tim for the review in #47 -- all good points.

Someone brought up the Translation thing before... I wasn't sure because PluginBase itself uses StringTranslationTrait, which kind of implies it expects the translation service to be included. But if you say it's OK not to do that, ... yeah I would not see anything PluignBase would want to translate. Good! Took that out.

And you are right, we should leave it up to the plugin whether they need to be container-aware or not. I think most would, but you never know, maybe Advanced Help will use this and do some kind of file system listing. :)

So here's yet one more patch... tested manually, still works... Hopefully this is converging to something Good For Drupal 8 (TM).

I just want to say again, I REALLY appreciate all the excellent reviews from everyone! Definitely improving the patch, little by little. ;)

I just made a minor update to the change record for this issue. Also updating the issue summary to add a few points.

Sorry, wrong related issue there.

I like the approach in the latest patches.

So... Regarding #52 - item 5 -- there is a simple template that is in the Stable theme (as well as a core template), which has no classes. Templates with classes belong in Classy, so that is where the 4-column theme template goes. I think Cottser/joelpittet would agree on this being the right thing to do, or at least that is what I thought Joel told me to do in IRC yesterday. Anyway, the help page *does* work in Stark, in the sense that the topics are listed on the page with no stying decisions (such as dividing the page up into exactly 4 columns, which is Classy's choice and should not be part of Stark).

Anyway, a few nitpicks on the latest patch, and 1 substantive comment:

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -801,15 +802,21 @@ public function getInternalPath() {
   +   *   The access result. Returns a boolean if $return_as_object is FALSE (this
   +   *   is the default) and otherwise an AccessResultInterface object.
   +   *   When a boolean is returned, the result of AccessInterface::isAllowed() is
   +   *   returned, i.e. TRUE means access is explicitly allowed, FALSE means
   +   *   access is either explicitly forbidden or "no opinion".
   

   In text, "boolean" should be "Boolean". It is a proper noun.

   Also, as you pointed out on one of my earlier patches, in the 3rd line the word "When" can be moved up to the previous line. ;) Funny how we can see this in someone else's patch...

   Also, in the 4th line, please do not use "i.e.". It is not punctuated properly and most people (as demonstrated by many many many examples of bad docs that we have laboriously fixed in Core) do not understand what it means. Really, we had some docs cleanup issues that got rid of all of the i.e. and e.g. in Core docs... it was painful to see how bad they were. So, please use "that is". And the punctuation should be:

   ... is returned; that is, TRUE means...

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -818,11 +825,11 @@ public function access(AccountInterface $account = NULL) {
   -   * @return bool
   -   *   Returns TRUE if the current user has access to the url, otherwise FALSE.
   +   * @return \Drupal\Core\Access\AccessResultInterface
   +   *   The access result.
       */
      public static function renderAccess(array $element) {
   

   I agree with dawehner here -- this definitely smells like a BC break.

   This is a public static function. Changing its return value cannot be OK. You cannot guarantee me that no one can call this outside the context where you say it doesn't break anything. If we need a function that returns an object then we should define a new function, not break BC on the old one, right?

3. +++ b/core/modules/help/src/Controller/HelpController.php
   @@ -40,62 +51,57 @@ public function __construct(RouteMatchInterface $route_match) {
   +      $links = $plugin->listTopics($cacheability);
   

   I do think this is a good addition to the API. Thanks Wim! Although it is a bit weird to have two separate places to get cacheability information, one for the plugin itself and one for the listTopics() method. Is it not possible we could combine them?

4. +++ b/core/modules/help/src/HelpSectionPluginInterface.php
   @@ -0,0 +1,40 @@
   +   * @param \Drupal\Core\Cache\CacheableMetadata &$cacheability
   

   We do not normally put & in @param doc lines. Please take that out.

5. +++ b/core/modules/help/src/HelpSectionPluginInterface.php
   @@ -0,0 +1,40 @@
   +   *   Cacheability metadata object, which will be populated based on the
   +   *   accessibility of the list of topics.
   

   I would not use "accessibility" here. Accessibility to me means "can people who are blind or have mobility limitations use the page", not "do I have access permission to see it". At least, it's definitely ambiguous.

   Also, you are assuming that the only reason listTopics() has cacheability metadata is due to access checking, but it's certainly possible for some implementation of this plugin to do something else that requires cacheability, right?

   So.... my suggested wording here would be:

   Cacheability metadata object; add cacheability information about the returned list of topics to this object.

   But agin... I'm not really sure why we need to do the cacheability stuff in two places in this plugin. There's really only 1 use for this plugin -- making a list of topics. Why should the plugin and listTopics() have separate cacheability stuff? Can't we just combine them?

6. +++ b/core/modules/help/src/HelpSectionPluginInterface.php
   @@ -0,0 +1,40 @@
   +  public function listTopics(CacheableMetadata &$cacheability);
   

   Is the & really necessary here? I think objects are always passed by reference. See
   http://php.net/manual/en/language.oop5.references.php

   I'm pretty sure we do not need the & here.

7. +++ b/core/modules/help/templates/help-section.html.twig
   @@ -0,0 +1,25 @@
   + * Default theme implementation for a section of the help page.
   

   Just pointing out that this is the default template implementation (which by necessity is also copied in Stable theme).

   It just outputs a UL list of the links, not divided into 4 columns.

8. +++ b/core/modules/tour/src/Plugin/HelpSection/TourHelpSection.php
   @@ -0,0 +1,140 @@
   +  public function getCacheMaxAge() {
   +    // The calculation of which URL (if any) gets put on which tour depends
   +    // on a route access check. This can have a lot of inputs, including user
   +    // permissions and other factors. Unfortunately, we don't have a way to
   +    // get cachability metadata for each of these factors, so the only way
   +    // to ensure that inapplicable information is not used is to forbid
   +    // caching completely.
   +    return 0;
   +  }
   

   So due to your patch for listTopics() just below, shouldn't we be taking this out?

9. +++ b/core/modules/tour/src/Plugin/HelpSection/TourHelpSection.php
   @@ -0,0 +1,140 @@
   +          $cacheability->addCacheableDependency($url_access);
   +          if (!$url_access->isAllowed()) {
   +            continue;
   +          }
   +
   +          // Generate the link HTML directly, using toString(), to catch
   +          // missing parameter exceptions now instead of at render time.
   +          $topics[$id] = Link::fromTextAndUrl($title, $url)->toString();
   +          // If the line above didn't generate an exception, we have a good
   +          // link that the user can access.
   +          $made_link = TRUE;
   

   Technically, we are adding more cacheable dependencies than we really need here, because some of the routes will need parameters so will be try/catched out below, but that is probably OK.

10. +++ b/core/themes/classy/templates/misc/help-section.html.twig
    @@ -0,0 +1,50 @@
    + * Classy theme implementation for a section of the help page.
    

    And this one is the Classy theme implementation, which is more complex: divides up into 4 columns and adds the column--quarter classes.

11. +++ b/core/themes/stable/templates/admin/help-section.html.twig
    @@ -0,0 +1,25 @@
    + * Default theme implementation for a section of the help page.
    

    And then here's the Stable copy of the Core theme.

And... I am now officially handing this over to Wim because I am NOT making changes to the Url object.

I was quite happy with the "don't cache the list of tours" solution. If we need to make this many changes in order to get proper cacheability information for a page we probably aren't going to cache anyway... why are we doing this?

Maybe we should just go back to the patch in #51, address dawenher's feedback in #54, and actually get this finished by the Beta deadline. I would really like to do that... thoughts? I fear that the current approach is not going to make it in at all (BC break and I don't understand the implications of removing it). I would really really really really not be happy if we miss the deadline over something that is probably not necessary.

OK... In case the approach in #57 is acceptable, here is a patch that:
- Starts with #51 [Wim's fixup of a few "nits" from my last patch in #48].
- Fixes the comment in the Tour plugin for the getMaxAge() method mentioned in #51, which wasn't fixed by Wim because he took a different approach in #52.
- Addresses @dawehner's feedback in #54, by adding getHeader() and getDescription() methods to the plugin interface and base class. Also clarifies the annotation docs for 'header'.
- Well, then I decided #54 was right and the annotation/method should be called 'title' instead of 'header', so I went and changed that.

Note that the interdiff is to #51.

After clearing the cache, the Help page still looks the same... so I expect it will pass the tests.

Thoughts? Further suggestions?

Any thoughts on the latest patch/approach? Anything else left to do?

Two notes:
- RE #61 - I already have the Configurable Help (sandbox currently) contrib module using this patch to add editable help topic pages to admin/help... I was going to try to get that into Core too but this issue was a prereq and there is not time for 8.1.x. Maybe 8.2.x, since webchick (Product Manager) thinks it is a good idea. We'll see.
- RE #62 - only the Available Tours segment of the page is uncacheable currently. So if you don't have the Tour module enabled, admin/help is cacheable. And we can probably add cacheability to the Tours section later if it becomes important.

#65 appears to be exactly the same file as #58. diff finds no differences. ???

Thanks @cottser for #77. Thanks @xjm for #76. And yes, the screen shots are current.

RE tours in Core, see this meta/strategy issue, which is stalled and needs to be restarted:
#1921152: META: Start providing tour tips for other core modules.

RE testing "nothing in section", good idea. I'll take care of it shortly.

Here's a new patch with the requested test for the empty section.

After looking at the screen shot of the verbose output of the test, I decided that the "nothing in the section" sentence should end with . -- it looked really weird without that. So, added that to HelpController, added a bit to HelpTest, and added a test plugin to the existing help_page_test test module.

Reviews welcome. :)

WOOT! Thanks all!!

Thanks @Wim Leers. I was just going to check to see if the change record had been published. :)