Checking if attachment is audio/video

[danielpataki]

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!

[rmccue]

Thanks for the patch!

Two small issues with it:

1. $image_exts is supposed to be called that; it stands for image extensions, not image exists
2. You appear to have accidentally duplicated the function name, I believe the second one should be wp_attachment_is_video instead.

(Apparently the filename has also disappeared from the diff, not quite sure why for that though.)

[danielpataki]

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 :)

[danielpataki]

Final version of diff to add audio and video checking to attachments in post.php

[SergeyBiryukov]

When uploading a new version of a patch, please don't overwrite the old one, so that context is preserved.

[danielpataki]

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

[DrewAPicture]

Hi Daniel,

Thanks for the patch, just a couple of suggestions:

• Since it looks like *_video() and *_audio() are essentially verbatim copies of wp_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 like wp_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 change image/ to video/ or audio/ in the type checks anyway.

• Last, in the future, please generate patches from the WordPress root :)

[danielpataki]

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

[nacin]

$post->post_mime_type?

[danielpataki]

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

[ericlewis]

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' )?

[wonderboymusic]

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.

[DrewAPicture]

@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?

[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 pdf as being useful now, since there are no instances in core that would use it.

[DrewAPicture]

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 pdf as being useful now, since there are no instances in core that would use it.

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 :-)

[wonderboymusic]

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"

[wonderboymusic]

In 31645:

Introduce a function, wp_attachment_is( $type, $post = 0 ), to collapse the logic for determining whether an attachment is an image, audio, or video.

This is admittedly a first pass. There needs to be a generic handler for when any other type is passed, but for now it accepts the whitelist.

See #25275.

[wonderboymusic]

In 31646:

After [31645], for the default case, return the result of checking the extension against the passed type.

See #25275.

[wonderboymusic]

In 31647:

Add unit tests for wp_attachment_is(), checks the whitelist and arbitrary extension.

See #25275.

[DrewAPicture]

In 31659:

Better document parameters and the return for the newly-introduced wp_attachment_is().

Also adds a changelog entry to the DocBlock for wp_attachment_is_image() to denote that it serves as a wrapper for wp_attachment_is() as of 4.2.0.

See [31645]. See #25275.

[DrewAPicture]

In 31660:

Add a missing @since 4.2.0 tag to the DocBlock for wp_attachment_is().

See #25275.

[boonebgorges]

In 31670:

Whitelist 'psd' for uploads when testing `wp_attachment_is() on multisite.

It's not allowed by default, which causes the fixture not to be built.

See #25275 [31647].

[DrewAPicture]

@wonderboymusic: What's left here?

[DrewAPicture]

Let's call this fixed. Please create new tickets for any outstanding issues.