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.

CommentFileSizeAuthor
#4 image-HEAD.image-block-number.patch3.2 KBsun
#3 configurable_number_of_random_latest_images_v2.patch4.16 KBsimon.males
configurable_number_of_random_latest_images.patch3.73 KBsimon.males

Comments

drewish’s picture

drewish commented
Status: Needs review » Needs work

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 ('').

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

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.

simon.males’s picture

simon.males commented
Status: Needs work » Needs review
StatusFileSize
new configurable_number_of_random_latest_images_v2.patch4.16 KB

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.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Fixed
StatusFileSize
new image-HEAD.image-block-number.patch3.2 KB

Thanks, committed attached patch.

@simon.males: Please take a close look at this patch and compare with your approach.

simon.males’s picture

simon.males commented

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!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Committed to 5.x-2.x as well.

Status: Fixed » Closed (fixed)

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