Image toolkit supported mime types / extensions

Other than a load of code-style quibbles, this looks very good.

+++ includes/image.inc	2 Sep 2009 01:10:54 -0000
@@ -140,6 +140,52 @@ function image_get_info($filepath, $tool
 }
 
+
+/**
+ * Retrieve a list of supported mime types for the
...
+}
+
+
+
+/**

One blank line between functions.

+++ includes/image.inc	2 Sep 2009 01:10:54 -0000
@@ -140,6 +140,52 @@ function image_get_info($filepath, $tool
+ * Retrieve a list of supported mime types for the
+ * default tookit or the specified toolkit
+ */

The PHPdoc comment needs a period at the end and should line-wrap at 80 characters.

Also, specify the @param and @return values.

+++ includes/image.inc	2 Sep 2009 01:10:54 -0000
@@ -140,6 +140,52 @@ function image_get_info($filepath, $tool
+  if (is_null($toolkit)) {

I think we generally use !isset() or empty() there. All of these will return the same value when $toolkit is NULL.

+++ includes/image.inc	2 Sep 2009 01:10:54 -0000
@@ -140,6 +140,52 @@ function image_get_info($filepath, $tool
+  $function = 'image_'. $toolkit .'_mime_types';

The new style guide specifies a space on both sides of the string-concatenating dot.

+++ includes/image.inc	2 Sep 2009 01:10:54 -0000
@@ -140,6 +140,52 @@ function image_get_info($filepath, $tool
+  return FALSE;

For a consistent return value, the function should probably be returning an empty array().

+++ includes/image.inc	2 Sep 2009 01:10:54 -0000
@@ -140,6 +140,52 @@ function image_get_info($filepath, $tool
+  if (is_array($types) && count($types) != 0) {

If the return value (see above) is made into an array, you only need to check count().

It is also more readable if you use ">" instead of "!=", making it clear that you are expecting a positive value.

+++ includes/image.inc	2 Sep 2009 01:10:54 -0000
@@ -140,6 +140,52 @@ function image_get_info($filepath, $tool
+    require_once DRUPAL_ROOT .'/includes/file.mimetypes.inc';

See string concatenation; add a space.

+++ includes/image.inc	2 Sep 2009 01:10:54 -0000
@@ -140,6 +140,52 @@ function image_get_info($filepath, $tool
+      $mime_id = array_search($type, $mappings['mimetypes'], TRUE);
+      if($mime_id !== FALSE) {
+        $extensions = array_merge($extensions, array_keys($mappings['extensions'], $mime_id));
+      }

Mh... I'm really trying to understand what's going on there, and an inline comment would probably help a lot. I gather the mime type is first searched for in the type mapping, after which the id of the mime type is searched for in the extension mapping?

+++ includes/image.inc	2 Sep 2009 01:10:54 -0000
@@ -140,6 +140,52 @@ function image_get_info($filepath, $tool
+    return FALSE;

An empty array() might be a better return value here too.

+++ modules/system/image.gd.inc	2 Sep 2009 01:10:54 -0000
@@ -11,6 +11,29 @@
  */
 
+
+/**

Space between functions...

+++ modules/system/image.gd.inc	2 Sep 2009 01:10:54 -0000
@@ -11,6 +11,29 @@
+ * Retrieve a list of the mime types supported by this toolkit.
+ * 
+ * In this case, the list depends on the types 
+ * compiled into your version of GD.

See more regarding PHP doc above. This one has no parameters, but the @return value needs to be specified.

This review is powered by Dreditor.

Despite the positive feedback it is unfortunate to see that this patch did not make it to mainline. Is there a particular reason for the patch not receiving any updates before inclusion in D7?