Provide a hook_help text

Assigning this to myself. Should have a decent patch by the end of today.

Changing category from Bug to Task (hook_help is working, the doco is shows is just not complete)

Priority from "normal" to "minor" (help is not intended to provide a substantial amount of info)

Component from UI to Code because the text needs to be put into code and submitted as a patch.

Assigned to myself, so others don't waste their time working on this issue, because I've started on it :-)

Here are before and after screenshots to help review.

Unassigning the task from myself so others won't shy away from testing this patch

Thanks for the review @ifrik, and or course your help would be greatly appreciated

However, your comment is quite vague, could you be a bit more specific about what exacty does not meet the standard?

The release notes for Drupal 8.1.0 indicates an " Improved site administration experience: * Improved admin/help page to be more flexible and list tours on it."

The Help text standard (for core and contrib) gives a detailed account of how to do this the 'drupal way'.

The way I've done it elsewhere is to carefully implement each aspect (1-4) and apply patch(s) to meet the specific module's use case. We'll see how that approach works here.

Following is my review checklist w/ comments of what is included with this patch.

1. Short description of what the module does. It is displayed on the Extend or Modules page (in Drupal 8 or 7). It is the only texts users will see if the module is not enabled yet.

^-- This was actually already in place and the format looks ok to me.

2. Description on links are displayed with the links on the Configuration and Structure pages and invite users to do something.

^-- This refers to the hover text ( or tooltip) in play when hovering over the link to /admin/structure/paragraph_type . This looks OK to me.

3. Explanations on the administration pages. Ideally this should not be needed, but if they do they are short and do not duplicate the help page.

^-- Not need in this case.

4. Help page displayed by the Help module with three sections: What does the module do, what can users do with it, and a link to the online documentation here on drupal.org. This hook_help() text is in the my_module.module file.

^-- The bulk of the work for this patch resides here. It includes the three required section with descriptive text to help user understand the particulars of this module. Also, Drupal standards call for a readme.txt file to be included with contributed projects. And, there is one for this module

Thanks @ifrik

I reviewed your patch @ #2702557: Edit UI texts for admin pages nice job there, and yes ... definately tweaking what I started here would be appreciated.

I've been targeting HELP on the modules we are using on D8 site builds. We'd like to get tours in them for our customers, but want to get decent HELP in place first. Thats why I reference the Release 8.1.0 notes

Appreciate your comments.

