Make WordPress Core

Opened 7 years ago

Last modified 5 years ago

#41801 assigned enhancement

add wp_get_image_extensions() and wp_image_extensions filter

Reported by: pbiron's profile pbiron Owned by: pbiron's profile pbiron
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9
Component: Media Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Core already has wp_get_audio_extensions() and wp_get_video_extensions() (which are used in wp_attachment_is()).

For consistency and convenience, core should also have wp_get_image_extensions() and wp_image_extensions filter.

Attachments (3)

41801.diff (2.0 KB) - added by pbiron 7 years ago.
41801.2.diff (4.6 KB) - added by pbiron 7 years ago.
revised patch: add unit tests, add call to wp_get_image_extensions() in populate_network()
41801.3.diff (8.4 KB) - added by pbiron 6 years ago.

Download all attachments as: .zip

Change History (29)

@pbiron
7 years ago

#1 @pbiron
7 years ago

  • Keywords has-patch added

In addition to adding wp_get_image_extensions() and wp_image_extensions filter, the patch also modifies wp_attachment_is() to use wp_get_image_extensions().

This ticket was mentioned in Slack in #core-media by pbiron. View the logs.


7 years ago

This ticket was mentioned in Slack in #core-media by pbiron. View the logs.


7 years ago

#4 follow-up: @joemcgill
7 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.0
  • Owner set to pbiron
  • Status changed from new to assigned

Thanks @pbiron. This seems like a nice addition and the patch looks good at first glance. It would be great to include a simple unit test on this function.

#5 in reply to: ↑ 4 @pbiron
7 years ago

Replying to joemcgill:

Thanks @pbiron. This seems like a nice addition and the patch looks good at first glance. It would be great to include a simple unit test on this function.

My bad...I thought I had included unit test(s). I'll get to those tomorrow.

@pbiron
7 years ago

revised patch: add unit tests, add call to wp_get_image_extensions() in populate_network()

#6 follow-up: @pbiron
7 years ago

I now remember why my original patch didn't include unit tests: there were no unit tests for wp_get_audio_extensions() and wp_get_video_extensions(), so I figured they weren't needed for the new wp_get_image_extensions().

41801.2.diff adds unit tests for all 3 wp_get_xxx_extensions() funcs.

It also adds a call to wp_get_image_extensions() in populate_network() to go along with the existing calls to wp_get_audio_extensions() and wp_get_video_extensions() there.

#7 @pbiron
7 years ago

  • Keywords needs-unit-tests removed

#8 in reply to: ↑ 6 ; follow-up: @pbiron
7 years ago

Replying to pbiron:

It also adds a call to wp_get_image_extensions() in populate_network() to go along with the existing calls to wp_get_audio_extensions() and wp_get_video_extensions() there.

The following question probably deserves a ticket of its own but I'll ask it here: why does populate_network() add a number of audio and video extensions to upload_filetypes that aren't included in the default wp_get_[audio|video]_extensions() return values?

This ticket was mentioned in Slack in #core-media by pbiron. View the logs.


6 years ago

#10 in reply to: ↑ 8 @pbiron
6 years ago

Replying to pbiron:

The following question probably deserves a ticket of its own but I'll ask it here: why does populate_network() add a number of audio and video extensions to upload_filetypes that aren't included in the default wp_get_[audio|video]_extensions() return values?

per the discussion at https://wordpress.slack.com/archives/C02SX62S6/p1536269609000100, I'll refresh the patch when I get the time.

Last edited 6 years ago by pbiron (previous) (diff)

#11 follow-up: @birgire
6 years ago

I noticed a typo in 41801.2.diff in wp_attachment_is(), where it adds a wp_get_audio_extensions() instead of wp_get_image_extensions().

Additionally I wonder if wp_get_image_extensions() should be used in translate_smiley():

https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/formatting.php#L2802

I'm not so sure about WP_Theme::get_screenshot() though:

https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/class-wp-theme.php#L962

#12 in reply to: ↑ 11 @pbiron
6 years ago

Replying to birgire:

I noticed a typo in 41801.2.diff in wp_attachment_is(), where it adds a wp_get_audio_extensions() instead of wp_get_image_extensions().

D'oh!

Additionally I wonder if wp_get_image_extensions() should be used in translate_smiley():

https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/formatting.php#L2802

I'm not so sure about WP_Theme::get_screenshot() though:

https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/class-wp-theme.php#L962

during yesterday's media chat I took a todo to look for other places in core that hardcode various media extensions and consider whether those should be replaced by calls to wp_get_[audio|image|video]_extensions. thanx for these suggestions, I will add them to the list.

Once I get what I think is a complete list I'll add it here and then we can discuss which ones should be replace and which should stay hardcoded.

This ticket was mentioned in Slack in #core-multisite by pbiron. View the logs.


6 years ago

#14 @pento
6 years ago

  • Milestone changed from 5.0 to 5.1

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


6 years ago

#16 @kirasong
6 years ago

#39821 was marked as a duplicate.

@pbiron
6 years ago

#17 @pbiron
6 years ago

41801.3.diff contains the following changes from 41801.2.diff

