WP_Query::is_page() should use stricter comparison

[clifgriffin]

In WP_Query::is_page(), the following evaluations are made to determine if the queried object matches the supplied page property:

		if ( in_array( $page_obj->ID, $page ) )
			return true;
		elseif ( in_array( $page_obj->post_title, $page ) )
			return true;
		else if ( in_array( $page_obj->post_name, $page ) )
			return true;

This works fine, as long as $page_obj->ID is not zero. If it is zero, the function will return true in every case.

For example:

$test = array('about-us');

if( in_array(0, $test) ) {
	// this will always be true
}

To fix this, I'd recommend using strict mode for in_array, at least for ID comparisons:

		if ( in_array( $page_obj->ID, $page, true ) )
			return true;
		elseif ( in_array( $page_obj->post_title, $page ) )
			return true;
		else if ( in_array( $page_obj->post_name, $page ) )
			return true;

It may seem unlikely that $page_obj->ID would ever be zero, but consider that some more complex plugins use virtual pages, or otherwise override the queried object. In this case, 0 is the only valid value that also won't result in a collision with an existing real post.

This seems like a harmless change that will make is_page() return more reliable results, with few if any side effects.

[nacin]

The problem with strict mode is it would prevent a numeric string from being treated as an ID. I'm sure someone has passed in '123' instead of int 123 to is_page() before.

It is possible for $page_obj to be 0 if a plugin is trying to trick WP_Query. (Not recommended.)

It would be best to simply return false under certain specific situations.

[clifgriffin]

Good point. That could certainly cause a lot of issues in existing code.

This may be crazy, but could we simply check if $page is_numeric, and then use absint()?

function is_page( $page = '' ) {
	if ( !$this->is_page )
		return false;

	if ( empty( $page ) )
		return true;

	$page_obj = $this->get_queried_object();

	if( is_numeric($page) )
		$page = absint($page);

	$page = (array) $page;

	if ( in_array( $page_obj->ID, $page, true ) )
		return true;
	elseif ( in_array( $page_obj->post_title, $page ) )
		return true;
	else if ( in_array( $page_obj->post_name, $page ) )
		return true;

	return false;
}

[nacin]

$page can be an array, so you'd need to do that check for each piece of the array. So is probably simpler to first test $page_obj->ID.

[clifgriffin]

D'oh.

This seems a bit lengthy, but should address the weaknesses noted above:

function is_page( $page = '' ) {
	if ( !$this->is_page )
		return false;

	if ( empty( $page ) )
		return true;

	$page_obj = $this->get_queried_object();

	$page = (array) $page;

	if ( $page_obj->ID > 0 && in_array( $page_obj->ID, $page ) )
		return true;
	elseif ( in_array( $page_obj->ID, $page, true ) )
		return true;
	elseif ( in_array( $page_obj->post_title, $page ) )
		return true;
	else if ( in_array( $page_obj->post_name, $page ) )
		return true;

	return false;
}

[SergeyBiryukov]

It's not just WP_Query::is_page(), other WP_Query methods (is_author(), is_category(), is_single()) follow the same pattern. Related: [6397], [6823], [15531].

[clifgriffin]

Attaching new patch with fixes for is_author(), is_category(), is_single() as well as is_page().

[wonderboymusic]

Someone setting a post's ID to zero should be a you-break-it-you-bought-it situation, but I guess we can discuss this in 3.7

[clifgriffin]

As WordPress increasingly becomes an application platform, the need for non-traditional approaches will increase.

The particular plugin in this instance is Shopp. The alternative solutions for managing the storefront URLs using actual pages and shortcodes had many shortcomings. The virtual page approach has been very successful.

Regardless, in my view, if there is a scenario where is_page(), etc return incorrect values, a patch is warranted. Especially when the patch should have no effect on any other implementations.

[wonderboymusic]

Setting post ID to zero is your business, it isn't a mandatory feature of an application platform.

[clifgriffin]

I didn't imply it was a mandatory feature. It's simply true that WordPress will be used in ways that aren't completely expected, and quirks like this will matter more.

That really isn't the issue though. The virtual page scenario is simply an example from a plugin with thousands of users.

The fact that is_page(0) returns true is a flaw, no matter the circumstance. It should always return false unless the page ID is literally 0, however unlikely.

Unless there is a legitimate reason it should return true or a scenario can be discovered where returning false could disrupt other configurations, I don't see the point in not patching it.

