Move custom help settings to blocks

I haven't tested this yet, but from a casual inspection this looks fantastic. +++1

By making help text blocks, in addition to the other positive points noted by Gábor, we also automatically gain, for free:
- the ability to selectively display (or not) help text by role, using the standard role/block ui.
- the ability to set the display of a help block as user configurable, and then allow a user to turn off help using her My Account page.
- the ability to set custom PHP code that controls the display of a help block, like automatically hiding it when the system is running in system_admin_compact_mode() from someone having pressed the "Hide descriptions" link.

This patch looks pretty good and works fine. Just a few comments:

The block does not depend on the help module, but the page linked to by the 'more help' link does. Where is the distinction? Shouldn't this help block be handled in help_block() instead of system_block()?

The notification messages set with drupal_set_message() are also help messages (although they are activity dependant rather than location dependant), and are generally displayed at the top of that same "help" region. Could these be turned into a block as well?

The patch is great. I tried out most everything I could think of, and really like the new functionality of help-as-block.

A couple of things:

1. As Gábor noted in his initial post, the upgrade path is not quite right. In particular, if this is a new installation of Drupal, and one has perhaps never installed contact module, then the custom block for contact module never gets created. In my installation, the custom block for user/register information was not created either. Obviously, if this happens, you have no way of editing site-wide contact page information or user/registration headers, until you create the custom blocks manually. I guess the .install routines for system and contact modules should create these custom blocks. I'm unsure what should happen if contact module is later disabled -- the block should also be disabled (and deleted if not modified?) perhaps.
2. There were a number of instances in the help text that must be modified to conform to this change. In particular, references in the help text to providing additional contact information must be modified. Additionally, we should add some help in both user module and blocks module to explain this new use of custom blocks.
3. There were one or two comments not ending with a period, not due to this patch, but this patch moves them.

The attached patch, which I am setting to "needs work" since I know that it does, addresses points 2 and 3, but not 1.

Moving help into blocks and regions makes a lot of sense. I'm not convinced about using 'boxes' as a storage method though.

If we stored help text in it's own table, could have a single help block (or two/three) which displays stuff conditionally based on path/module etc. then it ought to be easier to do import and export on it, integrate with localisaton etc.. Otherwise we could be looking at dozens of help blocks in the admin interface and you'd have to set up permissions and rules for them all individually alongside all the other blocks.

If help has it's own storage, could also have a flag for "inline" or "block" for help texts - so you could programmatically add help text to fapi elements etc as well as to whole pages, configure it to be collapsed, popup, etc. etc..

I haven't actually tested the patch though so might be making some false assumptions.

edit: having said that, site missions, footer etc. as 'boxes' sounds great. Just not sure about storing help in there.
edit again: custom data storage would also let us write hook_search and have search/help etc.

I don't think I looked at this closely enough. For some reason I thought this was the beginnings of moving hook_help() stuff into boxes, which seemed weird ;) Refactoring hook_help is a whole different issue, but the one dynamic help block displaying output from it is a great first step. As is moving the contact text into a box instead of the variables table, which makes loads of sense.

Just for clarity, I'm not talking about a help editor module in core, just a flexible enough API which would allow for such a module to exist cleanly (i.e. not boxes!). Fortunately that's a completely separate issue, so in general +1 from me and sorry for the confusion!

In terms of interest, I think this is a great patch, as I've mentioned before.

