Make WordPress Core

Opened 15 years ago

Last modified 5 years ago

#9824 reopened task (blessed)

make better use of stubs when verbose rules should apply

Reported by: denis-de-bernardy's profile 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 13 years ago.
revert-r15987.diff (1.0 KB) - added by nacin 13 years ago.
9824.2.patch (656 bytes) - added by SergeyBiryukov 13 years ago.
9824.3.patch (778 bytes) - added by SergeyBiryukov 13 years ago.
Added specific check for Multisite.
9824.4.patch (974 bytes) - added by SergeyBiryukov 13 years ago.
9824.5.patch (983 bytes) - added by SergeyBiryukov 13 years ago.
Removed hardcoded /index.php and /blog

Download all attachments as: .zip

Change History (42)

#2 @Denis-de-Bernardy
15 years ago

  • Keywords needs-patch added

#3 @Denis-de-Bernardy
15 years ago

  • Milestone changed from Future Release to 2.9

#4 @Denis-de-Bernardy
15 years ago

  • Type changed from defect (bug) to enhancement

#5 @Denis-de-Bernardy
15 years ago

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

#6 @ryan
14 years ago

  • Milestone changed from 2.9 to Future Release

#7 @scribu
13 years ago

  • Cc scribu added

#8 @anonymized_6443559
13 years ago

  • Cc Ryan_B added

#9 @nkuttler
13 years ago

  • Cc wp@… added

#10 @anonymized_6443559
13 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
13 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
13 years ago

  • Cc blt4 added

#13 @scribu
13 years ago

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

#14 @scribu
13 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
13 years ago

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

#16 @nacin
13 years ago

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

#17 @nacin
13 years ago

This resulted in #16041. Suggest revert and punt.

@nacin
13 years ago

#18 @nacin
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#19 @SergeyBiryukov
13 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
13 years ago

Added specific check for Multisite.

#20 @ryan
13 years ago

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

#21 @nacin
13 years ago

  • Milestone changed from 3.1 to Future Release

#22 @nacin
13 years ago

Additional issues: #16136.

#23 @scribu
13 years ago

  • Component changed from Optimization to Rewrite Rules

#24 @scribu
13 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
13 years ago

  • Cc dromsey@… added

#26 @nacin
13 years ago

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

#27 @nacin
13 years ago

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

#28 @nacin
13 years ago

  • Type changed from enhancement to task (blessed)

#29 @SergeyBiryukov
13 years ago

Is there something not covered by 9824.3.patch?

#30 @SergeyBiryukov
13 years ago

Refreshed for 3.3.

#31 follow-up: @nacin
13 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
13 years ago

Removed hardcoded /index.php and /blog

#32 in reply to: ↑ 31 @SergeyBiryukov
13 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
13 years ago

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

#34 @ryan
12 years ago

  • Milestone changed from 3.3 to Future Release

#35 @chriscct7
9 years ago

  • Keywords needs-refresh added; has-patch removed

#36 @chriscct7
8 years ago

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