Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 9 years ago

#24674 closed defect (bug) (fixed)

WP_Query::is_page() should use stricter comparison

Reported by: clifgriffin's profile clifgriffin Owned by: boonebgorges's profile boonebgorges
Milestone: 4.2 Priority: normal
Severity: minor Version: 2.5
Component: Query Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (6)

ispage.patch (706 bytes) - added by clifgriffin 11 years ago.
24674.patch (1.8 KB) - added by clifgriffin 11 years ago.
24674.diff (1.6 KB) - added by wonderboymusic 11 years ago.
query.php.diff (547 bytes) - added by nunomorgadinho 11 years ago.
unit test
24674.ray.patch (1.4 KB) - added by r-a-y 10 years ago.
24674.2.diff (2.1 KB) - added by boonebgorges 10 years ago.

Download all attachments as: .zip

Change History (33)

@clifgriffin
11 years ago

#1 @nacin
11 years ago

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.

#2 @clifgriffin
11 years ago

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;
}

#3 follow-up: @nacin
11 years ago

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

#4 in reply to: ↑ 3 @clifgriffin
11 years ago

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;
}

#5 @SergeyBiryukov
11 years ago

  • Component changed from General to Query
  • Keywords needs-patch added; has-patch removed
  • Version changed from 3.5.2 to 2.5

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

#6 @SergeyBiryukov
11 years ago

  • Keywords needs-unit-tests added

#7 @clifgriffin
11 years ago

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

@clifgriffin
11 years ago

#8 @wonderboymusic
11 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 3.7

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

#9 @clifgriffin
11 years ago

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.

#10 @wonderboymusic
11 years ago

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

#11 @clifgriffin
11 years ago

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.

#12 @nacin
11 years ago

  • Milestone changed from 3.7 to 3.8

Still needs unit tests.

@nunomorgadinho
11 years ago

unit test

#13 @nunomorgadinho
11 years ago

  • Keywords needs-unit-tests removed

#14 @nunomorgadinho
11 years ago

  • Cc nuno.morgadinho@… added

#15 @wonderboymusic
11 years ago

  • Milestone changed from 3.8 to Future Release

#16 follow-up: @tobyhawkins
10 years ago

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
}
Last edited 10 years ago by tobyhawkins (previous) (diff)

#17 in reply to: ↑ 16 @clifgriffin
10 years ago

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.

#19 @SergeyBiryukov
10 years ago

#31301 was marked as a duplicate.

@r-a-y
10 years ago

#20 @r-a-y
10 years ago

#31301 was just closed, which had a patch. I've attached that patch to this ticket for review.

Version 0, edited 10 years ago by r-a-y (next)

#21 @SergeyBiryukov
10 years ago

  • Milestone changed from Future Release to 4.2

#22 @boonebgorges
10 years ago

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

#23 @r-a-y
10 years ago

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!

#24 @nacin
10 years ago

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.

#25 @boonebgorges
10 years ago

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

#26 @boonebgorges
10 years ago

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

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.

#27 @boonebgorges
9 years ago

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.

Note: See TracTickets for help on using tickets.