WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 years ago

#21167 reopened enhancement

Problem with custom permalinks

Reported by: kaffeeringe Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.4
Component: Permalinks Keywords:
Focuses: Cc:

Description

I am currently moving my blog (http://kaffeeringe.de/) to Wordpress and I don't want to break my old URLs (Cool URLs don't change). The problem is, that these custom URLs don't work in Wordpress 3.4.1:

/blog/%postname%/%post_id%.html works
/blog/%postname%.%post_id%.html (which I need) doesn't work.

It doesn't work with the . in between these parameters…

I hope somebody is able to help me.

Attachments (4)

21167.patch (667 bytes) - added by SergeyBiryukov 3 years ago.
21167.2.patch (502 bytes) - added by SergeyBiryukov 3 years ago.
21167-unit-test.patch (3.1 KB) - added by kurtpayne 3 years ago.
Unit test
21167-tests.patch (3.1 KB) - added by boonebgorges 5 months ago.
Removing from trunk as per #30284

Download all attachments as: .zip

Change History (29)

comment:1 @Ipstenu3 years ago

  • Keywords close added
  • Resolution set to invalid
  • Status changed from new to closed

This isn't a bug - Trac is for submitted bugs in WordPress :)

You should post in the WordPress forums instead (and I would in this case use a .htaccess to parse /blog/%postname%.%post_id%.html into /blog/%postname%/%post_id%/ (or anything else you wanted)

http://wordpress.org/support if you need more help.

comment:2 @helenyhou3 years ago

  • Milestone Awaiting Review deleted

comment:3 @nacin3 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Sounds like a bug to me. We allow random characters in a URL permalink structure, like ".html". Would be interesting to see where and why this fails.

comment:4 @helenyhou3 years ago

  • Keywords close removed
  • Milestone set to Awaiting Review

comment:5 @nacin3 years ago

  • Type changed from defect (bug) to enhancement

Looks like our support for this is quite tenuous. Many combinations work, but mostly by happenstance.

comment:6 @Ipstenu3 years ago

/%postname%.%post_id%/ fails

/%year%.%monthnum%.%postname%/ works

/%post_id%.%postname%/ works

/%postname%.%year%/ works

Well now that's just odd...

Last edited 3 years ago by Ipstenu (previous) (diff)

comment:7 @sirzooro3 years ago

  • Cc sirzooro added

I checked this and found that dot is not escaped when generating rules, e.g.:

  '([^/]+).([0-9]+)(/[0-9]+)?/?$' => 'index.php?name=$1&p=$2&page=$3'

Other special RX chars may also cause problems here - need to check if patch will work with them too.

@SergeyBiryukov3 years ago

comment:8 @SergeyBiryukov3 years ago

  • Keywords has-patch added

21167.patch is an attempt to escape the special characters which are currently allowed in permalink structure and are acceptable in terms of valid URLs.

Seems to work in the following cases:

/%postname%.%post_id%/
/%postname%+%post_id%/
/%postname%$%post_id%/
/%postname%(%post_id%)/

comment:9 follow-up: @dd323 years ago

Is there any reason why we can't just run the permastructure through preg_quote()?

> preg_quote( '.+/()$[]%' );
\.\+/\(\)\$\[\]%

comment:10 in reply to: ↑ 9 @SergeyBiryukov3 years ago

Replying to dd32:

Is there any reason why we can't just run the permastructure through preg_quote()?

Thanks, that indeed makes more sense.

comment:11 @SergeyBiryukov3 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 3.5

@SergeyBiryukov3 years ago

@kurtpayne3 years ago

Unit test

comment:12 @kurtpayne3 years ago

  • Cc kpayne@… added

Unit test added, 21167.2.patch looks good. Let me know if there are any test cases missing.

comment:13 @tlovett13 years ago

  • Owner set to tlovett1
  • Status changed from reopened to accepted

comment:14 @tlovett13 years ago

  • Owner tlovett1 deleted
  • Status changed from accepted to assigned

comment:15 @nacin3 years ago

  • Keywords commit added

comment:16 @wonderboymusic2 years ago

@kurtpayne - was that unit test patch committed?

comment:17 @SergeyBiryukov2 years ago

  • Keywords needs-unit-tests removed

comment:19 @ryan2 years ago

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

comment:20 @xknown2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The fix is probably incomplete. We should be passing the second argument on preg_quote and use the same delimiter where rewrite rules are being used.

For example, having the following permalink structure "/%post_id%#foo" (yes, probably unrealistic) would produce an invalid regular expression in wp-includes/class-wp.php, where # is used as a delimiter.
http://core.trac.wordpress.org/browser/trunk/wp-includes/rewrite.php#L344
http://core.trac.wordpress.org/browser/trunk/wp-includes/class-wp.php#L204

comment:21 @ryan2 years ago

  • Keywords has-patch commit removed

comment:22 @nacin2 years ago

Seems to me like this should be preg_quote()'d before getting used in a regexp, rather than on save.

comment:23 @jeremyfelt2 years ago

Using preg_quote() breaks rewrites for post types with hyphens. I'm not entirely sure how supported that is to begin with, but I know I need to convert a few now. :)

comment:24 @ryan2 years ago

In 22403:

Revert [22365]. see #21167

comment:25 @ryan2 years ago

  • Milestone changed from 3.5 to Future Release

@boonebgorges5 months ago

Removing from trunk as per #30284

Note: See TracTickets for help on using tickets.