get_page() breaking $post when it's called with empty parameter

[shidouhikari]

( get_page() and get_post() are both in \wp-includes\post.php, in 3.0 RC1 they are in lines 2866 and 356 )

As we know, global $post should always be an object, but in a rare situation it gets broken and stores a post ID.

That happens when the funtion get_page() is called and its first parameter, &$page, has a value that makes empty($page) true. When that happens and $GLOBALS[ 'post'] was correcly set earlier and has a valid $GLOBALS[ 'post']->ID, the function get_page() calls get_post(), so that it returns the page data formatted according to $output and $filter parameters.

The problem is that get_post() receives &$post as reference, reference to the global variable in this case.

In line 370, if $post is an object, it's overridden to its ID, because it's the ID that will be required later. After that the variable $post is never touched again, and get_post() returns leaving $GLOBALS[ 'post'] set as an int refering to the post ID, and not the post object. get_page() also returns doing nothing about it.

To replicate the bug, put the following code anywhere that will run when is_singular() is true and $post has already been set (probably by The Loop), regardless if it's a post or page. I didn't test with custom types.

<?php

global $post;
$null = null;

echo '<p>$post begin (should be an object of type stdClass):</p>';
var_dump($post);

$post_id = $post->ID;
echo '<p>Saving post ID into $post_id:</p>';
var_dump($post_id);

$getPost = get_post($null);
echo '<p>get_post(null):</p>';
var_dump($getPost);
echo '<p>$post after get_post(null) (should be of type stdClass):</p>';
var_dump($post);


$getPage = get_page($null);
echo '<p>get_page(null):</p>';
var_dump($getPage);
echo '<p>$post after get_page(null) (should be of type stdClass, but now it\' Integer):</p>';
var_dump($post);

?>

[westi]

Working on this

[ryan]

get_page() should just call get_post() and not do anything else.

[shidouhikari]

I tested it here and confirm the patch worked :)

[westi]

We should also fix get_post through the code hilighted does stop on the variable passed in shouldn't get stomped like that in one path through the code - in fact does it even need to be a reference?

[shidouhikari]

I believe the desired behavior is that passed $post be updated by the function to reflect the post. Consumer of the function should be aware the variable is being passed as reference and is possible to be changed. If he doesn't want it, he must create a secure variable to pass, but in this case we have lower memory efficiency.

Indeed, the function returns an object of the required post, so there wouldn't be the need of editing the parameter. Maybe it's this way because $output and $filter parameters makes the returning value have an altered format (note that the code that uses them doesn't touch $post anymore, and returns $_post, a different variable). So, the parameter has the object and returning value has the formatted data.

If you wanna get the post with a different format and also the raw object, the alternative would be call the function twice... So the function offers you 2 outputs. We MUST be aware that when get_post() returns, $post may have changed (but not always...).

The problem here is that when get_page() is called and $post is empty, it tries to use the global variable to identify current post, and aparrently only calls get_post() so that it can be formatted as required. But when the global variable is sent as parameter, the post object is replaced by its ID, which shouldn't happen with global $post.

It seems get_page()'s author just wanted to use get_post() to format the returning variable, and didn't note that get_post() must receive a variable that's secure to be edited.

get_post() isn't intended to format an existing object, if get_page() remains as it is, it should duplicate $post and leave global variable alone, or we should have a separate function just to format the object, which should be called by both get_post() and get_page() when they find the object they want, just to format the output. I believe get_page() is wrongly using get_post().

get_post() wasn't developed intended to receive an object and just format it. When it receives an object, it believes the object can be freely edited, so the variable is assigned to the received object's ID and tries to retrieve a valid post object from cache or database. *Indeed, anybody that gets global $post and later pass it to get_post() to have it formatted will break the global variable... that's bug prone and should be fixed!*

Note also that, even receiving a post object, get_post() doesn't believe it's a valid object, *deletes* its content and try to get a new post based on the object's ID. I believe it was intended to update that post data, or maybe just doesn't consider it's content valid since the function doesn't know where it came from...

Another interesting thing is that if $post is an object and empty($post->filter), it's sanitized and added to cache... I don't understand the objective in this case, why $post->filter being empty makes the object so reliable that it's added to cache without any verification, while when it's not empty the object's content is deleted and re-retrieved?

Also, the paramater content is totally inconsistent when get_post() returns, so inconsistent we almost can't trust in it:

• if it's empty, it's left empty

• if it's an object and empty($post->filter), it's content is sanitized and cached, sometimes being considered as that posts's official content (maybe it was intended for a plugin hack into specific posts? I doubt, because for that it would be better to use filters), and not altered

• else, $post is considered to be the post ID (and if it's an object, it's overwridden as the object's ID), retrieved from cache or database and assigned as the post ID

I'm not skilled enough to judge it, but I think get_post() would be more consistent if the parameter always had an object related to the formatted output when the function returns, or left alone if that post wasn't found and return null. As we saw, some people may try to use get_post() just to format a post, but in some situations $post may be altered and its content lost. Documentation is faulty for not alerting about it. If there's a need of just formatting some data, there should be a function for that only purpose, that doesn't receive this data as reference and that's used by get_post(), so that people only wanting to format a post doesn't need to use a more complex function.

Well I hope I didn't make it even more confused and helped somehow o.O

[shidouhikari]

<?php

global $post;

echo '<p>$post before calling get_post($post):</p>';
var_dump($post);

$getPost = get_post($post);
echo '<p>get_post($post):</p>';
var_dump($getPost);

echo '<p>$post after calling get_post($post):</p>';
var_dump($post);

?>

Yeah, if get_post() receives global $post as parameter it bugs. Assigning global $post somewhere and later using get_post() to format it breaks the global variable.

It's very rare to happen, but if somebody does it, other codes will be affected.

[westi]

Additive patch on Ryans to also make it reference safe and not stop unnecessarily

[ryan]

(In [15188]) Fix global post stomping in get_post(). Turn get_page() into an alias of get_post(). Props westi. fixes #13753