#25275 closed enhancement (fixed)
Checking if attachment is audio/video
Reported by: | danielpataki | Owned by: | |
---|---|---|---|
Milestone: | 4.2 | Priority: | normal |
Severity: | normal | Version: | 3.6 |
Component: | Media | Keywords: | has-patch needs-testing needs-unit-tests 4.2-beta |
Focuses: | Cc: |
Description
Hi Everyone!
This is my first time attaching a diff, if you have a moment, please let me know if I am doing it wrong,
I am building a theme just now which needs to check if an attachment is audio. It seems that only image format functions are included in post.php (wp_attachment_is_image()) so I quickly created one for audio and video.
I am not a huge expert on formats, so if you have any more to add, please let me know!
Attachments (3)
Change History (29)
#2
@
11 years ago
Sorry about that, I only made 10 errors in 20 lines of code, I must be nervous to add a diff :D I had a detailed look at the changes, all should be well now.
Is there anything I should do differently when contributing to make peoples' lives easier? (apart from not leaving errors in there)?
Thanks :)
#3
@
11 years ago
- Component changed from General to Media
- Keywords has-patch added; attachments removed
- Summary changed from Checking if attachment is adio/video to Checking if attachment is audio/video
When uploading a new version of a patch, please don't overwrite the old one, so that context is preserved.
#4
@
11 years ago
Hi Sergey,
I normally would not have done that, the only reason I overwrote was because the patch had errors like an extra comma after an array and so on. I will definitely not overwrite at all from now on!
Daniel
#5
@
11 years ago
Hi Daniel,
Thanks for the patch, just a couple of suggestions:
- Since it looks like
*_video()
and*_audio()
are essentially verbatim copies ofwp_attachment_is_image()
with some small changes, it might make more sense to deprecate*_image()
and combine all three into a single function with a type flag, maybe something likewp_attachment_is_type()
.
- Also, we could probably just use
wp_ext2type()
to determine the extension type because it opens the door for future types to be added to a*_type()
function. It looks like you forgot to changeimage/
tovideo/
oraudio/
in the type checks anyway.
- Last, in the future, please generate patches from the WordPress root :)
#7
@
11 years ago
Agreed on all counts! I will modify the code to reflect al that.
I'll add a wp_attachment_is_type() function which uses wp_ext2type() internally.
Sorry for being a bit of a dunce but I'm completely new to making patches. When you say "generate patches from the WordPress root", does this mean to diff the whole directory? I basically used: diff -u post.php post-mine.php > post.diff to create the diff file. Would the method to generate from the root be to have a vanilla copy of wordpress and another one where I make changes side by side and then just use this: diff -u wordpress wordpress-mine > wordpress-diff.
Also, since wp_ext2type is only in alpha, do you recommend always creating patches from an SVN checkout of the trunk?
Sorry for all the questions!
Daniel
#9
@
11 years ago
Hi nacin,
I did think about using the mime type but I think a more general-purpose function might be a more reliable and more flexible solution. The mime type would work pretty well for audio and video, but wouldn't be too useful for documents, archive files and so on. I addition some files like markdown files don't have an official mime type.
By using the filter in wp_ext2type() users could customize the thing completely.
Let me know if I am completely unaware of how mime types work, I am not very familiar with them :(
Daniel
#10
@
11 years ago
- Keywords needs-patch added; has-patch removed
There's plenty of code duplication here, and an open argument for wp_attachment_is_pdf()
and on and on. How about wp_attachment_is( $attachment_id, 'type' )
?
#11
@
10 years ago
- Keywords has-patch needs-testing needs-unit-tests added; needs-patch removed
- Milestone changed from Awaiting Review to 4.2
25275.diff is more comprehensive. wp_attachment_is_image()
is only called twice, and one of them is deprecated. wp_attachment_is_audio()
would be called 8 times and unify the approach.
#12
@
10 years ago
@wonderboymusic: What do you think about my suggestion in comment:5 to deprecate wp_attachment_is_image()
and create a generic wp_attachment_is_type()
function instead of creating two new functions that do exactly the same thing for different mime types?
#13
follow-up:
↓ 14
@
10 years ago
@DrewAPicture - the internals need different functions to supply the list of accepted mime types - that new func might be useful for other types, but I don't see pdf
as being useful now, since there are no instances in core that would use it.
#14
in reply to:
↑ 13
@
10 years ago
Replying to wonderboymusic:
@DrewAPicture - the internals need different functions to supply the list of accepted mime types - that new func might be useful for other types, but I don't see
Regardless, as the proposed functions are essentially 97 percent the same, I'd still argue for a single function with a switch in it, even if we only cover the audio and video cases initially. It's the DRY-ght thing to do :-)
#15
@
10 years ago
25275.2.diff is a DRY™ update. It has a switch
for audio|video|image
but still isn't generic, since it has to know how to check each "alias"
This ticket was mentioned in Slack in #core by drew. View the logs.
10 years ago
This ticket was mentioned in Slack in #core by drew. View the logs.
10 years ago
#25
@
10 years ago
- Resolution set to fixed
- Status changed from new to closed
Let's call this fixed. Please create new tickets for any outstanding issues.
Thanks for the patch!
Two small issues with it:
$image_exts
is supposed to be called that; it stands forimage extensions
, notimage exists
wp_attachment_is_audio
andwp_attachment_is_video
instead.(Apparently the filename has also disappeared from the diff, not quite sure why for that though.)