Make WordPress Core

Opened 14 years ago

Last modified 3 years ago

#12456 reopened defect (bug)

Canonical URL redirect issue with post_id/postname permalink structure

Reported by: frankprendergast's profile Frank.Prendergast Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 2.9.2
Component: Canonical Keywords: has-unit-tests has-patch
Focuses: Cc:

Description

The issue:

Using /%post_id%/%postname%/ as permalink structure,
Most canonical redirects work fine, except:

domain.com/post_id/ brings you to the post but does not redirect to the canonical domain.com/post_id/postname/

Additional info:

The above permalink structure conforms to best practice as described in the codex:
http://codex.wordpress.org/Using_Permalinks#Structure_Tags

Therefore I figured this was a bug, given the attention given to redirecting to the canonical url "So to avoid confusing search engines and to consolidate your rankings for your content, there should only be one URL for a resource."
http://markjaquith.wordpress.com/2007/09/25/wordpress-23-canonical-urls/

Attachments (5)

12456.diff (933 bytes) - added by dd32 14 years ago.
12456.2.diff (1.2 KB) - added by wonderboymusic 11 years ago.
12456.3.diff (1.3 KB) - added by atimmer 11 years ago.
12456.4.diff (814 bytes) - added by bmcculley 3 years ago.
redirectPostId.php (1.4 KB) - added by bmcculley 3 years ago.

Download all attachments as: .zip

Change History (56)

#1 @scribu
14 years ago

  • Component changed from Permalinks to Canonical
  • Milestone changed from Unassigned to 3.0
  • Owner changed from ryan to markjaquith

#2 follow-ups: @dd32
14 years ago

  • Keywords has-patch needs-testing added; permalinks canonical redirect removed
  • Milestone changed from 3.0 to 3.1
  • Type changed from defect (bug) to enhancement

It doesnt appear that Redirecting Rewritten URL's has ever been handled by the Canonical system for WordPress. Most of the code is for redirecting ?query=var to /query/something/var/.

I'm going to attach a little-tested patch, please test it out, and try to break it if possible :)

IMO, This may be fitting in as an enhancement since its not currently a handled case and is not a regression, which would mean relegating it to 3.1 as we're in 3.0 feature freeze.

@dd32
14 years ago

#3 in reply to: ↑ 2 ; follow-up: @Frank.Prendergast
14 years ago

Thank you so much for coming back to me on this. I've implemented the patch on a test site using the nightly build and so far so good.

I wasn't able to try it on my main site because I wouldn't know how to implement the patch as the canonical.php is different, so proper & full tire kicking might need to wait until 3.0 is ready for implementing on my main site.

Many thanks again. This could provide people with a nice little url shortening tool without risking losing any "Google juice" if people link to the propogated shorter url.

#4 in reply to: ↑ 3 @Frank.Prendergast
14 years ago

(My main site being WP 2.9.2)

#5 in reply to: ↑ 2 @Frank.Prendergast
14 years ago

Just to check back and say that I've had this patch on a test install I use for development and haven't run into any issues with it.

Thank you again, and let me know if I should do anything else (first time submitting anything to trac).

#6 @dd32
14 years ago

  • Keywords tested early added; needs-testing removed
  • Owner changed from markjaquith to dd32
  • Status changed from new to accepted

Thanks for getting back to me that it works as expected.

I'm not going to add this to 3.0 due to how close we are to release, so it'll be in 3.1

#7 @Otto42
14 years ago

Noticed this one yesterday. This is definitely a problem that needs to be resolved. Recommend pushing it into the next release.

#8 @nacin
14 years ago

  • Milestone changed from Awaiting Triage to 3.1

#9 @hlanggo
14 years ago

  • Cc hlanggo added

/postname/post_id also has the same problem. /postname shows the correct page but doesn't redirect to the canonical /postname/post_id

With /postname/post_id (no ending slash) permalink structure:

Canonical: http://metroparkhotelcebu.com/mph/138 (/postname/post_id)
Redirect OK: http://metroparkhotelcebu.com/138/mph (/post_id/postname)
No redirect: http://metroparkhotelcebu.com/mph (/postname)
404: http://metroparkhotelcebu.com/138 (/post_id)

Yes, I am aware that using postname first causes performance issues but back when I decided on it, http://codex.wordpress.org/Using_Permalinks said it was okay to start with postname. Thankfully, my sites are small so the performance problems aren't debilitating.

#10 @dd32
13 years ago

