Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#52603 closed defect (bug) (fixed)

Inconsistent doc for return value of wp_generate_attachment_metadata

Reported by: chouby's profile Chouby Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.8 Priority: normal
Severity: normal Version: 2.5
Component: Media Keywords: has-patch commit
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 (3)

52603.diff (614 bytes) - added by Chouby 4 years ago.
52603.2.diff (1.2 KB) - added by SergeyBiryukov 4 years ago.
52603.3.diff (865 bytes) - added by hellofromTonya 4 years ago.
Reformats multi-line comment to use /* instead of per coding standards.

Download all attachments as: .zip

Change History (15)

@Chouby
4 years ago

#1 @Chouby
4 years ago

  • Keywords has-patch added

#2 @SergeyBiryukov
4 years 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.

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


4 years ago

#4 @desrosj
4 years ago

This is a documentation only change, and can be made during the beta part of the release cycle, so going to leave this one in the milestone, even though today is beta 1.

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


4 years ago

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


4 years ago

#7 @JeffPaul
4 years ago

  • Keywords needs-refresh added

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


4 years ago

#9 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 51162:

Media: Make sure wp_generate_attachment_metadata() always returns an array.

This matches the documentation for the filter of the same name.

Previously, the function could return false for an audio or video attachment that does not exist in the local filesystem.

Props Chouby, SergeyBiryukov.
Fixes #52603.

@hellofromTonya
4 years ago

Reformats multi-line comment to use /* instead of per coding standards.

#10 follow-up: @hellofromTonya
4 years ago

  • Keywords commit added; needs-refresh removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for a small coding standards follow-up to [51162].

Patch 52603.3.diff reformats the multi-line comment to use /* instead of //.

Marking the patch for commit cc @SergeyBiryukov

#11 in reply to: ↑ 10 @SergeyBiryukov
4 years ago

Replying to hellofromTonya:

Reopening for a small coding standards follow-up to [51162].

Patch 52603.3.diff reformats the multi-line comment to use /* instead of //.

Thanks for the patch!

As previously noted in comment:1:ticket:53004 and comment:9:ticket:52025, there are many other instances in core of the single-line comment style being used for multi-line comments.

There is a section in the documentation standards that clarifies which format is generally recommended, but that is just a recommendation and not a strict rule.

This was previously corrected for most of inline comments in core in [47122]. I think I opted to keep comments with one or two lines in single line format, and switch comments with three or more lines to the multi-line format.

At this time, until we have a WPCS rule that forbids using this comment style for multi-line comments, I think it makes sense to keep using single line format for comments with one or two lines.

That said, I have no objections converting this comment to three lines :)

#12 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 51166:

Docs: Update syntax for multi-line comment in wp_generate_attachment_metadata() per the documentation standards.

Follow-up to [23766], [25968], [35554], [51162].

Props hellofromTonya.
Fixes #52603.

Note: See TracTickets for help on using tickets.