There are still 2 places in core that have hardcoded lists of image extensions, that I think should remain so:

  • WP_Theme::get_screenshot()
    • the extensions for supported formats for theme screenshots. Don't think it is a good idea to open this up to "arbitrary" image formats
  • wp_get_ext_types()

Note: I'm currently having problems running unit tests locally (50+ tests are failing even before applying this patch...although no additional tests fail after it is applied), so it should be checked carefully before committing.

Hopefully this can land in 5.1.0 beta.

This ticket was mentioned in Slack in #core-media by pbiron. View the logs.


6 years ago

#19 follow-up: @kirasong
6 years ago

  • Keywords dev-feedback added

Thanks for updating the patch @pbiron!

I noticed that in the patch, in src/wp-includes/customize/class-wp-customize-media-control.php:89, it looks like bmp was in the extension list before the patch and isn't anymore after the patch. Is that intentional, and if so, could you please walk me through the reasoning?

#20 in reply to: ↑ 19 @pbiron
6 years ago

Replying to mikeschroder:

I noticed that in the patch, in src/wp-includes/customize/class-wp-customize-media-control.php:89, it looks like bmp was in the extension list before the patch and isn't anymore after the patch. Is that intentional, and if so, could you please walk me through the reasoning?

No, not intentional. I hadn't noticed that that particular extension list included something that wasn't in the default list...thanx for catching that. Looking over the rest of the patch, that is the only list that includes something not in the default list. Note, however, that the default list does include 'jpe', which is not in the original lists in a couple of other places.

BTW: the "default" list comes from what was previously in the switch/case statement in src/wp-includes/post.php.

Which raises 2 questions:

  • why isn't 'bmp' in the default list?
  • why is 'bmp' included in that one (and no where else)?

I don't know the answer to either of those questions.

If there is good reason for 'bmp' not to be included in the default list and good reason for it to be included in that particular list, then I think the best thing to do is to bracket that line with add_filter( 'wp_image_extensions', ... ) and remove_filter( 'wp_image_extensions', ... ) with a function that will add 'bmp' to that list.

If 'bmp' can/should be added to the default list, then I'll update the hooked func in the unit test to add some other image extension.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 years ago

#22 @pento
6 years ago

I'm inclined to think that bmp not being in the list is an oversight, rather than intentional. BMP is generally supported in major browsers, so I'm fine with it being in the default list.

#23 @pbiron
6 years ago

I was going to update the patch per our discussion yesterday on Slack, however there is another "gotcha" with using the list(s) in wp_get_ext_types() as the "source of truth": that function applies the ext2type filter to those lists before returning. Hence, unless we temporarily unhook ext2type we're not guaranteed to get those lists as the defaults.

And temporarily unhooking (and later rehooking) every callback for that hook is nasty business. I've done the following in plugins before, but I'm reluctant to do it in core (would set a bad precedent):

global $wp_filter;

$hooked = null;
if ( isset( $wp_filter['ext2type'] ) ) {
    $hooked = $wp_filter['ext2type'];
    unset( $wp_filter['ext2type'] );
}

$ext_types = wp_get_ext_types();

if ( $hooked ) {
    $wp_filter['ext2type'] = $hooked;
}

...

I think the right thing to do is the following:

  1. move the image, audio and video lists from wp_get_ext_types() into wp_get_[image|audio|video]_extensions()
  2. modify wp_get_ext_types() as follows:
    return apply_filters(
        'ext2type',
        array(
            'image' => wp_get_image_extensions(),
            'audio' => wp_get_audio_extensions(),
            'video' => wp_get_video_extensions(),
            ...
        )
    );
    

Given how close we are to 5.1beta (tomorrow), I think we should punt this to 5.2 to give us time to consider all the options.

Last edited 6 years ago by pbiron (previous) (diff)

This ticket was mentioned in Slack in #core-media by pbiron. View the logs.


6 years ago

#25 @joemcgill
6 years ago

  • Milestone changed from 5.1 to Future Release

Thanks @pbiron. Going to move it to a future release.

#26 @gmariani405
5 years ago

Hello,

I'd like to add to this ticket a suggestion on improving the functionality here. I have a use-case where I'm trying to display a set of posts using a custom post type in Elementor. When Elementor goes to render and the option to display the featured image is selected, it calls wp_attachment_is_image() on the individual post which in turn calls wp_attachment_is('image', $post).

That is all fine and dandy, except that if the attachment is a PDF this will fail. Which shouldn't happen (if ImageMagick is installed), because as of 4.7, WordPress creates a thumbnail for PDFs. So technically it does have a valid image that could be displayed.

In order to get this to work, we would need something like what @pbiron has added, a filter to add the 'pdf' extension (or include it in core). But we also need it to allow the post_mime_type to allow 'application/pdf' (or the like). Because right now it fails at this part in WordPress 5.4.2:

<?php
if ('import' !== $post->post_mime_type) {
    return $type === $ext;
}

testing with something like this and adding 'pdf' to the list of allowed extensions lets it to work as expected:

<?php
if ('import' !== $post->post_mime_type && 'application/pdf' !== $post->post_mime_type) {
    return $type === $ext;
}

Now, not to say that should be the solution, you guys know how WordPress functions (and should function) better than I. But if something like this could be customizable or added to core in some fashion that would be great.

Note: See TracTickets for help on using tickets.