WordPress.org

Make WordPress Core

Opened 11 years ago

Last modified 17 months ago

#9824 reopened task (blessed)

make better use of stubs when verbose rules should apply

Reported by: Denis-de-Bernardy Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Rewrite Rules Keywords: needs-refresh needs-unit-tests
Focuses: Cc:

Description

Related to:

http://core.trac.wordpress.org/ticket/6603#comment:27

Problem fixed is:

posts show up as www.apexprd.org/page/2 and not /news-and-events/page/2 as it should.

with permalinks set to /something/$postname%/

we arguably don't necessarily need verbose rules here, since there is a stub.

Attachments (6)

fix_verbose_rewrite.patch (612 bytes) - added by blt4 10 years ago.
revert-r15987.diff (1.0 KB) - added by nacin 10 years ago.
9824.2.patch (656 bytes) - added by SergeyBiryukov 10 years ago.
9824.3.patch (778 bytes) - added by SergeyBiryukov 10 years ago.
Added specific check for Multisite.
9824.4.patch (974 bytes) - added by SergeyBiryukov 9 years ago.
9824.5.patch (983 bytes) - added by SergeyBiryukov 9 years ago.
Removed hardcoded /index.php and /blog

Download all attachments as: .zip

Change History (42)

#2 @Denis-de-Bernardy
11 years ago

  • Keywords needs-patch added

#3 @Denis-de-Bernardy
11 years ago

  • Milestone changed from Future Release to 2.9

#4 @Denis-de-Bernardy
11 years ago

  • Type changed from defect (bug) to enhancement

#5 @Denis-de-Bernardy
11 years ago

  • Owner Denis-de-Bernardy deleted
  • Status changed from new to assigned

#6 @ryan
11 years ago

  • Milestone changed from 2.9 to Future Release

#7 @scribu
10 years ago

  • Cc scribu added

#8 @Ryan_B
10 years ago

  • Cc Ryan_B added

#9 @nkuttler
10 years ago

  • Cc wp@… added

#10 @Ryan_B
10 years ago

In wp-includes/rewrite.php on line 1984.

change code to if ( preg_match("#\A/%(?:postname|category|tag|author)%#", $this->permalink_structure) )

Works on my test. It appears the current regular expression returns looks at the first wildcard and if it is postname, category, tag, or author returns true, forcing verbose page rules. However per discussion on wp-hackers mailing list verbose page rules need not apply in all cases where one of those items is the first wildcard, if preceeded by a static string, such as /blog/%pagename%. My new regular expression specifically looks at the first element in the path and if it is one of those 4 wildcards returns true, otherwise if its some other wildcard or a string like /blog/... returns false ensuring verbose page rules are set only when absolutley necessary.

Sorry I was unable to provide a .diff for this, I could not get the current code to checkout from subversion, some error I have been unable to resolve, so since its a minor fix figured i'd just include it as a comment here to be included in next release hopefully.

#11 @blt4
10 years ago

Attached is a patch that should fix this.

Current code makes the verbose rules judgement based on the first structure tag. This post suggests you should be able to use a static string in front of a string structure tag to prevent verbose rules.

For example, the following permalink custom structure should not trigger verbose rules, but does in WP 3.0.1:

/view/%pagenum%

#12 @blt4
10 years ago

  • Cc blt4 added

#13 @scribu
10 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 3.1

#14 @scribu
10 years ago

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

(In [15987]) Don't trigger verbose page rules if permalink begins with fixed string. Fixes #9824

#15 @nacin
10 years ago

(In [15988]) Bool cast preg_match. see #9824.

#16 @nacin
10 years ago

(In [15989]) Bool cast preg_match, in readable form. see #9824.

#17 @nacin
10 years ago

This resulted in #16041. Suggest revert and punt.

@nacin
10 years ago

#18 @nacin
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#19 @SergeyBiryukov
10 years ago

I guess we need to exclude the standard prefixes, e.g. /index.php without mod_rewrite and /blog on Multisite. This also fixes the issue in #16041. Uploaded the patch.

@SergeyBiryukov
10 years ago

Added specific check for Multisite.

#20 @ryan
10 years ago

(In [17226]) Revert [15987] [15988] [15989]. Props SergeyBiryukov. see #9824 fixes #16041

#21 @nacin
10 years ago

  • Milestone changed from 3.1 to Future Release

#22 @nacin
10 years ago

Additional issues: #16136.

#23 @scribu
10 years ago

  • Component changed from Optimization to Rewrite Rules

#24 @scribu
10 years ago

  • Component changed from Rewrite Rules to Performance
  • Milestone changed from Future Release to 3.2

I think we should take another swing at this.

#25 @goto10
10 years ago

  • Cc dromsey@… added

#26 @nacin
9 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 3.2 to Future Release

#27 @nacin
9 years ago

  • Component changed from Performance to Rewrite Rules
  • Milestone changed from Future Release to 3.3

#28 @nacin
9 years ago

  • Type changed from enhancement to task (blessed)

#29 @SergeyBiryukov
9 years ago

Is there something not covered by 9824.3.patch?

#30 @SergeyBiryukov
9 years ago

Refreshed for 3.3.

#31 follow-up: @nacin
9 years ago

Looks like it. It doesn't cause any issue in either #16136 or #16041?

The thing I see is that /blog can be changed via a manual edit to the permalink_structure. If we're on the main site of a subdirectory multisite, perhaps we should pop off the first segment? Might need wpmuguru to explain what people may change that to.

@SergeyBiryukov
9 years ago

Removed hardcoded /index.php and /blog

#32 in reply to: ↑ 31 @SergeyBiryukov
9 years ago

  • Keywords has-patch added; needs-patch removed

Replying to nacin:

It doesn't cause any issue in either #16136 or #16041?

Nope.

The thing I see is that /blog can be changed via a manual edit to the permalink_structure. сIf we're on the main site of a subdirectory multisite, perhaps we should pop off the first segment?

Done in 9824.5.patch. Perhaps instead of a hardcoded value, it should be a variable in WP_Rewrite, like $wp_rewrite->index?

#33 @nacin
9 years ago

This ticket is mitigated by the fix in #16687, but it'd still be nice to get this in.

#34 @ryan
9 years ago

  • Milestone changed from 3.3 to Future Release

#35 @chriscct7
6 years ago

  • Keywords needs-refresh added; has-patch removed

#36 @chriscct7
5 years ago

  • Keywords needs-unit-tests added
Note: See TracTickets for help on using tickets.