Remove Hardcoded Image Extensions

[acumensystems]

Hi,

As the web changes, browsers support more image extensions. Part of what I'm doing is write plugins that extend Wordpress's ability to thumbnail beyond the norm (PDFs for example).

In order to extend functionality and provide the most support to existing themes, one way is to override functions like wp_attachment_is_image(). Rather than full overriding, it would be better still to change the data their logic is based on. However, these functions do make some unfortunate hard-coded assumptions.

This function, in wp-includes/post.php includes the code:

$image_exts = array('jpg', 'jpeg', 'gif', 'png');

I believe that should refer to a global array. This is not in object scope, so where would be best to place it?

Best,
Leo

Complementary to this, I propose a cleanup of the final, quite unreadable, line of logic in the function:

if ( 'image/' == substr($post->post_mime_type, 0, 6) || $ext && 'import' == $post->post_mime_type && in_array($ext, $image_exts) )
                return true;

This is my proposed structuring, which I can submit a patch for as required:

// what do we know about the attachment?
$is_imported      = 'import' == $post->post_mime_type;
$is_mime_image    = 'image/' == substr($post->post_mime_type, 0, 6);
$is_thumbnailable = in_array($ext, $image_exts);

// if the mime type has this as an image, then it's an image
if($is_mime_image)
	return true;

// if it's an otherwise imported image, but we accept the file extension...
if($is_imported && $is_thumbnailable)
	return true;

I do, as stated above, think the "thumbnailable" type detection does need to be improved/abstracted - possibly as a filter, what do you think?

The logic of 'import' == $post->post_mime_type seems unnecessary - this is not a valid MIME type. Do we have any info on the background for when this would occur? Is it using Wordpress XML import/export? Or something different altogether?

In my mind, if we accept the file extension, we should accept it regardless of MIME type.

The best way to suggest changes is with a patch file rather than a comment in a ticket.

If you want to make the like of image types extendable by a plugin your patch should add a filter on them in the function.

Westi,

Understood - please see patch attached for the logic clarification I was discussing.

Though still there are a few unresolved questions, that's what I thought it better to discuss first, as this patch alone is just a cleanup.

Cheers
Leo

Westi,

My patch is somewhat pointless as I didn't delete the existing logic. Have amended.

Leo

Post.php Logic Clarification

Hi

Have now added a filter to allow amendment of the default thumbnailable extensions, and have cleaned up the logic as described.

Tested on WP trunk SVN with TwentyTen attachment pages working well.

Please consider for inclusion.

Thanks,
Leo

The import check was dropped and some logic changed.

Hi - was that a question or a summary?

To clarify - the "import" MIME type check was intentionally dropped. I have scanned the entire WP codebase, and can't find this (invalid) MIME type being set anywhere. So effectively this function would never thumbnail based on image extension, but only on a correct "image/xyz" MIME type. Most likely it was a hack for an early import function that failed to capture the MIME type from the browser upload.

Dropping the check for this fake MIME type both simplifies the code but also ensures that files with image extensions can be thumbnailed.

I am right - it looks like this was removed 4 years ago in [4552] so is redundant code. Cheers, Leo.

Your logic change here is somewhat wrong.

The code as-is only uses that hardcoded list of extensions when the mime type is "import", which is that exceedingly old way of doing imports. That is solely for backwards compatibility.

The primary image check now is through the image/ mime-type. So being able to modify the backwards-compatible list of extensions doesn't make a whole lot of sense.

Also, your patch drops the "import" type, breaking backward compatibility there and making the extension check work for all cases, which is undesirable.

Instead, you should add a filter on the "true" or "false" return value, thus allowing you to add your own function to return true for "application/pdf" or what have you.

Hi Otto,

That makes sense, I can avoid dropping the import type and add your logic if it may then be included. I'll come back to it when I have a little more time.

Thanks!
Leo