It seems we need to address at least:
- installing the contact help and user registration help custom blocks by default on install (or enable of contact module). (I think that the current patch will only create these blocks if text exists for either contact module or user registration help in the variables table, and without the default installation, I'm not certain how the default text gets into the block.)
- determine whether the contact help should go away when contact.module is disabled.
- review the help text added in the patch in #4 to make sure it accurately represents these changes.

Hi ,

I want to display help text for my site pages is specific help block that may appear in the right side column say.I have a module say 'x' which has 2 different menu router paths.

So,I did the following:
- I merge the above patch.
- In my module write a update hook which adds a block to the blocks table for my theme and a sentence in the 'boxes' table.
- Add a hook_help in which I mention the paths for my module.

I disable,enable and install the new update(which i wrote above).

- The block with the text do appear but the when goto menu paths,the help texts doesnot appear in the block

So,here are the missing links I want to know:
- how the patch links the my hook_help to the custom help block I have generated.The patch doesnot seem to have any info!!??

I'm sorry for mistakenly putting in 6.3 thread.

However,do you have any suggestion for the type of requirement I'm trying to materialize?!?

+1

However, needs a re-roll. Some issues in the update function are obvious; I can fix those after it has been re-rolled.

Subscribing, this still is interesting and relevant to how we can provide useful inline help.

Looking at d7's new page.tpl.php, I see that $help comes right before the $content variable. If we made $content a region, having help be a region too seems redundant too me. That, of course, is completely dependent on #428744: Make the main page content a real block actually being committed.

This looks good but I have one concern with moving everything into blocks. There will be so many blocks to deal with. It's already a problem with complex sites and this would make it even noisier in the block config page.

This is off-topic but if each block had the ability to tag or classify itself on what type of content it's pushing out, it would allow filtering. Maybe even allow all the block visibility settings for a given type of block. Taxonomy on blocks? Maybe not but you get the idea.

dvessel, that's a valid concern, but the flexibility this would allow by far outweighs this, IMO.
Improving the blocks page is something crucial, though.
Jeff Noyes mocked up something a while ago, which goes in the direction Lullabot is heading with the new Buzzr project.
While I wholeheartedly hate the current admin/build/block, what Gabor outlined above really makes my head spin (in a good way).

If we have a problem with blocks administration being overloaded by too many blocks, which I agree is a problem, then we need to look at ways to make block admin scale better. That's not a reason for core to provide less blocks - the interface is fine when you just have core running.

In terms of moving custom help texts to custom blocks - if the idea is that we essentially remove the feature of custom help texts from those modules and provide this as an upgrade path, then that sounds great to me. And if that's the case, then I think it scores against a specific 'help' region - for system provided help, themes can just as easily style #block-help as a help region, or add their own styled help region themselves. The main question then is where to put the help block in the upgrade path.

A new Drupal 7 site is going to have neither of these blocks, and the forms in user and contact modules have been removed - so on install you'd get a single help region with a single help block. I don't think the fact that two core modules decided to re-implement the custom blocks system using hook_help() in c.2004 is a sufficient reason to make a separate help region.

There's nothing intrinsic to the contact and and user registration blocks created in the upgrade path that identifies them as help - apart from the fact the content used to be in a variable + hook_help() prior to the patch. I don't think that's a problem, and we could just let them live on as custom blocks on existing sites. Having said that, I'm not opposed to a new region, I just want to examine the reasons for one carefully, and these two blocks doesn't seem like enough.

Should the code to enable the block should also be in help_enable() as well as the core profiles?

Additionally - once this is in, do we want to look at moving node type help to blocks in a followup patch? That's another ugly use of hook_help() which should really go and die, and requires all kinds of ungodly things in i18n module to make it translatable.

Didn't slip my attention, I just don't think core needs to support or suggest this for custom blocks at all - but I'm more interested in the patch getting in than this detail, so will support consensus in either direction.

Despite the test failing, I think this is a nice clean-up. As far as I'm concerns this can be marked RTBC once the test succeed.

One minor nitpick was the wording of "System generated help for page" -- I felt that "for page" sounded a little bit forced but I'm not native English. It is something we can revise. Like permissions, it would be nice if we had some additional description for each block on the block administration page. But as suggested by catch, that's another issue.

Tagging it.

I'd probably just call it 'system help'.

Nice stuff. I've rerolled the patch (attached) so it now applies to the latest HEAD.

Some comments:

• These variables used to display with filter_xss_admin(), but in the upgrade code they are switched to the default text format, so for some sites, the formatting will break when they upgrade from D6, right? I've struggled with the exact same issue at #391924: Use Fields to store text variables, especially those that require text format support, so would love to know the right way to deal with it :) Maybe it should just be as simple as printing a note somewhere documenting that fact that the format changed?
• Is there any reason the update function can't delete the 'contact_form_information and 'user_registration_help' variables once it's done with them?
• Seems to have been discussed a bit above, but I'm not sure I understand what (if anything) the resolution was: Shouldn't the default install profile (and contact.install in the case of the contact module) insert these blocks upon installation so that people who want them don't have to create them themselves?
• I agree with @catch that "System help" is best...

This patch looks ready to me. Marking it RTBC -- will commit it later today unless someone disagrees.

I didn't run the update path, looks like Gabor didn't either judging by comments in #41, so I'm assuming Dries did before setting RTBC ;)

One code style nit, otherwise looks great to me too:

+    case 'help':
+      return array(
+        'subject' => NULL, // Don't display a title

Missing a period, not sure what our convention is for comments within arrays. Could be fixed on commit so not changing status.

I didn't actually test the upgrade path, and I missed Gabor's comment about not having tested it either. I did a code review of it, and that looked good. Setting back to 'needs review' so we can test the upgrade path more thoroughly.

I reviewed this patch and it looked good. This is very exciting, IMO. Committed to CVS HEAD.

There needs to be upgrade information. At least in the theme handbook.

Let's get a CHANGELOG.txt entry as well.

Huh? How could this be committed?

function contact_menu() {
  $items['contact'] = array(
    'title' => 'Contact',
...
-    $contact_form_information = $this->randomName(100);
-    $edit['contact_form_information'] = $contact_form_information;
@@ -82,7 +80,7 @@ class ContactSitewideTestCase extends Dr
-    $this->assertText($contact_form_information, t('Contact form is shown when there is one category.'));
+    $this->assertText(t('Contact'), t('Contact form is shown when there is one category.'));

"Contact" is the page title - this does not assert there is a form.

+function system_update_7021() {
...
+    $ret[] = update_sql("INSERT INTO {block} (module, delta, theme, status, weight, region, pages, cache) VALUES ('system', 'help', '". $theme->name ."', 1, 0, 'help', '', 1)");
...
+      $ret[] = update_sql("INSERT INTO {block} (module, delta, theme, status, weight, region, visibility, pages, cache) VALUES ('block', '". ($bid_max + 1) ."', '". $theme ."', 1, 5, 'help', 1, 'contact', -1)");
...
+    drupal_set_message('The contact form information setting was migrated to <a href="'. url('admin/build/block/configure/block/' . ($bid_max + 1)) . '">a custom block</a> and set up to only show on the site-wide contact page. The block was set to use the default text format, which might differ from the HTML based format used before. Please check the block and ensure that the output is right.');
...
+      $ret[] = update_sql("INSERT INTO {block} (module, delta, theme, status, weight, region, visibility, pages, cache) VALUES ('block', '". ($bid_max + 2) ."', '". $theme ."', 1, 5, 'help', 1, 'user/register', -1)");
+    }
+    drupal_set_message('The user registration guidelines were migrated to <a href="'. url('admin/build/block/configure/block/' . ($bid_max + 2)) . '">a custom block</a> and set up to only show on the user registration page. The block was set to use the default text format, which might differ from the HTML based format used before. Please check the block and ensure that the output is right.');

Wrong string concatenation.

+function system_update_7021() {
...
+  // Rebuild theme data, so the new 'help' region is identified.
+  system_theme_data();

Reminds me of #305653: Themes disabled during update. This will most probably disable all themes.

+  $blocks['help'] = array(
+   'info' => t('System help'),
+   'weight' => '5',
+  );

Wrong indentation.

       return $block;
+    case 'help':
+      return array(
+        // Don't display a title.
+        'subject' => NULL, 
+        'content' => menu_get_active_help(),
+      );
+      break;
     default:

Missing blank lines between switch cases. Also, // Don't display a title. is obvious from the next line; we should document the why's and how's or just omit the comment for the obvious.

   $form = array();
 
-  // Display the registration form.
-  if (!$admin) {
-    $form['user_registration_help'] = array('#markup' => filter_xss_admin(variable_get('user_registration_help', '')));
-  }
-
   // Merge in the default user edit fields.
   $form = array_merge($form, user_edit_form($form_state, NULL, NULL, TRUE));

array_merge() is superfluous now.

 function default_profile_tasks(&$task, $url) {
...
+  // Enable 5 standard blocks.
+  $values = array(
+    array(
+      'module' => 'user',
+      'delta' => 'login',
+      'theme' => 'garland',
+      'status' => 1,
+      'weight' => 0,
+      'region' => 'left',
+      'pages' => '',
+      'cache' => -1
+    ),
+    array(
+      'module' => 'system',
+      'delta' => 'navigation',
+      'theme' => 'garland',
+      'status' => 1,
+      'weight' => 0,
+      'region' => 'left',
+      'pages' => '',
+      'cache' => -1
+    ),
+    array(
+      'module' => 'system',
+      'delta' => 'management',
+      'theme' => 'garland',
+      'status' => 1,
+      'weight' => 1,
+      'region' => 'left',
+      'pages' => '',
+      'cache' => -1
+    ),
+    array(
+      'module' => 'system',
+      'delta' => 'powered-by',
+      'theme' => 'garland',
+      'status' => 1,
+      'weight' => 10,
+      'region' => 'footer',
+      'pages' => '',
+      'cache' => -1
+    ),
+    array(
+      'module' => 'system',
+      'delta' => 'help',
+      'theme' => 'garland',
+      'status' => 1,
+      'weight' => 0,
+      'region' => 'help',
+      'pages' => '',
+      'cache' => -1
+    ),
+  );

Missing trailing comma after last array elements ('cache').

Better suggestion: Any unique string on the contact form page that asserts there is a contact form. For example, t('Your e-mail address').

Coding standard for switch statements is documented as direct example snippet only without further explanation. That snippet uses blank lines between case/default statements, and that's also the style we're trying to foster throughout core.

I edited that quite a bit.

opsie i followed up on #51 -- the issue got commented heavily since i was here.

     case 'powered-by':
       $image_path = 'misc/' . variable_get('drupal_badge_color', 'powered-blue') . '-' . variable_get('drupal_badge_size', '80x15') . '.png';
-      // Don't display a title.
       $block['subject'] = NULL;
       $block['content'] = theme('system_powered_by', $image_path);
       return $block;
+      break;
     case 'help':
-      return array(
-        // Don't display a title.
-        'subject' => NULL, 
-        'content' => menu_get_active_help(),
-      );
+      $block['subject'] = NULL;
+      $block['content'] = menu_get_active_help();
+      return $block;
       break;
     default:

Either return or break to terminate a switch case, but not both. That has nothing to do with coding standards.

The rest looks much better now, RTBC after this remaining fix.

Awesome review, sun! Thanks. Committed to CVS HEAD.

In the d7 theme upgrade docs, I added text about needing to define the help region line in the .info file of a theme that overrides the default regions. Otherwise, the $help variable would be undefined in that type of theme’s page.tpl.php template.

Wow...this patch left lots of goodies that no longer exist like "Customize the contact page with additional information (like physical location, mailing address, and telephone number) using the contact form settings page."

:(

Follow to remove the references to the non-existant contact information setting.

No longer applies but this hunk of help text is till in HEAD.

We definitely want the remaining help text correction, but that's not critical.

Tagging novice for core mentoring.

This patch no longer applies, and it appears that the help text for the contact module has been completely re-written.