Configurable number of images in Latest/Random blocks
simon.males - December 23, 2008 - 13:36
| Project: | Image |
| Version: | 6.x-1.x-dev |
| Component: | image.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
I suggest the number of images displayed in the Latest and/or Random blocks should be configurable.
The configuration is to be done in the respective blocks configuration page via a single field. Each block will have its own variable in the {variable} table.
The number of images, or input, must be an integer. To keep it realistic the integer value cannot be greater then 99.
| Attachment | Size |
|---|---|
| configurable_number_of_random_latest_images.patch | 3.73 KB |
#1
This looks really good with the exception of these lines:
+ $output = null;NULL should be in caps but I'd rather we just use an empty string ('').
#2
Also,
return $block;+ case 'configure':
Missing blank line between switch cases.
+ case 'configure':...
+ }
case 'view':
Missing break for 'configure' case.
+ $form['latest_num_images'] = array(+ '#type' => 'textfield',
+ '#title' => t('Latest images to display'),
+ '#size' => 5,
+ '#maxlength' => 2,
+ '#default_value' => variable_get('image_latest_num_images', 1),
+ );
...
+ // Ensure submitted value is an integer. Don't ever show more then 99 images.
+ case 0:
+ $edit['latest_num_images'] = intval($edit['latest_num_images']);
+ $edit['latest_num_images'] = ($edit['latest_num_images'] > 99) ? 99 : $edit['latest_num_images'];
If we would use #type 'weight', or a simple select list, we would not have to verify that the value is an integer. IMO, a weight/select field with values from 1 to 99 would be the way to go.
Additionally, wrong indentation here in the form definition.
#3
I've taken all coding styling/standards suggestions on board with the attached patch. Rather then a free textfield the configuration is done by a select list (sun's suggestion). A new function called image_block_display_range() generates an array for #options.
Since the form is validated by the FAPI I don't see the need to wrap the input in an intval() anymore. Though I'm not sure of the best practice here. Is it taboo to store variables as strings if they are only going to used as integers?
Lastly, I updated the _uninstall hook so the module cleans up after itself.
Thanks for your suggestions.
#4
Thanks, committed attached patch.
@simon.males: Please take a close look at this patch and compare with your approach.
#5
I really appreciate bringing this to my attention.
drupal_map_assoc() is now obvious to me. Never needed to use it until now and it elegantly drops the need for a silly function.
I liked how you reduced code by using $delta directly rather then constantly running it through a switch.
Lastly I noticed the use of double quotes in variable_get/set which I wasn't aware about.
Cheers!
#6
Committed to 5.x-2.x as well.
#7
Automatically closed -- issue fixed for 2 weeks with no activity.