Posted by eojthebrave on December 12, 2009 at 3:28pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | image.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Novice |
Issue Summary
hook_image_effect_info() allows you to include a 'help' text with your effect definition.
"help": (optional) A brief description of the effect that will be shown when adding or configuring this image effect.
However, image_effect_form() never displays the help text.
Fix: Display the $effect['help'] information if present at the top of the effect add/edit form.
Comments
#1
This should do it. And marking as critical since this really should be fixed for Drupal 7.
#2
This patch applies cleanly and displays the effect help at the top of the form.
#3
The last submitted patch failed testing.
#4
matason requested that failed test be re-tested.
#5
+++ modules/image/image.admin.inc@@ -378,6 +378,9 @@ function image_effect_form($form, &$form_state, $style, $effect) {
+ $form['effect_help'] = array(
+ '#markup' => isset($effect['help']) ? $effect['help'] : '',
+ );
Without studying the surrounding code, is there a reason why we don't use $form['#markup'] instead of $form['effect_help']['#markup']? (I should probably look at the surrounding code.)
#6
Dries, I am not sure exactly why but $form['#markup'] doesn't seem to get picked up, it seems to need to be nested...
#7
A missing help text is not a critical bug. It is an important bug, but not one that should block a release.
#8
The last submitted patch, , failed testing.
#9
Re-test of from comment #2386316 was requested by @user.
#10
Dries, regarding #7, I completely agree, this is an easy fix, #1 does the job.
#11
Is there a reason this isn't in hook_help()?
#12
I believe it was done this way so that modules which add additional effects could declare the help for those effects in hook_image_effect_info(). Though image.module could collect them into hook_help(), or every module that declares image effects could implement hook_help() itself. Preference?
#14
Here's a patch that displays the help text with hook_help().
#15
#14 is the right way to do this. Thanks!
Assuming this passes tests (and I can't see why it wouldn't) this is ready to go.
Maybe not a critical issue, but an important one for sure. Sticking with the theme of making Drupal 7 the most user friendly version of Drupal ever lets make sure this gets in. There are no API changes and it is a super simple change fixing something that should have been working in the first place.
#16
Great. Thanks a lot, folks!
Committed to HEAD.
#17
Automatically closed -- issue fixed for 2 weeks with no activity.