WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 9 months ago

#41801 assigned enhancement

add wp_get_image_extensions() and wp_image_extensions filter

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

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 2 years ago.
41801.2.diff (4.6 KB) - added by pbiron 14 months 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 10 months ago.

Download all attachments as: .zip

Change History (28)

@pbiron
2 years ago

#1 @pbiron
2 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.


15 months ago

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


14 months ago

#4 follow-up: @joemcgill
14 months 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
14 months 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
14 months ago

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

#6 follow-up: @pbiron
14 months 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
14 months ago

  • Keywords needs-unit-tests removed

#8 in reply to: ↑ 6 ; follow-up: @pbiron
14 months 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.


14 months ago

#10 in reply to: ↑ 8 @pbiron
14 months 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 14 months ago by pbiron (previous) (diff)

#11 follow-up: @birgire
14 months 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
14 months 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.


13 months ago

#14 @pento
12 months ago

  • Milestone changed from 5.0 to 5.1

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


10 months ago

#16 @mikeschroder
10 months ago

#39821 was marked as a duplicate.

@pbiron
10 months ago

#17 @pbiron
10 months 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.


10 months ago

#19 follow-up: @mikeschroder
10 months 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
10 months 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.


9 months ago

#22 @pento
9 months 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
9 months 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 9 months ago by pbiron (previous) (diff)

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


9 months ago

#25 @joemcgill
9 months ago

  • Milestone changed from 5.1 to Future Release

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

Note: See TracTickets for help on using tickets.