Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 6 years ago

#25275 closed enhancement (fixed)

Checking if attachment is audio/video

Reported by: danielpataki's profile 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)

post.diff (1.5 KB) - added by danielpataki 11 years ago.
Final version of diff to add audio and video checking to attachments in post.php
25275.diff (7.1 KB) - added by wonderboymusic 10 years ago.
25275.2.diff (8.1 KB) - added by wonderboymusic 10 years ago.

Download all attachments as: .zip

Change History (29)

#1 @rmccue
11 years ago

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 they should be wp_attachment_is_audio and wp_attachment_is_video instead.

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

Last edited 11 years ago by rmccue (previous) (diff)

#2 @danielpataki
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 :)

@danielpataki
11 years ago

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

#3 @SergeyBiryukov
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 @danielpataki
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 @DrewAPicture
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 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 :)

#6 @DrewAPicture
11 years ago

  • Cc xoodrew@… added

#7 @danielpataki
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

#8 @nacin
11 years ago

$post->post_mime_type?

#9 @danielpataki
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 @ericlewis
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 @wonderboymusic
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 @DrewAPicture
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: @wonderboymusic
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 @DrewAPicture
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 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 :-)

#15 @wonderboymusic
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"

#16 @wonderboymusic
10 years ago

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.

#17 @wonderboymusic
10 years ago

In 31646:

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

See #25275.

#18 @wonderboymusic
10 years ago

In 31647:

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

See #25275.

This ticket was mentioned in Slack in #core by drew. View the logs.


10 years ago

#20 @DrewAPicture
10 years ago

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.

#21 @DrewAPicture
10 years ago

In 31660:

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

See #25275.

#22 @boonebgorges
10 years ago

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].

This ticket was mentioned in Slack in #core by drew. View the logs.


10 years ago

#24 @DrewAPicture
10 years ago

  • Keywords 4.2-beta added

@wonderboymusic: What's left here?

#25 @DrewAPicture
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.

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


6 years ago

Note: See TracTickets for help on using tickets.