hlanggo: Did you try the attached patch at all?

#11 @hlanggo
13 years ago

No, I did not try the patch. Just wanted to report that /postname/post_id has the same redirect issue as post_id/postname

It's the "starting with postname" performance issue which concerns me the most. I just came across your ticket while searching about it.

#12 @jane
13 years ago

  • Milestone changed from 3.1 to Future Release

Punting this enhancement ticket b/c we are going into beta. Can reconsider for 3.2 when there is a well-tested patch.

#13 @dd32
13 years ago

  • Keywords 3.2-early added; early removed

planning on covering this, and a few other cases such as #14773 in 3.2

#14 @dd32
13 years ago

  • Keywords 3.3-early added; 3.2-early removed

Missed the boat for 3.2 again.

This covers #17653 as well.

#15 @dd32
13 years ago

Patch doesnt cover /page/xx/ redirections and a few others probably.

#16 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.6

I rejiggered dd32's patch to make it work against trunk 3 years later (class of 3.3-early) - and also fixes #17653, which is class of 3.2-early. Passes all Unit Tests.

#17 @ryan
11 years ago

  • Milestone changed from 3.6 to Future Release

@atimmer
11 years ago

#18 @atimmer
11 years ago

12456.3.diff refreshes dd32's patch against current trunk and the new repository structure.

Also adds set_url_scheme that wonderboymusic added here.

#19 @seth17
10 years ago

I'm also experiencing this issue. The /%post_id%/%postname%/ permalink structure is my preferred method, as changing the slug won't cause the post to 404, since the post id remains the same. Any chance of this getting fixed in 3.9.2?

#20 @SergeyBiryukov
10 years ago

#28781 was marked as a duplicate.

#21 @dd32
9 years ago

#29858 was marked as a duplicate.

#22 @Niresh12495
8 years ago

When will this issue be fixed?

#23 @calvin_ngan
7 years ago

  • Resolution set to worksforme
  • Status changed from accepted to closed

I've found the answer only for redirecting /%post_id%/ to /%post_id%/%postname%/
Add this to your .htaccess, change the domain name and the http/https

RedirectMatch 301 ^/(\d+)/$ https://www.geckoandfly.com/?p=$1
Last edited 7 years ago by calvin_ngan (previous) (diff)

#24 follow-up: @Niresh12495
7 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution worksforme deleted
  • Status changed from closed to reopened
  • Type changed from enhancement to defect (bug)

@calvin_ngan this solution relatively works for all domains simply add this to your .htaccess, i have been using this on my blog http://www.niresh.guru since a long time but Wordpress must fix this with PHP so im reopening this ticket

RedirectMatch 301 ^/(\d+)/$ /?p=$1

#25 in reply to: ↑ 24 ; follow-up: @calvin_ngan
7 years ago

Replying to Niresh12495:

@calvin_ngan this solution relatively works for all domains simply add this to your .htaccess, i have been using this on my blog http://www.niresh.guru since a long time but Wordpress must fix this with PHP so im reopening this ticket

RedirectMatch 301 ^/(\d+)/$ /?p=$1
  1. Nobody gave him a temporary solution, thus, I posted one.
  2. After 7 years, it is still not taken care of, I doubt they see this as bug. Probably will never be resolve.

#26 in reply to: ↑ 25 @anbumz
6 years ago

Replying to calvin_ngan:

Replying to Niresh12495:

@calvin_ngan this solution relatively works for all domains simply add this to your .htaccess, i have been using this on my blog http://www.niresh.guru since a long time but Wordpress must fix this with PHP so im reopening this ticket

RedirectMatch 301 ^/(\d+)/$ /?p=$1
  1. Nobody gave him a temporary solution, thus, I posted one.
  2. After 7 years, it is still not taken care of, I doubt they see this as bug. Probably will never be resolve.

Very unfortunate that this is not an in-built Wordpress feature as it causes problems with search engines and analytics. This .htaccess solution works like a charm, however.

How would you have to change the expression to capture URLs like this as well: /lc/post_id/ whereas lc is a two-character language code like "en" for english or "de" for German and so on. This is relevant for multilingual plugins such as WPML. Redirection destination then would be /lc/post_id/post_name/

#27 @Otto42
6 years ago

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

This likely isn't going to be fixed, because the truth is that nearly nobody uses this permalink format. The format of /%post_id%/%postname%/ is less used than just the format of /%postname%/.

