WordPress.org

Make WordPress Core

Opened 10 years ago

Last modified 3 months ago

#9824 reopened task (blessed)

make better use of stubs when verbose rules should apply

Reported by: Denis-de-Bernardy Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Rewrite Rules Keywords: needs-refresh, needs-unit-tests, bulk-reopened
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 9 years ago.
revert-r15987.diff (1.0 KB) - added by nacin 8 years ago.
9824.2.patch (656 bytes) - added by SergeyBiryukov 8 years ago.
9824.3.patch (778 bytes) - added by SergeyBiryukov 8 years ago.
Added specific check for Multisite.
9824.4.patch (974 bytes) - added by SergeyBiryukov 8 years ago.
9824.5.patch (983 bytes) - added by SergeyBiryukov 8 years ago.
Removed hardcoded /index.php and /blog

Download all attachments as: .zip

Change History (44)

#2 @Denis-de-Bernardy
10 years ago

  • Keywords needs-patch added

#3 @Denis-de-Bernardy
10 years ago

  • Milestone changed from Future Release to 2.9

#4 @Denis-de-Bernardy
10 years ago

  • Type changed from defect (bug) to enhancement

#5 @Denis-de-Bernardy
10 years ago

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

#6 @ryan
10 years ago

  • Milestone changed from 2.9 to Future Release

#7 @scribu
9 years ago

  • Cc scribu added

#8 @Ryan_B
9 years ago

  • Cc Ryan_B added

#9 @nkuttler
9 years ago

  • Cc wp@… added

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

  • Cc blt4 added

#13 @scribu
9 years ago

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

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

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

#16 @nacin
9 years ago

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

#17 @nacin
8 years ago

This resulted in #16041. Suggest revert and punt.

@nacin
8 years ago

#18 @nacin
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

Added specific check for Multisite.

#20 @ryan
8 years ago

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

#21 @nacin
8 years ago

  • Milestone changed from 3.1 to Future Release

#22 @nacin
8 years ago

Additional issues: #16136.

#23 @scribu
8 years ago

  • Component changed from Optimization to Rewrite Rules

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

  • Cc dromsey@… added

#26 @nacin
8 years ago

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

#27 @nacin
8 years ago

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

#28 @nacin
8 years ago

  • Type changed from enhancement to task (blessed)

#29 @SergeyBiryukov
8 years ago

Is there something not covered by 9824.3.patch?

#30 @SergeyBiryukov
8 years ago

Refreshed for 3.3.

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

Removed hardcoded /index.php and /blog

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

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

#34 @ryan
8 years ago

  • Milestone changed from 3.3 to Future Release

#35 @chriscct7
5 years ago

  • Keywords needs-refresh added; has-patch removed

#36 @chriscct7
3 years ago

  • Keywords needs-unit-tests added

#37 @iseulde
5 months ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

This ticket has not seen any activity in over *two* years, so I'm closing it as "wontfix".

The ticket may lack decisiveness, may have become irrelevant, or may not have gathered enough interest.

If you think this ticket does deserve some attention again, feel free to reopen.

For bugs, it would be great if you could provide updated steps to reproduce against the latest version of WordPress (5.0.2 at the time of writing). Remember images or a video can be superior to explain a problem. At the very least, quickly test again to make sure the bug still exists.

If it’s an enhancement or feature, some extra motivation may help.

Thank you for your contributions to WordPress! <3

#38 @JeffPaul
3 months ago

  • Keywords bulk-reopened added
  • Milestone set to Awaiting Review
  • Resolution wontfix deleted
  • Status changed from closed to reopened

A decision was made to reopen tickets that were closed in the bulk edit that this ticket was affected by. This ticket is being placed back into the Awaiting Review milestone so it can be individually evaluated and verified to determine if it is still relevant/valid or reproducible.

Note: See TracTickets for help on using tickets.