#24674 closed defect (bug) (fixed)
WP_Query::is_page() should use stricter comparison
Reported by: | clifgriffin | Owned by: | 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)
Change History (33)
#2
@
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:
↓ 4
@
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
@
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
@
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
#7
@
11 years ago
Attaching new patch with fixes for is_author()
, is_category()
, is_single()
as well as is_page()
.
#8
@
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
@
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
@
11 years ago
Setting post ID to zero is your business, it isn't a mandatory feature of an application platform.
#11
@
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.
#16
follow-up:
↓ 17
@
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 for slug '1-about-us' and page ID 1 }
#17
in reply to:
↑ 16
@
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.
#20
@
10 years ago
#22
@
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
@
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
@
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
@
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:
- Is the ID of the current page 123? If so, return true.
- Is the post_title of the current page 123? If so, return true.
- 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).
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 ofint 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.