WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 10 months ago

#15606 closed enhancement (duplicate)

Support thumbnails for non-image media types

Reported by: acumensystems Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Media Keywords:
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 4 years ago.
Post.php Logic Clarification

Download all attachments as: .zip

Change History (19)

comment:1 @acumensystems4 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 @westi4 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 @acumensystems4 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 @acumensystems4 years ago

Westi,

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

Leo

@acumensystems4 years ago

Post.php Logic Clarification

comment:5 @acumensystems4 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 @nacin4 years ago

The import check was dropped and some logic changed.

comment:7 @acumensystems4 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 @acumensystems4 years ago

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

comment:9 @Otto424 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 @Otto424 years ago

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

comment:11 @acumensystems4 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 @iseulde20 months ago

  • Keywords reporter-feedback needs-patch added

comment:13 @ericlewis10 months ago

  • Resolution set to invalid
  • Status changed from new to closed

Images will always have a top-level media type of "image". Their mime-types will always take the format of "image/[subtype]" (See RFC-2046).

So with this check: 'image/' == substr($post->post_mime_type, 0, 6) we're all good. Closing this out.

comment:14 @ericlewis10 months ago

  • Keywords reporter-feedback needs-patch removed

comment:15 follow-up: @SergeyBiryukov10 months ago

  • Milestone Future Release deleted

The suggestion in comment:9 was to add a filter on the return value of wp_attachment_is_image(), to allow plugins create thumbnails for PDFs or other file types.

No traction in 3 years, feel free to reopen if this is still relevant.

Last edited 10 months ago by SergeyBiryukov (previous) (diff)

comment:16 in reply to: ↑ 15 @ericlewis10 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Summary changed from Remove Hardcoded Image Extensions to Support thumbnails for non-image media types

Replying to SergeyBiryukov:

The suggestion in comment:9 was to add a filter on the return value of wp_attachment_is_image(), to allow plugins create thumbnails for PDFs or other file types.

I missed the point here. Let's re-open this. That would be cool to support. I think the focus of this ticket on the wp_attachment_is_image() is too narrow, and adding filters here shouldn't be considered. This function has one job and is doing it perfectly. Let's open the floor for other options.

comment:17 @Otto4210 months ago

I might be wrong, but I think all one would need to do to add thumbnails for other files is to a) generate them, b) save them somewhere in wp-content, and c) add their file paths to the attachment_metadata using the 'thumb' name.

If you do that, then it seems like wp_get_attachment_thumb_url will try to do image_downsize, fail on that because it's not an image, then call wp_get_attachment_thumb_file, which returns the 'thumb' metadata, which gets str_replaced into a URL.

comment:18 @ericlewis10 months ago

  • Resolution set to duplicate
  • Status changed from reopened to closed

Duplicate of #20205.

Note: See TracTickets for help on using tickets.