Not all permalink formats need to be totally optimized, I think. We can provide examples for best choices without providing optimized support in core for every possible choice.

#28 @SergeyBiryukov
4 years ago

#35690 was marked as a duplicate.

#29 @SergeyBiryukov
4 years ago

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

Reopening, this is still a legitimate issue.

#30 @SergeyBiryukov
4 years ago

  • Keywords early added; tested 3.3-early removed
  • Milestone changed from Future Release to 5.5
  • Owner changed from dd32 to SergeyBiryukov
  • Status changed from reopened to accepted

#31 @seth17
4 years ago

Agreed, I still need this fix too.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

This ticket was mentioned in PR #287 on WordPress/wordpress-develop by donmhico.


4 years ago
#33

Redirect example.org/%post_id%/ to canonical url

Trac ticket: https://core.trac.wordpress.org/ticket/12456

#34 @donmhico
4 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

I added a PR that could hopefully resolve this issue. More tests are welcome!

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#36 @whyisjake
4 years ago

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

In 47937:

Canonical: Ensure a redirect for posts missing postname in a query with an ID.

If a site is using /%post_id%/%postname%/ as permalink structure, and is missing the postname, the site won't redirect to the appropriate URL. This change ensure that the redirect happens.

Fixes: #12456.
Props: Frank.Prendergast, dd32, Otto42, hlanggo, wonderboymusic, atimmer, seth17, calvin_ngan, Niresh12495, anbumz, SergeyBiryukov, donmhico.

#38 @SergeyBiryukov
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks for the commit, but I believe something along the lines of 12456.3.diff would be a better fix here, and would also cover some other tickets: #14773, #17653, #35437. Reopening for a closer review.

#39 @SergeyBiryukov
4 years ago

  • Keywords needs-unit-tests added; needs-testing removed

This also needs unit tests.

#40 @needle
4 years ago

Not quite sure how best to describe this, but [47937] breaks rewrite rules that use a Page as the "top level" of a "virtual hierarchy", where the path that follows the top level slug is interpreted as configuring the Page.

So, for example, the URL https://my.domain/mypage/one/two/ can be rewritten via add_rewrite_rule() to render https://my.domain/mypage/ with the "path" one/two/ determining the content that is rendered in the Page with slug mypage.

Here's a real world example of this rewrite rule in CiviCRM:

https://github.com/civicrm/civicrm-wordpress/blob/master/civicrm.php#L826-L831

What happens since [47937] is that https://my.domain/civicrm/event/info/?reset=1&id=1 incorrectly redirects to https://my.domain/civicrm/?reset=1&id=1 and therefore breaks the rendering of the Page, since the "path" has been cleared.

#41 @SergeyBiryukov
4 years ago

In 48026:

Canonical: Revert [47937] pending unit tests and further review.

Props needle.
See #12456.

#42 @davidbaumwald
4 years ago

  • Keywords early removed

Removing early tag per discussion with @SergeyBiryukov

#43 @SergeyBiryukov
4 years ago

  • Milestone changed from 5.5 to 5.6

#44 @hellofromTonya
3 years ago

  • Keywords needs-refresh added

Patch 12456.3.diff no longer applies cleanly. Also, it is under review. Marking this ticket as needs-refresh.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

#46 @hellofromTonya
3 years ago

  • Milestone changed from 5.6 to 5.7

Per scrub today, we are very late into the 5.6 beta cycle with RC1 next week. With no activity on this ticket, punting it to 5.7.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#47 @hellofromTonya
3 years ago

  • Keywords needs-patch added; needs-refresh has-patch removed

As this ticket was reverted, reclassifying from needs-refresh to needs-patch.

#48 @lukecarbis
3 years ago

  • Milestone changed from 5.7 to 5.8

Shifting this forward to 5.8.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


3 years ago

#50 @JeffPaul
3 years ago

  • Milestone changed from 5.8 to Future Release

Given no movement on a patch or PR for this since the 5.5 release cycle I'm going to punt this to the Future Release milestone. If someone is able to work on a patch or PR, then this can get added back to a numbered milestone.

@bmcculley
3 years ago

#51 @bmcculley
3 years ago

  • Keywords has-unit-tests has-patch added; needs-unit-tests needs-patch removed

Hello,

I've tried my hand at creating a patch and a unit test for this one. For the test I created a whole new file. I wasn't sure what to do for that so I attached the whole thing. If something is not right please let me know; I'd be happy to fix it.

Thank you

Note: See TracTickets for help on using tickets.