WordPress.org

Make WordPress Core

Opened 9 years ago

Last modified 2 years ago

#9824 reopened task (blessed)

make better use of stubs when verbose rules should apply

Reported by: Denis-de-Bernardy Owned by:
Milestone: Future Release 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 8 years ago.
revert-r15987.diff (1.0 KB) - added by nacin 7 years ago.
9824.2.patch (656 bytes) - added by SergeyBiryukov 7 years ago.
9824.3.patch (778 bytes) - added by SergeyBiryukov 7 years ago.
Added specific check for Multisite.
9824.4.patch (974 bytes) - added by SergeyBiryukov 7 years ago.
9824.5.patch (983 bytes) - added by SergeyBiryukov 7 years ago.
Removed hardcoded /index.php and /blog

Download all attachments as: .zip

Change History (42)

#2 @Denis-de-Bernardy
9 years ago

  • Keywords needs-patch added

#3 @Denis-de-Bernardy
9 years ago

  • Milestone changed from Future Release to 2.9

#4 @Denis-de-Bernardy
9 years ago

  • Type changed from defect (bug) to enhancement

#5 @Denis-de-Bernardy
9 years ago

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

#6 @ryan
9 years ago

  • Milestone changed from 2.9 to Future Release

#7 @scribu
8 years ago

  • Cc scribu added

#8 @Ryan_B
8 years ago

  • Cc Ryan_B added

#9 @nkuttler
8 years ago

  • Cc wp@… added

#10 @Ryan_B
8 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
8 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
8 years ago

  • Cc blt4 added

#13 @scribu
8 years ago

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

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

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

#16 @nacin
8 years ago

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

#17 @nacin
7 years ago

This resulted in #16041. Suggest revert and punt.

@nacin
7 years ago

#18 @nacin
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

Added specific check for Multisite.

#20 @ryan
7 years ago

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

#21 @nacin
7 years ago

  • Milestone changed from 3.1 to Future Release

#22 @nacin
7 years ago

Additional issues: #16136.

#23 @scribu
7 years ago

  • Component changed from Optimization to Rewrite Rules

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

  • Cc dromsey@… added

#26 @nacin
7 years ago

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

#27 @nacin
7 years ago

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

#28 @nacin
7 years ago

  • Type changed from enhancement to task (blessed)

#29 @SergeyBiryukov
7 years ago

Is there something not covered by 9824.3.patch?

#30 @SergeyBiryukov
7 years ago

Refreshed for 3.3.

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

Removed hardcoded /index.php and /blog

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

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

#34 @ryan
7 years ago

  • Milestone changed from 3.3 to Future Release

#35 @chriscct7
4 years ago

  • Keywords needs-refresh added; has-patch removed

#36 @chriscct7
2 years ago

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