Ticket #19876 (closed defect (bug): fixed)

Opened 5 weeks ago

Last modified 2 weeks ago

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

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

Description

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

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

Change History

duck_5 weeks ago

In [19737]:

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

duck_4 weeks ago

  • Milestone changed from Awaiting Review to 3.4

duck_4 weeks ago

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

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.

comment:4 follow-up: ↓ 5   mdawaffe3 weeks ago

  • Status changed from closed to reopened
  • Resolution fixed deleted

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

comment:5 in reply to: ↑ 4   duck_3 weeks 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.

duck_3 weeks 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.

In [19810]:

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

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

Should be done here.

Note: See TracTickets for help on using tickets.