Make WordPress Core

Opened 9 years ago

Closed 4 years ago

Last modified 4 years ago

#33068 closed defect (bug) (fixed)

get_post() returning current object if 0

Reported by: rahe's profile Rahe Owned by: audrasjb's profile audrasjb
Milestone: 5.7 Priority: normal
Severity: normal Version: 3.5
Component: Posts, Post Types Keywords: has-patch commit
Focuses: docs Cc:

Description

When calling get_post(0) and the global object post is set, the current object is returned.
Like in this example :

global $post;
$post = get_post(384);
var_dump( get_post(0));

The get_post returns the 384 object, and not null as expected when needing the object 0, that doesn't exists.
I propose this patch for stricly checking this is not 0 for this case and not just the handy empty.

Attachments (5)

patch.diff (548 bytes) - added by Rahe 9 years ago.
The original diff file
33068.patch (466 bytes) - added by juliobox 9 years ago.
! is_null instead of empty()
33068-2.patch (1.1 KB) - added by Rahe 9 years ago.
33068.3.diff (967 bytes) - added by audrasjb 4 years ago.
Clarify Docs
33068.diff (1000 bytes) - added by peterwilsoncc 4 years ago.

Download all attachments as: .zip

Change History (24)

@Rahe
9 years ago

The original diff file

#1 @juliobox
9 years ago

  • Version changed from 4.2.2 to 3.5

Good catch,

A little of history:
WP 1.5.1, the get_post() function is added. The first parameter was (and is) $post. But at this time you can avoid it, it's required, no default value.
So if you try to use it with an empty string, a false, null or 0, the global var will be used if set.
WP 3.5, the first parameter gets its default value null. BUT we kept to test this with is_empty.

Since the $post parameter is set to null by default, i prefer to test this to be different.
Also, i already found that some plugin were creating a post with the ID 0, ok, it's not a good idea but now we know that this can break that. See what i mean.

Thanks anyway!

@juliobox
9 years ago

! is_null instead of empty()

#2 @Rahe
9 years ago

So if I call get_post() with a real WP_Post object on a singular page, my object is replaced by the current global.

#3 @juliobox
9 years ago

My bad!
Note: Don't try to patch at midnight again.

@Rahe
9 years ago

#4 @Rahe
9 years ago

I've added a patch file with the coding standards respected in the function.

Maybe we can replace the line :

	if ( ! $_post ) {
		return null;
	}

by :

	if ( ! $_post ) {
		return;
	}

#7 @Rahe
5 years ago

Happy to see my issue opened again, I hope this will be merged :)

#9 @Rahe
4 years ago

Updated the patch with PR from github.

#10 @audrasjb
4 years ago

  • Keywords needs-testing added
  • Milestone set to 5.7

Moving for 5.7 consideration

#11 @johnbillion
4 years ago

  • Keywords 2nd-opinion added

This is a risky change. There's definitely code out there that passes integer 0 and expects to get the current post. Changing this would break that code. WPGraphQL does it explicitly, there's bound to be many more instances of it implicitly or where the value comes from a variable.

If your code does not check for emptiness before passing a value to get_post() then it will not work as expected regardless of whether that value is integer 0 or anything else empty. Maybe the unexpectedly empty value is null instead of 0, but if it's not being checked then you can't be sure that get_post() will return what you expect.

Ideally get_post() would be strict about its accepted values but that ship sailed fifteen years ago.

Personally this seems like a wontfix for backwards compatibility concerns.

#12 @Rahe
4 years ago

Hello,

For me passing an integer is wanting to get this specific ID.
Post 0 doesn't exists and maybe post 56743 too.

Based on this behaviour, get_post on 56743 should return current post too.
The current behaviour for 0 is not logic for me.

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


4 years ago

#14 @peterwilsoncc
4 years ago

I agree with @johnbillion that this is a wontfix due to backward-compatibility concerns.

The documentation could be improved to indicate:

  • A falsey value returns the current global post inside the loop
  • A numerically valid post ID that points to a non-existent post returns null.

@Rahe You're right be the behavior of an invalid ID is a little inconsistent (0 returns the global, a negative number or float returns null). For valid IDs it's consistent but the return types differ.

#15 @hellofromTonya
4 years ago

  • Focuses docs added
  • Keywords needs-patch added; needs-testing 2nd-opinion removed

Changing the focus of this ticket to docs. The proposed code changes are a wontfix. But we agree to improve the function's DocBlock to be more clear.

@audrasjb
4 years ago

Clarify Docs

#16 @audrasjb
4 years ago

  • Keywords has-patch added; needs-patch removed

Thanks @peterwilsoncc, that makes sense to me. I tried to clarify the Documentation in 33068.3.diff but I'm not so sure about the ideal wording.

#17 @audrasjb
4 years ago

  • Owner set to audrasjb
  • Status changed from new to accepted

@peterwilsoncc
4 years ago

#18 @peterwilsoncc
4 years ago

@audrasjb That reads well to me, in 33068.diff I've made a minor edit:

  • Moved default value to end of parameter definition, as that seems to be standard
  • Added a note about all falsey values returning the current global post, get_post( [] ) for example.

#19 @johnbillion
4 years ago

  • Keywords commit added

#20 @johnbillion
4 years ago

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

In 50266:

Posts, Post Types: Clarify the documentation about the return value of get_post() when a falsey value is passed.

Props Rahe, juliobox, peterwilsoncc, hellofromTonya, audrasjb

Fixes #33068

hellofromtonya commented on PR #277:


4 years ago
#21

Was deemed too risky for backwards compatibility and marked as wontfix. Closing the PR.

Note: See TracTickets for help on using tickets.