Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#19876 closed defect (bug) (fixed)

generate_rewrite_rules() for posts/pages should set $paged and $walk_dirs false

Reported by: duck_ Owned by: duck_
Milestone: 3.4 Priority: normal
Severity: normal Version:
Component: Rewrite Rules Keywords: has-patch
Focuses: Cc:


The default value of true for these two flags results in extra, nonsensical rules for these permastructs.

$walk_dirs = true indicates that a loop should be run through each component (separated by /) of the permastruct with rules being built for each level in turn.

The purpose of walking through the directory structure is to build pretty permalinks for date archives and was introduced in [1373]. [1374] started the shift to the code structure we have today with rewrite_rules() calling generate_rewrite_rules() for different structs, but continued to do the walk for the post structure and only did a separate call for date archives if %year%, %monthnum% and %day% weren't all in the permalink structure. Finally [1868] started always generating the date permalinks, but left in the directory walking for post permalinks.

This leads to duplicated effort - date permalinks don't need to be generated twice.

It also causes endpoints to be applied in too many locations, e.g. set permalink structure to "%category%/%postname%/" and set up an endpoint for permalinks:

add_rewrite_endpoint( 'foo', EP_PERMALINK );

Notice that the endpoint rewrite not only gets added to the end of the single post rule but also:

"(.+?)/foo(/(.*))?/?$" => "index.php?category_name=$matches[1]&foo=$matches[3]"

which is a rewrite to a category archive and not a permalink.

For the same reason as above (the $ep_mask, EP_PERMALINK, passed as an argument doesn't apply to all the 'directories' being walked over) you also get strange rules like:

"(.+?)/comment-page-([0-9]{1,})/?$" => "index.php?category_name=$matches[1]&cpage=$matches[2]"

Comment pages for a category archive?

$paged = true indicates that the archive style pagination rule (/page/[0-9]+/) should be added. This leads to an extra rule, for example:

'([0-9]{4})/([0-9]{1,2})/([^/]+)/page(/?[0-9]+)/?$' => 'index.php?year=$matches[1]&monthnum=$matches[2]&name=$matches[3]&paged=$matches[4]'

That style of pagination is for archives and not single posts or pages which use <permalink>/[0-9]+/.

This extra pagination rule was introduced for posts in [1373] (to allow for pretty pagination for date archives) and for pages in [2146].

One reason against the change that I can think of is that these rules have been around for so long somebody might expect them to be there.

There are also some situations which currently work when really I don't think they should.

  • http://example.com/uncategorized/ (without the category base) works and currently isn't redirected by canonical (#18734). With the patch it would 404.
  • http://example.com/2012/hello-world/page/2/ works as it is redirected by canonical to http://example.com/2012/hello-world/, but with the patch it would 404.

Related #17185

Attachments (4)

19876.diff (3.9 KB) - added by duck_ 6 years ago.
bare-category-redirect.diff (569 bytes) - added by duck_ 6 years ago.
PoC hack to redirect http://example.com/uncategorized/
bare-category-redirect.2.diff (3.3 KB) - added by duck_ 6 years ago.
19876.paged-for-pages.diff (612 bytes) - added by duck_ 6 years ago.

Download all attachments as: .zip

Change History (15)

6 years ago

#1 @duck_
6 years ago

In [19737]:

Set $paged and $walk_dirs to false in calls to WP_Rewrite::generate_rewrite_rules() for posts/pages. See #19876.

#2 @duck_
6 years ago

  • Milestone changed from Awaiting Review to 3.4

#3 @duck_
6 years ago

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

In [19778]:

Redirect bare category URLs for permalink structures starting with %category%. Fixes #19876.

When using a structure like /%category%/%postname%/ it might be thought that /uncategorized/
(missing the "/category/" base) results in the category archive. Previously this worked due to
$walk_dirs = true for the post permalink structure, but canonical didn't redirect it. Now
there is no rewrite rule to match => 404. The fix is to use a canonical redirect.

#4 follow-up: @mdawaffe
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[19737] breaks /page/2 links for page_for_posts.

#5 in reply to: ↑ 4 @duck_
6 years ago

Replying to mdawaffe:

[19737] breaks /page/2 links for page_for_posts.

Oh yeah, good catch.

So we have to bring $paged = true back for pages. It could be done only when get_option( 'page_for_posts' ) !== false, but it might just be better to always have it.

#6 @nacin
6 years ago

Better to always have it on the basis that we don't need to flush the rewrite rules for when that option switches (and which may be controlled by a plugin). Good with paged-for-pages.diff.

#7 @duck_
6 years ago

In [19810]:

Pages must be paged (/page/x/) to allow for the page_for_posts option. See #19876.

#8 @duck_
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Should be done here.

#10 follow-up: @LucasMS
4 years ago

The endpoints are still being added to categories, this bug was not fixed.
Wordpress keeps generating rules like this:

"(.+?)/foo(/(.*))?/?$" => "index.php?category_name=$matches[1]&foo=$matches[3]"

#11 in reply to: ↑ 10 @SergeyBiryukov
4 years ago

Replying to LucasMS:

The endpoints are still being added to categories, this bug was not fixed.

The fix was reverted because it broke some valid use cases, see the follow-up tickets linked above.

If you'd like to suggest another approach, feel free to create a new ticket.

Note: See TracTickets for help on using tickets.