Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#23255 closed defect (bug) (fixed)

get_post_format() can throw PHP notice and doesn't return WP_Error

Reported by: azaozz's profile azaozz Owned by: nacin's profile nacin
Milestone: 3.6 Priority: normal
Severity: normal Version:
Component: Warnings/Notices Keywords: has-patch commit
Focuses: Cc:

Description

The function get_post_format() part of the post formats API throws a PHP notice when called with non-existing post_ID. It also doesn't return WP_Error although most uses in core expect that.

Attachments (4)

get-post-format.diff (707 bytes) - added by adamsilverstein 11 years ago.
get-post-format.2.diff (742 bytes) - added by adamsilverstein 11 years ago.
23255.diff (617 bytes) - added by ericlewis 11 years ago.
23255.1.diff (749 bytes) - added by ericlewis 11 years ago.
Forgot to include the empty( $post ) check in 23255.diff

Download all attachments as: .zip

Change History (19)

#1 @azaozz
11 years ago

For consistency's sake we should add a check whether $post exists and return WP_Error, however that might break back-compat in plugins that expect either false or a string and don't follow core's example.

#2 @adamsilverstein
11 years ago

  • Cc adamsilverstein@… added

#3 follow-up: @adamsilverstein
11 years ago

this patch adds the WP_Error return and updates the phpdoc. also i added some missing space to post-template.php, but now that i am looking at that file i see lots of other spots where coding practice spaces are missing. surprised to see that, but maybe doesn't belong in this ticket... going to fix and submit separately.

#4 in reply to: ↑ 3 ; follow-up: @SergeyBiryukov
11 years ago

Replying to adamsilverstein:

i see lots of other spots where coding practice spaces are missing. surprised to see that, but maybe doesn't belong in this ticket... going to fix and submit separately.

Note that formatting cleanup tickets are discouraged. Coding style fixes should be a result of more substantial changes: http://wpdevel.wordpress.com/2011/03/23/code-refactoring/.

#5 in reply to: ↑ 4 ; follow-up: @adamsilverstein
11 years ago

  • Keywords has-patch added

Replying to SergeyBiryukov:

Replying to adamsilverstein:

i see lots of other spots where coding practice spaces are missing. surprised to see that, but maybe doesn't belong in this ticket... going to fix and submit separately.

Note that formatting cleanup tickets are discouraged. Coding style fixes should be a result of more substantial changes: http://wpdevel.wordpress.com/2011/03/23/code-refactoring/.

thanks for pointing that and saving me the effort. i was surprised to see the non-standard coding, but realized it was older code.

still, i found it a useful exercise in self-training to go thru the code for that single file (post-template.php) and correct mistakes. i'm trying to train myself to see the proper coding style by default (i've been lazy too long), and this was good practice, i even had to go back to the coding style document a few times to check my assumptions, so i definitely learned something.

i'll keep the diff for myself, but won't bother submitting as a ticket.

any other comments on the changes in this ticket, and whether adding the missing WP_Error return is worthwhile, is it there for some future purpose, consistency, or is it just an extra check for a condition that will never occur?

#6 in reply to: ↑ 5 ; follow-up: @SergeyBiryukov
11 years ago

Replying to adamsilverstein:

i'm trying to train myself to see the proper coding style by default

Nitpicking: get-post-format.diff still misses some spaces. :) This would look better:

	if ( empty( $post ) )
		return new WP_Error( 'invalid_post', __( 'Invalid post' ) );

#7 in reply to: ↑ 6 @adamsilverstein
11 years ago

Replying to SergeyBiryukov:

Replying to adamsilverstein:

i'm trying to train myself to see the proper coding style by default

Nitpicking: get-post-format.diff still misses some spaces. :) This would look better:

	if ( empty( $post ) )
		return new WP_Error( 'invalid_post', __( 'Invalid post' ) );

THANK YOU for your attention! i corrected that and one other spot where space missing; i promise i'm getting the hang of this, but always appreciate your corrections... aka nitpicking.

#8 follow-up: @nacin
11 years ago

The WP_Error bit appears to be a documentation issue. This function already has multiple error conditions that return false, and none that return WP_Error. By suddenly adding return values of WP_Error that already had scalar return values, we risk fatal errors during usage. Overall, not a good idea.

#9 @SergeyBiryukov
11 years ago

  • Component changed from General to Warnings/Notices

#10 in reply to: ↑ 8 @adamsilverstein
11 years ago

Replying to nacin:

The WP_Error bit appears to be a documentation issue. This function already has multiple error conditions that return false, and none that return WP_Error. By suddenly adding return values of WP_Error that already had scalar return values, we risk fatal errors during usage. Overall, not a good idea.

we got the not good idea addressing #23198, looking at set_post_format() which makes the same call $post = get_post($post); then immediately checks for empty($post). if you don't think it needs changing thats fine with me.

#11 @azaozz
11 years ago

Agree with @nacin.

The discussion started in http://core.trac.wordpress.org/ticket/23198#comment:10 as this was discovered there. Initially get_post_format() was returning WP_Error coming from the lower level API [15777]. Later on we improved it to use better low level API that wasn't returning WP_Error any more [16319]. So for over two years this function has been returning either false or a string.

#12 follow-up: @adamsilverstein
11 years ago

  • Keywords dev-feedback added

this ticket is languishing. can someone wise and decisive commit what we did or mark as won't fix and close? thanks!

@ericlewis
11 years ago

#13 in reply to: ↑ 12 @ericlewis
11 years ago

In 23255.diff, in response to the comments by nacin and azaozz, leave the return values from get_the_format() as is, modify inline docs accordingly.

@ericlewis
11 years ago

Forgot to include the empty( $post ) check in 23255.diff

#14 @SergeyBiryukov
11 years ago

  • Keywords commit added; dev-feedback removed

#15 @nacin
11 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 24057:

Return false in get_post_format() if the post does not exist.

props adamsilverstein, ericlewis.
fixes #23255.

Note: See TracTickets for help on using tickets.