Make WordPress Core

Opened 13 years ago

Closed 10 years ago

#16802 closed enhancement (fixed)

is_page() doesn't accept a full path

Reported by: johnbillion's profile johnbillion Owned by: johnbillion's profile johnbillion
Milestone: 4.0 Priority: normal
Severity: minor Version: 3.1
Component: Query Keywords: has-patch commit
Focuses: Cc:

Description

If you have a page at example.com/foo/bar, then is_page('foo/bar') will return false on that page.

The is_page() function only checks the ID, title and post name, not the full path.

If the $page parameter passed to is_page() contains a slash then we should check the full path. If I get a chance I'll see if I can whip up a patch in the next couple of days.

Attachments (6)

16802.patch (641 bytes) - added by johnbillion 13 years ago.
16802.2.patch (789 bytes) - added by Jesper800 10 years ago.
16802.3.patch (2.6 KB) - added by johnbillion 10 years ago.
16802.diff (3.6 KB) - added by johnbillion 10 years ago.
16802.2.diff (5.2 KB) - added by engelen 10 years ago.
16802.3.diff (5.3 KB) - added by engelen 10 years ago.

Download all attachments as: .zip

Change History (27)

#1 @johnbillion
13 years ago

  • Cc johnbillion@… added

#2 follow-up: @hakre
13 years ago

I suggest to name such a function is_page_path('foo/bar') for a starter to prevent further parameter overloading of is_page().

#3 in reply to: ↑ 2 @johnbillion
13 years ago

Replying to hakre:

I suggest to name such a function is_page_path('foo/bar') for a starter to prevent further parameter overloading of is_page().

What's "parameter overloading"? Why do we need another function?

#4 @greuben
13 years ago

  • Keywords 2nd-opinion added

is_page() accepts slug and title, I don't think there is need for is_page( path ) or is_page_path function.

#5 @kawauso
13 years ago

function is_page_path( $path ) {
	$page = get_page_by_path( $path );
	return $page ? is_page( $page->ID ) : null;
}

Something like that?

-1 to having this in core, it seems like a niche and untidy way of doing things.

@johnbillion
13 years ago

#6 @johnbillion
13 years ago

  • Keywords has-patch added

Patch.

@hakre, @greuben and @kawauso: Why the opposition? is_page() accepts a slug, so accepting a full path is a natural extension. In fact it's not even a natural extension, I'd say it's expected behaviour. Multiple pages with different parents can have the same slug, so this eliminates the ambiguity.

#7 @wonderboymusic
11 years ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Awaiting Review to 3.7

Patch still applies cleanly to trunk - this patch makes sense to me, moving to 3.7 for discussion

#8 @SergeyBiryukov
11 years ago

Shouldn't get_page_by_path() be used instead?

#9 @nacin
11 years ago

Probably the best argument against this is that get_page_uri() triggers possibly unnecessary queries.

If it is to happen, it should be smarter. As in, explode by /, first check the slug, then check if it has a post_parent (and if not, then it's not a match if the path indicates it is a child page), then check the next post parent, etc. Essentially, it should walk its way up the tree one by one and compare, rather than a get_page_uri() which could cause a few queries and/or cache hits.

#10 @helen
11 years ago

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

See nacin's comment for trying an alternate approach.

@Jesper800
10 years ago

#11 @Jesper800
10 years ago

I agree with Sergey Biryukov, get_page_by_path should be used for the approach suggested by Nacin. It runs a single query to fetch the ID, parent ID, slug and post type from the pages in the page path (exploded by /). After that, it checks the parts starting by the last child and moving up to the top ancestor.

However, when running this with many page paths in the array parameter passed to is_page, this might actually be slower than the get_page_uri approach.

I've patched this to use the get_page_by_path-approach (16802.2.patch).

If this is accepted, it should also be implemented for is_single.

#12 @Jesper800
10 years ago

  • Cc jeve0@… added
  • Keywords has-patch added; needs-patch removed

#13 @wonderboymusic
10 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.0

#14 @jnhghy
10 years ago

Why would

is_page($path)

be false when $path is actually a page?
I think that the function is_page() should answer with true if a page path is passed to it.
At least this makes sense to me.

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


10 years ago

#16 @johnbillion
10 years ago

  • Keywords commit added; needs-unit-tests removed

I'm happy with 16802.3.patch.

#17 @SergeyBiryukov
10 years ago

Should this be applied to is_single() as well (for hierarchical custom post types), as suggested in comment:11?

#18 @engelen
10 years ago

Jesper800 here.

@SergeyBirukov I believe it should. As closely related as is_single and is_page are (they behave exactly the same, except for the first conditional ($this->is_single and $this->is_page, respectively), their behaviours shouldn't diverge. Can I start implementing this additional feature?

On a side-note: shouldn't we actually add a separate method for the main functionality of is_single and is_page and call it from both functions after the is_single and is_page property checks?

@johnbillion
10 years ago

#19 @johnbillion
10 years ago

16802.diff implements the same path checking for is_single() and also adds a check for the existence of a forward slash at a non-zero position in the path in order to avoid unnecessary queries.

@engelen
10 years ago

@engelen
10 years ago

#20 @engelen
10 years ago

16802.3.diff adds unit testing (similar to the path unit testing for is_page) to is_single.

#21 @johnbillion
10 years ago

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

In 29039:

Add support for a full path parameter to is_page() and is_single(). Props Jesper800, engelen, johnbillion. Fixes #16802.

Note: See TracTickets for help on using tickets.