[nacin]

Still needs unit tests.

[nunomorgadinho]

unit test

[tobyhawkins]

The lack of strictness also causes problems where a page title starts with a number. I'm working on a site that has some intro step pages that someone has numbered (e.g. /1-set-up-your-profile.php etc). These pages have an alternative header that is getting used on pages that have the same ID as the numbered page.

$test = array('1-about-us');

if( in_array(1, $test) ) {
	// true
}

[clifgriffin]

Replying to tobyhawkins:

The lack of strictness also causes problems where a page title starts with a number. I'm working on a site that has some intro step pages that someone has numbered (e.g. /1-set-up-your-profile.php etc). These pages have an alternative header that is getting used on pages that have the same ID as the numbered page.

$test = array('1-about-us');

if( in_array(1, $test) ) {
	// true
}

Great catch! Didn't think about that scenario.

[SergeyBiryukov]

Related: #29467

[SergeyBiryukov]

#31301 was marked as a duplicate.

[r-a-y]

#31301 was just closed, which had a patch including a unit test. I've attached that patch to this ticket for review. Patch only addresses is_page() and not the other functions listed in comment:5.

Please read #31301 for more info behind the patch if interested.

[boonebgorges]

I see at least two problems here, both of which come down to the weird behavior of is_array() when the needle is an integer. Both of the following resolve to true:

in_array( 0, array( 'foo' ) );
in_array( 1, array( '1-foo' ) );

We can avoid the weirdness of is_array() with integer needle by casting the needle as a string. See 24674.2.diff​. I believe this fixes the bugs in the current ticket, though it'd be nice to get a second opinion on that.

Switching to strict mode for is_array() is another option. However, this will break backward compatibility in cases like those noted by nacin here https://core.trac.wordpress.org/ticket/24674#comment:1. We could, more generously, assume that integer-ish values passed to the function are intended to be integers. I'm afraid to guess what happens when you have a page with ID = 123 and another page with page_name = '123'. (Spoiler alert: I have already guessed.)

[r-a-y]

Switching to strict mode for is_array() is another option. However, this will break backward compatibility in cases like those noted by nacin here https://core.trac.wordpress.org/ticket/24674#comment:1.

It also fails this test:
https://core.trac.wordpress.org/browser/tags/4.1/tests/phpunit/tests/canonical/pageOnFront.php?marks=51#L40

We can avoid the weirdness of is_array() with integer needle by casting the needle as a string. See 24674.2.diff​.

I think the (string) casting works and fixes the other issue mentioned by Toby in comment:16. +1!

[nacin]

Whatever we do here, let's make sure there are some good unit tests so someone doesn't break this in the future. :-)

We should be able to specifically tell the difference between a numeric string and a non-numeric string pretty easily.

[boonebgorges]

We should be able to specifically tell the difference between a numeric string and a non-numeric string pretty easily.

Casting the queried object ID to a string before testing against it has this effect. Basically, if you pass either 123 or '123' to is_page(), the following checks happen:

1. Is the ID of the current page 123? If so, return true.
2. Is the post_title of the current page 123? If so, return true.
3. Is the post_name of the current page 123? If so, return true.

The string-casting trick ensures that there's no funny business in check (1).

[boonebgorges]

In 31458:

More careful type conversion in WP_Query is_*() methods.

is_array( 1, '1-foo' ) returns true, which means that is_page( 1 )
was returning true when on a page with the slug '1-foo'. We avoid this odd
behavior by casting the queried object ID to a string before testing against
the value passed to the conditional function.

This also helps to avoid a problem where an arbitrary value for $page would
cause is_page( $page ) to return true if the query had been manipulated by
a plugin to show that the current page's ID is 0.

Props boonebgorges, r-a-y, nunomorgadinho, wonderboymusic, clifgriffin.
Fixes #24674.

[boonebgorges]

In 36625:

Query: is_*( $int ) should not falsely match strings starting with "$int".

Another chapter in the Storied Annals of Weird in_array() Behavior:
in_array( 4, array( "4-cool-dudes" ) ); resolves to true, such that
is_page( 4 ) was returning true for posts with the name '4-cool-dudes'.

We work around this behavior by ensuring that values passed to the is_
methods are cast to strings before the in_array() checks. ID checks still
work as expected; see #24674.

Props mikejolley, swissspidy, boonebgorges.
Fixes #35902.