WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 2 months ago

#52603 reviewing defect (bug)

Inconsistent doc for return value of wp_generate_attachment_metadata

Reported by: Chouby Owned by: SergeyBiryukov
Milestone: 5.8 Priority: normal
Severity: normal Version: 2.5
Component: Media Keywords: has-patch
Focuses: docs Cc:

Description

The return value of wp_generate_attachment_metadata() is documented as mixed. However, the filter wp_generate_attachment_metadata documents it as array. So I would expect that the return value would be documented as array too.

Attachments (2)

52603.diff (614 bytes) - added by Chouby 2 months ago.
52603.2.diff (1.2 KB) - added by SergeyBiryukov 2 months ago.

Download all attachments as: .zip

Change History (4)

@Chouby
2 months ago

#1 @Chouby
2 months ago

  • Keywords has-patch added

#2 @SergeyBiryukov
2 months ago

  • Milestone changed from Awaiting Review to 5.8
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

Thanks for the patch!

It looks like wp_generate_attachment_metadata() can actually return false for an audio or video attachment that doesn't exist in the local filesystem, as previously noted in comment:5:ticket:25649.

Some history here:

Since that was never documented neither in the function nor in the wp_generate_attachment_metadata filter, I think we have two options here:

  • Make sure the function indeed always returns an array, as the wp_generate_attachment_metadata filter says.
  • Document that it returns false for an audio or video attachment that doesn't exist in the local filesystem.

For consistency with other functions like wp_create_image_subsizes(), I think the first option would be preferable here, see 52603.2.diff. Looking at the wp_generate_attachment_metadata() instances in core, we don't really expect it to return anything other than an array.

P.S. To complicate things a bit further, wp_read_image_metadata() does return false on failure, same as wp_read_audio_metadata() and wp_read_video_metadata(), but that's probably not super relevant here.

Note: See TracTickets for help on using tickets.