I would like to propose a change to this module that will help guide administrators in configuration even if they haven't read the INSTALL.txt. Specifically, I think the status report mechanism is underutilized and could be leveraged to report problems or inconsistencies. Of particular interest:

  • [Image gallery] Add a warning to the status report if the image gallery module is enabled and there are no image galleries defined. This warning could have a link to the gallery creation page.
  • [Image import] Add an error to the status report if the image import directory does not exist or the web container cannot write to it.
  • [Image import] Add an error to the status report if the image temp directory does not exist or the web container cannot write to it.
  • [ImageMagick advanced options] If the ImageMagick Advanced Options module is installed but the image.imagemagick.inc is not installed such that image_get_available_toolkits() does not return the imagemagick toolkit, an error should appear on the status report.

Implementing these minor changes would have a noticeable impact on ease of identifying or diagnosing administration issues, particularly in cases in which the Drupal installation comes with the image module.

CommentFileSizeAuthor
#6 image_267489_6.patch6.91 KBpaul.lovvik
#5 image_267489_5.patch6.91 KBpaul.lovvik
#3 image_267489.patch4.14 KBdrewish
#1 imageStatus.patch7.91 KBpaul.lovvik

Comments

paul.lovvik’s picture

paul.lovvik commented
Status: Active » Needs review
StatusFileSize
new imageStatus.patch7.91 KB

I have created a patch that implements these checks using hook_requirements().

drewish’s picture

drewish commented

very cool! anyone have a chance to review this?

drewish’s picture

drewish commented
StatusFileSize
new image_267489.patch4.14 KB

looks pretty good.

hook_requirements phpdoc comments aren't really necessary. usually you just say:

/**
 * Implementation of hook_requirements().
 *
 * Optionally, include something about what you're checking.
 */
function foo_requirements...

I joined some of the short lines... I used to be strict about wrapping at 80 chars but since most Drupal code doesn't I try to stick with the existing code. For the image module I only wrap comments at 80 chars.

Re-ordered some of the im_advanced code so that errors are returned then warning/okay.

paul.lovvik’s picture

paul.lovvik commented

This patch looks great! Thank you for the careful look and rework.

paul.lovvik’s picture

paul.lovvik commented
StatusFileSize
new image_267489_5.patch6.91 KB

The patch in #3 is missing the modifications to the image_im_advanced module. This new patch includes the image_im_advanced modification.

paul.lovvik’s picture

paul.lovvik commented
StatusFileSize
new image_267489_6.patch6.91 KB

Changed "else if" -> "elseif".

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work

All of those requirements except image_im_advanced_requirements() add nothing but cruft to the system status report. Please remove the other.

That said, shouldn't Image module check for availability of ImageMagick when it's configured to use it? (instead of image_im_advanced?)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Closed (won't fix)

This was a nice idea, but it's not going to happen anymore.