Problem/Motivation

On #2920309: Add experimental module for Help Topics, one thing that came up was that we wanted to move the new section for Help Topics on admin/help, so that it appears before the section for Module Overviews (hook_help() output).

Sections on admin/help are created by plugins of type HelpSection (each is an annotated class). Currently, they have no weight, so they are just ordered arbitrarily by the HelpSectionPluginManager.

Proposed resolution

Add a weight property to the HelpSection annotation, so that HelpSection plugins can be ordered on the admin/help page. Proposed order:
- [-10] Help Topics (if #2920309: Add experimental module for Help Topics gets in)
- [0] Module Overviews (hook_help() output)
- [10] Tours

Proposed weights for those sections are in [] in that list, with space left so that contributed modules defining HelpSection plugins can insert their sections wherever they want.

Remaining tasks

1. [done] Make a patch that adds weight functionality and weights.

2. [done] Write a test. Should include a testing module that demonstrates the order can be altered via a hook that alters the plugin data.
https://api.drupal.org/api/drupal/core%21modules%21help%21help.api.php/f...

3. [done] Change the function body in that hook (in the help.api.php file) so that the sample code also alters the weights, as that is the most likely use for the hook. Old function body was actually not valid, so that is also updated.

User interface changes

The order of sections on admin/help will be governed by plugin weight, instead of kind of random.

API changes

There will be a new property for HelpSection plugin annotation (given a default value of 0 for backwards compatibility).

Data model changes

None.

Release notes snippet

Modules can add sections to the admin/help page by defining a HelpSection plugin. See
https://api.drupal.org/api/drupal/core%21modules%21help%21src%21HelpSect...
for more information about HelpSection plugins.

These plugins can now provide a weight property in their annotation, in order to define their position on the admin/help page. Existing core sections are given the following weights:
- Help topics (experimental module): -10
- hook_help() module overviews: 0
- Tours: 10

Example annotation with weight, for a HelpSection plugin:

 * @HelpSection(
 *   id = "help_topics",
 *   title = @Translation("Topics"),
 *   weight = -10,
 *   description = @Translation("Topics can be provided by modules or themes. Top-level help topics on your site:"),
 *   permission = "access administration pages"
 * )
CommentFileSizeAuthor
#18 2994748-weight-18.patch6.26 KBandypost
#18 interdiff.txt3.54 KBandypost
#16 interdiff.txt606 bytesjhodgdon
#16 2994748-16.patch6.92 KBjhodgdon
#14 interdiff.txt1.44 KBjhodgdon
#14 2994748-14.patch6.99 KBjhodgdon
#13 interdiff.txt3.48 KBjhodgdon
#13 2994748-with-tests-13.patch7.13 KBjhodgdon
#6 2994748-5.patch3.05 KBjhodgdon

Comments

jhodgdon created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented

On other hand any list of help section plugins instantiated by block which is rendered in some context ... So block could store in its config list if plug-in ids already ordered
Another option is event to collect/sort filtered plugin definitions https://www.drupal.org/node/2961820

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Assigned: Unassigned » jhodgdon

I'm going to work on this today.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Well, this turned out to be pretty simple. Here's a patch. Do we need a test? I tested it manually by looking at the order of the page (which happened to be Module Overviews, Topics, then Tours), and then applying the patch and clearing the cache, and the new order is Topics, then Overviews, then Tours.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2994748-5.patch3.05 KB

Might help if I actually uploaded the patch. :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Assigned: jhodgdon » Unassigned
Status: Needs review » Active
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Active » Needs review
andypost’s picture

andypost
he/him
Primary language Russian
commented

Nice! Sure it needs test and testing module that will show how using hook plugin alter to reorder sections

Meantime question - are we allow any UI for reordering outofbox?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs tests

I do not think we need a UI, and that was not in the original proposal. If someone wants to alter it on their own web site, and we have an alter hook, that should be good enough. Good idea for the test, adding that to the issue summary, and marking as Needs Work for the test.

And I think we should add that alteration to the function body of
https://api.drupal.org/api/drupal/core%21modules%21help%21help.api.php/f...
as part of the sample code.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Actually I think the function body in that help.api.php file is just wrong. There is no "header" property of those plugins. ?!?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Assigned: Unassigned » jhodgdon

I'm working on the test now... should have a patch later today.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Assigned: jhodgdon » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2994748-with-tests-13.patch7.13 KB
new interdiff.txt3.48 KB

OK, here's a patch that adds two tests (both pass locally), and also updates the api.php file so the alter hook body there actually would work.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue summary: View changes
Issue tags: -Needs tests
StatusFileSize
new 2994748-14.patch6.99 KB
new interdiff.txt1.44 KB

Tests pass on the bot too. Here's a fix for the coding standards problems... I think this is probably ready... Any thoughts @andypost?

Also adding release notes snippet to the issue summary for the change notice.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Quick skim - not sure alter hook should return $info when it passed by reference, the rest looks nice

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
StatusFileSize
new 2994748-16.patch6.92 KB
new interdiff.txt606 bytes

Good point! Fixing that.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I added a draft change record for this issue -- a copy-paste of the Release Notes section of the issue summary:
https://www.drupal.org/node/3068527

Code still needs review.

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt3.54 KB
new 2994748-weight-18.patch6.26 KB

A bit of clean-up, looks default weight always applied

  1. +++ b/core/modules/help/src/Annotation/HelpSection.php
@@ -57,4 +57,13 @@ class HelpSection extends Plugin {
+  public $weight = 0;

+++ b/core/modules/help/src/Plugin/HelpSection/HookHelpSection.php
@@ -13,6 +13,7 @@
+ *   weight = 0,

    I bet default value will be applied by default 

  2. +++ b/core/modules/help/src/Controller/HelpController.php
@@ -87,6 +87,10 @@ public function helpMain() {
+      if (isset($plugin_definition['weight'])) {
+        $this_output['#weight'] = $plugin_definition['weight'];
+      }

    This is only question here, if annotation defines default value (0) then this definition should always contain value.
    Also it could be added to list above like `'#weight' => $plugin_definition['weight']` 

  3. +++ b/core/modules/help/tests/src/Functional/HelpPageOrderTest.php
@@ -0,0 +1,59 @@
+  /**
+   * Modules to enable, so that we have at least two sections on the page.
+   *
+   * @var array
+   */
+  public static $modules = ['help', 'tour'];

    Should be "protected" and could use @inheritdoc 

  4. +++ b/core/modules/help/tests/src/Functional/HelpPageOrderTest.php
@@ -0,0 +1,59 @@
+  /**
+   * The admin user that will be created.
+   */
+  protected $adminUser;

    missing @var

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

interdiff looks good to me, assuming tests still pass, thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

So... I am happy with the changes in #18 and +1 for RTBC. @andypost what are your thoughts?

andypost’s picture

andypost
he/him
Primary language Russian
commented

Last patch is mine, but I'd like rtbc)

jhodgdon’s picture

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

Well, it looks like you like what I did on the patch, and I like what you did, so I think we both agree it should be RTBC. I'll go ahead and mark it RTBC but if anyone else wants to review, that would be good too.

amber himes matz’s picture

amber himes matz
she/her
Primary language English
Location Portland, OR USA
commented

I'm currently reviewing. Is this patch supposed to move the "Module overviews" section to the end? (So sections are ordered Help Topics, Tours, Module Overviews?)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

No, the plan was to make it Help Topics, Module Overviews, Tours. At least for now... Do you think we should change the order?

amber himes matz’s picture

amber himes matz
she/her
Primary language English
Location Portland, OR USA
commented

I think the order is fine as-is. I got thrown off when reading help.api.php (which is just showing documentation for how a developer could alter the weight). And also, just noticed that the tests are looking for module overviews before tours.

This looks good to me. +1 to RTBC.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Great, thanks for the review!

The other one that is a blocker for beta for Help Topics is #3067943: Add capability for search blocks on non-default search... :)

amber himes matz’s picture

amber himes matz
she/her
Primary language English
Location Portland, OR USA
commented

Do you think we should add a weight to the HookHelpSection as well? (As with TourHelpSection?)

namespace Drupal\help\Plugin\HelpSection;
...

/**
 * Provides the module topics list section for the help page.
 *
 * @HelpSection(
 *   id = "hook_help",
 *   title = @Translation("Module overviews"),
 *   weight = 5,
 *   description = @Translation("Module overviews are provided by modules. Overviews available for your installed modules:"),
 * )
 */

I was thinking this might help someone looking at help.api.php's hook_help_section_alter connect $info['hook_help']['weight'] with the weight in the annotation. Perhaps, too, we should add a comment for @HelpSection annotations about hook_help_section_alter?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I had a weight of 0 in there originally, but andypost thought we should take it out and let it default to 0 on an earlier review, so I did that.

As far as documenting the hook... The hook existed prior to this patch, and when it was created, we followed the general practice for this type of hook (altering plugin definitions). So it is linked from where the hook is invoked, which is on the plugin manager class:
https://api.drupal.org/api/drupal/core%21modules%21help%21src%21HelpSect...

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

issue credits for @Amber's review

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

nice work folks 🎉

Committed 973da43 and pushed to 8.8.x. Thanks!

  • larowlan committed 973da43 on 8.8.x 
    Issue #2994748 by jhodgdon, andypost, Amber Himes Matz: Make a way for...

Status: Fixed » Closed (fixed)

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