WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 18 months ago

#41802 closed enhancement (maybelater)

add wp_attachment_is_audio() and wp_attachment_is_video()

Reported by: pbiron Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.9
Component: Media Keywords: has-patch
Focuses: Cc:

Description

Core already has wp_attachment_is_image().

For consistency and convenience, core should also have wp_attachment_is_audio() and wp_attachment_is_video().

Attachments (1)

41802.diff (10.7 KB) - added by pbiron 2 years ago.

Download all attachments as: .zip

Change History (10)

@pbiron
2 years ago

#1 @pbiron
2 years ago

  • Keywords has-patch added

In addition to adding wp_attachment_is_audio() and wp_attachment_is_video(), the patch also replaces all uses of wp_attachment_is( 'audio' ) and wp_attachment_is( 'video' ) with the new functions (except in unit tests, where the new functions are added to the relevant existing tests that use wp_attachment_is()).

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


19 months ago

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


19 months ago

#4 follow-up: @mikeschroder
19 months ago

I personally prefer the generic nature of wp_attachment_is() (it looks like this was an intentional move) to having a separate function each of many eventual file types.

Is there a particular reason it'd be helpful to have functions for these, specifically?

#5 in reply to: ↑ 4 ; follow-up: @pbiron
19 months ago

Replying to mikeschroder:

I personally prefer the generic nature of wp_attachment_is() (it looks like this was an intentional move) to having a separate function each of many eventual file types.

Yes, I think the introduction of wp_attachment_is() was intentional in 4.2.0...and wp_attachment_is_image() was changed to be a simple wrapper around that in 4.2.0.

Is there a particular reason it'd be helpful to have functions for these, specifically?

As I said, the reason is consistency...why have wp_attachment_is_image() but not these others?

I'd be fine with deprecating wp_attachment_is_image(), but if it's not going to be deprecated then I think the others should be added.

#6 in reply to: ↑ 5 @pbiron
19 months ago

Replying to pbiron:

Is there a particular reason it'd be helpful to have functions for these, specifically?

As I said, the reason is consistency...why have wp_attachment_is_image() but not these others?

I'd be fine with deprecating wp_attachment_is_image(), but if it's not going to be deprecated then I think the others should be added.

There are currently 8 uses of wp_attachment_is_image() in core...so if it does get deprecated those uses would need to be changed to wp_attachment_is( 'image' ).

#7 @SergeyBiryukov
19 months ago

Re-reading #25275, deprecating wp_attachment_is_image() was indeed suggested, but I don't think switching to a more generic function is a strong enough reason to do that.

wp_attachment_is() was introduced as an alternative to duplicating the same code in three different functions.

Since duplication is no longer a concern here, I don't have any objections to adding these wrappers (maybe without replacing the existing wp_attachment_is() usage though).

That said, introducing them means that any new formats added to wp_attachment_is() in the future (e.g. pdf) would probably need a corresponding wp_attachment_is_*() function for consistency.

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


18 months ago

#9 @joemcgill
18 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

After some discussion here, as well as the previous discussion in #25275 and in Slack, the general consensus seems to be that there isn't currently a compelling reason to add these functions at this time. The desire to create consistency across the codebase with wp_attachment_is_image() is reasonable, but the burden required to maintain this consistency by adding new functions for each supported attachment type over time, along with the very likely potential of these wrapping functions diverging over time, is more persuasive at this time. However, if our thinking shifts in the future, 41802.diff is a model for how we would want to execute this.

Closing for now as maybelater.

Any related discussion about deprecating wp_attachment_is_image() should happen in a separate ticket.

Note: See TracTickets for help on using tickets.