WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 8 months ago

#15606 new enhancement

Remove Hardcoded Image Extensions

Reported by: acumensystems Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Media Keywords: reporter-feedback needs-patch
Focuses: Cc:

Description

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

Attachments (1)

post.php.patch (1.2 KB) - added by acumensystems 3 years ago.
Post.php Logic Clarification

Download all attachments as: .zip

Change History (13)

comment:1 acumensystems3 years ago

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.

comment:2 westi3 years ago

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.

comment:3 acumensystems3 years ago

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

comment:4 acumensystems3 years ago

Westi,

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

Leo

acumensystems3 years ago

Post.php Logic Clarification

comment:5 acumensystems3 years ago

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

comment:6 nacin3 years ago

The import check was dropped and some logic changed.

comment:7 acumensystems3 years ago

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.

comment:8 acumensystems3 years ago

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

comment:9 Otto423 years ago

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.

comment:10 Otto423 years ago

  • Component changed from General to Media
  • Milestone changed from Awaiting Review to Future Release

comment:11 acumensystems3 years ago

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

comment:12 avryl8 months ago

  • Keywords reporter-feedback needs-patch added
Note: See TracTickets for help on using tickets.