Opened 7 years ago
Last modified 5 years ago
#41801 assigned enhancement
add wp_get_image_extensions() and wp_image_extensions filter
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (29)
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:
↓ 5
@
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
@
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.
@
7 years ago
revised patch: add unit tests, add call to wp_get_image_extensions() in populate_network()
#6
follow-up:
↓ 8
@
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.
#8
in reply to:
↑ 6
;
follow-up:
↓ 10
@
7 years ago
Replying to pbiron:
It also adds a call to
wp_get_image_extensions()
inpopulate_network()
to go along with the existing calls towp_get_audio_extensions()
andwp_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
@
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 toupload_filetypes
that aren't included in the defaultwp_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.
#11
follow-up:
↓ 12
@
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
@
6 years ago
Replying to birgire:
I noticed a typo in 41801.2.diff in
wp_attachment_is()
, where it adds awp_get_audio_extensions()
instead ofwp_get_image_extensions()
.
D'oh!
Additionally I wonder if
wp_get_image_extensions()
should be used intranslate_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
This ticket was mentioned in Slack in #core-media by desrosj. View the logs.
6 years ago
#17
@
6 years ago
41801.3.diff contains the following changes from 41801.2.diff
- updates
@since
tag to 5.1.0 (inwp_get_image_extensions()
) - fixes the typo mentioned in https://core.trac.wordpress.org/ticket/41801#comment:11
- removes the hardcoded image extensions from
$misc_ext
in `populate_network_meta()- note: previous patch and comments on this ticket had those changes in
populate_network()
, but a subsequent commit moved that code topopulate_network_meta()
- note: previous patch and comments on this ticket had those changes in
- replaces additional hardcoded array of extensions with call to
wp_get_image_extensions()
in the following:translate_smiley()
check_upload_mimes()
WP_Customize_Media_Control::to_json()
- in
wp-admin/network/settings.php
- to go along with the change in
check_upload_mimes()
- to go along with the change in
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:
↓ 20
@
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
@
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 likebmp
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
@
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
@
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:
- move the
image
,audio
andvideo
lists fromwp_get_ext_types()
intowp_get_[image|audio|video]_extensions()
- 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.
This ticket was mentioned in Slack in #core-media by pbiron. View the logs.
6 years ago
#25
@
6 years ago
- Milestone changed from 5.1 to Future Release
Thanks @pbiron. Going to move it to a future release.
#26
@
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.
In addition to adding
wp_get_image_extensions()
andwp_image_extensions
filter, the patch also modifies wp_attachment_is() to usewp_get_image_extensions()
.