+++ b/modules/paragraphs_demo/paragraphs_demo.module
@@ -5,14 +5,38 @@
+function paragraphs_demo_help($route_name, RouteMatchInterface $route_match){

+++ b/modules/paragraphs_type_permissions/paragraphs_type_permissions.module
@@ -7,6 +7,28 @@
+function paragraphs_type_permissions_help($route_name, RouteMatchInterface $route_match){

+++ b/paragraphs.module
@@ -19,14 +19,22 @@ define('PARAGRAPHS_DEFAULT_FORM_DISPLAY_MODE', 'default');
+function paragraphs_help($route_name, RouteMatchInterface $route_match){

A whitespace missing between ) and {

+++ b/modules/paragraphs_demo/paragraphs_demo.module
@@ -5,14 +5,38 @@
+ ¶

Extra whitespace.

+++ b/modules/paragraphs_type_permissions/paragraphs_type_permissions.module
@@ -7,6 +7,28 @@
+      $output .= '<dt>' . t('Configuring permissons per paragraphs type') . '</dt>';
+     $output .= '<dd>' . t('Administrators can configure the permissions to view, create, edit and delete each <em>paragraphs type</em> individually on the <a href=":permissions">Permissions page</a>.', array(':permissions' => \Drupal::url('user.admin_permissions'))) . '</dd>'; ¶
+      return $output;

Indentation.

I have added hook_help() with appropriate information, please apply and test this patch.

Looks better to me now but I would like to get more feedback.

+++ b/modules/paragraphs_demo/paragraphs_demo.module
@@ -5,14 +5,36 @@
+      $output .= '<p>' . t('The Paragraphs Demo module provides several <em>paragraphs types</em> for the <a href=":paragraphs">Paragraphs module</a>, but no separate user interface. For more information, see the <a href=":online">online documentation for the Paragraphs module</a>.', array(':online' => 'https://www.drupal.org/node/2444881', ':paragraphs' => \Drupal::url('help.page', array('name' => 'paragraphs')))) . '</p>';
...
+      $output .= '<dd>' . t('Adminstrators can edit the provide <em>paragraphs types </em> on the <a href=":paragraphs">Paragraphs types page</a> if the <a href=":field_ui">Field UI</a> module is enabled. For more information on fields and entities, see the <a href=":field">Field module help page</a>.', array (':paragraphs' => \Drupal::url('entity.paragraphs_type.collection'), ':field' => \Drupal::url('help.page', array('name' => 'field')), ':field_ui' => (\Drupal::moduleHandler()->moduleExists('field_ui')) ? \Drupal::url('help.page', array('name' => 'field_ui')) : '#')) . '</dd>';
...

[and more...]

Please don't use Drupal::url(...), since it is deprecated. Use Url::fromRoute(...)->toString() instead.

@ifrik wrt #19

What do you think about the changes that I made to your wording? Did I miss anything?

I think the wording definately moved this patch forward, nice job on that

@yongt9412 wrt #21

Looks better to me now but I would like to get more feedback.

I agree with you @yongt9412 looks like @ifrik's #19 patch cleaned up the whitespace errors you identified

@tduong wrt #22

Please don't use Drupal::url(...), since it is deprecated. Use Url::fromRoute(...)->toString() instead.

guess I'm confused about this comment, the HELP standard were are addressing is here -->https://www.drupal.org/node/632280

on that page there is a section "Drupal 8 note about url() and routing"

??? The way I understand this, @ifrik has followed the standard ???

guess I'm confused about this comment, the HELP standard were are addressing is here -->https://www.drupal.org/node/632280

I guess that needs to be updated ^^'

tduong, can you point me to the issue or so that depriciated Drupal::url() ?

I pointed out them yesterday but saw that the comment was too long, so I've truncated it. Basically you need to update the deprecated Drupal::url() each time you put the link to the a-tag with a route name. In details you have:

• in paragraphs.module: 2 calls for help.page, 2 for entity.paragraphs_type.collection and 1 for paragraphs.prepare_uninstall
• in paragraphs_demo.module: 3 calls for help.page and 2 calls for entity.paragraphs_type.collection
• in paragraphs_type_permissions.module: 1 call for help.page and 1 for user.admin_permissions

Furthermore, please also switch the array() to its shortened syntax [] according to the array standard coding.

And...

+++ b/modules/paragraphs_type_permissions/paragraphs_type_permissions.module
@@ -7,6 +7,27 @@
+ * Provides a help text for the Paragraphs Type Permissions  module.

double space :P

Btw, I was wondering if it would be better if we start to initialise $output = ''; outside the switch control structure and place return $output; at the end of the hook_help method (?) also to avoid a "missing return statement" when you do an "inspect code" of the module...
But this is just my opinion. For now let's fix the remaining things here, and commit nice help pages for Paragraphs! :D

@ifrik @tduong thanks for you comments

I guess that needs to be updated

yep, looks like the the Help text standard (for core and contrib) needs to be update per the "Deprecated" notice on public static function Drupal::url

Still needs work:

1. +++ b/paragraphs.module
   @@ -18,15 +19,22 @@ define('PARAGRAPHS_DEFAULT_FORM_DISPLAY_MODE', 'default');
   +      $output .= '<dd>' . t('Adminstrators can add fields to a <em>paragraphs type</em> on the <a href=":paragraphs">Paragraphs types page</a> if the <a href=":field_ui">Field UI</a> module is enabled. The form display and the display of the paragraphs type can also be managed on this page. For more information on fields and entities, see the <a href=":field">Field module help page</a>.', [':paragraphs' => Url::fromRoute('entity.paragraphs_type.collection')->toString(), ':field' => Url::fromRoute('help.page', array('name' => 'field'))->toString(), ':field_ui' => (\Drupal::moduleHandler()->moduleExists('field_ui')) ? Url::fromRoute('help.page', array('name' => 'field_ui'))->toString() : '#']) . '</dd>';
   

   you still have 2 array() to be shortened in paragraphs.module. And typo: "Adminstrators".

2. +++ b/modules/paragraphs_demo/paragraphs_demo.module
   @@ -5,14 +5,37 @@
   +      $output .= '<dd>' . t('Adminstrators can edit the provide <em>paragraphs types </em> on the <a href=":paragraphs">Paragraphs types page</a> if the <a href=":field_ui">Field UI</a> module is enabled. For more information on fields and entities, see the <a href=":field">Field module help page</a>.', [':paragraphs' => Url::fromRoute('entity.paragraphs_type.collection')->toString(), ':field' => Url::fromRoute('help.page', ['name' => 'field'])->toString(), ':field_ui' => (\Drupal::moduleHandler()->moduleExists('field_ui')) ? Url::fromRoute('help.page', ['name' => 'field_ui'])->toString() : '#']) . '</dd>';
   

   Typo: "Adminstrators".

3. +++ b/modules/paragraphs_type_permissions/paragraphs_type_permissions.module
   @@ -4,9 +4,31 @@
   +    // Help for the paragraphs type permissons module.
   ...
   +      $output .= '<dt>' . t('Configuring permissons per paragraphs type') . '</dt>';
   

   2 typos: "permissons".

4. +++ b/modules/paragraphs_demo/paragraphs_demo.module
   @@ -5,14 +5,37 @@
   -function paragraphs_demo_preprocess_node(&$variables) {
   -  // If more general approach is needed then implement preprocessor for
   -  // paragraph.html.twig.
   + function paragraphs_demo_preprocess_node(&$variables) {
   +   // If more general approach is needed then implement preprocessor for
   +   // paragraph.html.twig.
   

   what happened ? it was fine as before :)

Otherwise looks fine to me :)

+++ b/modules/paragraphs_demo/paragraphs_demo.module
@@ -5,14 +5,37 @@
 function paragraphs_demo_preprocess_node(&$variables) {
...
+   // If more general approach is needed then implement preprocessor for
+   // paragraph.html.twig.

These lines should only be indented 2 spaces, right?

Here's a patch with the change I suggested in #32.
Regards,
loop

I applied -32.patch by @ifrik to fresh git clone of --branch 8.x-1.x. It applied cleanly.

I did a visual review of the code to compare -32.patch to another local install patched with the -19.patch. I was interested to learn URL:: and the shortened syntax for the array. I clicked thru all links and read all revised content. It looks good to me.

Thanks for the work on this @ifrik, and for your keen reviews @tdoung. For me, it will provide a good model reference for HELP updates on other core/contrib modules moving forward.

wrt -33.patch by @loopduplicate ... imo ... guess that seems out of scope for this issue, but I guess cleanup of comment spacing is not a bad thing

"guess that seems out of scope for this issue, but I guess cleanup of comment spacing is not a bad thing"
The patch in #33 leaves the comment alone. The patch in #31 altered the comment and put in extra space.

Regards,
loop

"guess that seems out of scope for this issue, but I guess cleanup of comment spacing is not a bad thing"
The patch in #33 leaves the comment alone. The patch in #31 altered the comment and put in extra space.

Yes, until #31 that indentation has been added probably by mistake (as @ifrik wrote, it was probably for his new vim setup). In #33 it has just been reverted as it is on the branch now, so on commit those lines won't be changed, thus "no unrelated changes", just a healthy "patch cleanup" ;)

oooo ... my bad .... so, nice catch @loopduplicate, I didn't pick up on that

This help has some problems actually:

1. +++ b/modules/paragraphs_demo/paragraphs_demo.module
   @@ -5,6 +5,29 @@
   + * Provides a help text for the Paragraphs demo module.
   

   This should instead say:

   Implements hook_help().

2. +++ b/modules/paragraphs_demo/paragraphs_demo.module
   @@ -5,6 +5,29 @@
   +      $output .= '<p>' . t('The Paragraphs Demo module provides several <em>paragraphs types</em> for the <a href=":paragraphs">Paragraphs module</a>, but no separate user interface. For more information, see the <a href=":online">online documentation for the Paragraphs module</a>.', [':online' => 'https://www.drupal.org/node/2444881', ':paragraphs' => Url::fromRoute('help.page', ['name' => 'paragraphs'])->toString()]) . '</p>';
   

   paragraphs types -> paragraph types

3. +++ b/modules/paragraphs_demo/paragraphs_demo.module
   @@ -5,6 +5,29 @@
   +      $output .= '<dt>' . t('Changing demo paragraphs types') . '</dt>';
   

   paragraphs types -> paragraph types

4. +++ b/modules/paragraphs_demo/paragraphs_demo.module
   @@ -5,6 +5,29 @@
   +      $output .= '<dd>' . t('Administrators can edit the provide <em>paragraphs types </em> on the <a href=":paragraphs">Paragraphs types page</a> if the <a href=":field_ui">Field UI</a> module is enabled. For more information on fields and entities, see the <a href=":field">Field module help page</a>.', [':paragraphs' => Url::fromRoute('entity.paragraphs_type.collection')->toString(), ':field' => Url::fromRoute('help.page', ['name' => 'field'])->toString(), ':field_ui' => (\Drupal::moduleHandler()->moduleExists('field_ui')) ? Url::fromRoute('help.page', ['name' => 'field_ui'])->toString() : '#']) . '</dd>';
   

   provide -> provided
   paragraphs types -> paragraph types

5. +++ b/modules/paragraphs_demo/paragraphs_demo.module
   @@ -5,6 +5,29 @@
   +      $output .= '<dt>' . t('Deleting demo paragraphs types') . '</dt>';
   

   paragraphs types -> paragraph types

6. +++ b/modules/paragraphs_type_permissions/paragraphs_type_permissions.module
   @@ -4,9 +4,31 @@
   + * Provides a help text for the Paragraphs Type Permissions module.
   

   This should say:

   Implements hook_help().

7. +++ b/modules/paragraphs_type_permissions/paragraphs_type_permissions.module
   @@ -4,9 +4,31 @@
   +      $output .= '<p>' . t('The Paragraphs Type permission module allows administrators to configure permission individually for each <em>paragraphs type</em>. For more information, see the <a href=":online">online documentation for the Paragraphs module</a>.', [':online' => 'https://www.drupal.org/node/2444881']) . '</p>';
   ...
   +      $output .= '<dt>' . t('Configuring permissions per paragraphs type') . '</dt>';
   

   What is the real name of this module? I guess it is probably

   Paragraph Type Permissions

   ? Make sure it matches here what is in the .info.yml file.

   Also
   configure permission -> configure permissions
   paragraphs type -> paragraph type

8. +++ b/modules/paragraphs_type_permissions/paragraphs_type_permissions.module
   @@ -4,9 +4,31 @@
   +      $output .= '<dd>' . t('Administrators can configure the permissions to view, create, edit and delete each <em>paragraphs type</em> individually on the <a href=":permissions">Permissions page</a>.', [':permissions' => Url::fromRoute('user.admin_permissions')->toString()]) . '</dd>';
   

   paragraphs type -> paragraph type

   Also there should be a comma before and

9. +++ b/paragraphs.module
   @@ -18,15 +19,22 @@
   +      $output .= '<p>' . t('The Paragraphs module provides a field type that can contain several other fields and thereby allows users to break content up on a page. Administrators can predefine <em>paragraphs types</em> (for example a simple text block, a video, or a complex and configurable slideshow). Users can then place them on a page in any order instead of using a text editor to add and configure such elements. For more information, see the <a href=":online">online documentation for the Paragraphs module</a>.', [':online' => 'https://www.drupal.org/node/2444881']) . '</p>';
   

   paragraphs types -> paragraph types

   I am getting tired of typing this... please make this fix in the rest of the patch.

10. +++ b/paragraphs.module
    @@ -18,15 +19,22 @@
    +      $output .= '<dt>' . t('Preparing to uninstalling paragraphs') . '</dt>';
    

    This is not grammatical

11. +++ b/paragraphs.module
    @@ -18,15 +19,22 @@
    +      $output .= '<dd>' . t('The Paragraphs module cannot be uninstalled when there is Paragraphs data on your website. Users with the appropriate permissions can delete all relevant data by clicking <em>Delete Paragraphs data</em> on the <a href=":uninstall">Prepare uninstall page</a>.', [':uninstall' => Url::fromRoute('paragraphs.prepare_uninstall')->toString()]) . '</dd>';
    

    We don't tell how to uninstall any other similar modules. We should really just drop this.

Oh sorry. I was led here by a Drupal Core issue and didn't realize this was a separate module until after that review.

You can ignore me... although I think it would be good if the grammar and standards were cleaned up as suggested there, you can do as you wish in your contrib module. Sorry about intruding!

Thanks @jhodgdon for the feedback. I am wondering what should be done since the original module is called "Paragraphs" and "Paragraphs types permissions" is a sub module of it. IMHO it looks like a grammar mistake but is because the sub module name is based on the module name. What do you suggest?

+++ b/modules/paragraphs_demo/paragraphs_demo.module
@@ -5,6 +5,29 @@
+ * Implements hook().

+++ b/modules/paragraphs_type_permissions/paragraphs_type_permissions.module
@@ -4,9 +4,31 @@
+ * Implements hook_help.

This should be changed to: Implements hook_help().

added a patch with fixes regarding the issues mentioned in #44

Thanks!
~Chirag

Please also provide an interdiff with your changes.

The interdiff for the previous patch

Thanks everyone for such amazing patches, few small things that we needed to change. :)

+++ b/modules/paragraphs_type_permissions/paragraphs_type_permissions.module
@@ -1,7 +1,8 @@
- * Add view / create / update / delete permissions for all paragraph types.
+ * Contains paragraphs_type_permissions.module

Personally I don't see the point writing this, I mean.. it is clear.. but this is also used e.g. in image_widget_crop and dropzonejs...
Otherwise I would set it as RTBC ;)

I think is fine anyway...

Committed. Finally, the user isn't left alone and has some help. .-)
We can always improve in small follow-ups.

+1

Great job everyone on this, especially @ifrik for you persitance and grace in responding to numerous comment/reviews. It represents a lot of work on such a 'minor' task but has led to good discussion on some larger issues wrt core and HELP standards. Some are tagged below for future reference.

#2731835: Fix all hook function bodies so that their example code uses [] instead of array() syntax.

#2731817: Replace all calls to the deprecated Drupal::url() function in Core