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

[azaozz]

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.

[azaozz]

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.

[adamsilverstein]

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.

[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/.

[adamsilverstein]

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?

[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' ) );

[adamsilverstein]

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.

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

[adamsilverstein]

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.

[azaozz]

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.

[adamsilverstein]

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

[ericlewis]

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]

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

[nacin]

In 24057:

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

props adamsilverstein, ericlewis.
fixes #23255.