Opened 14 years ago
Last modified 12 days ago
#20902 reviewing defect (bug)
redirect_canonical() on using permalink: Not all $_GET being redirected
| Reported by: | arieputranto | Owned by: | chriscct7 |
|---|---|---|---|
| Priority: | normal | Milestone: | |
| Component: | Canonical | Version: | 3.4 |
| Severity: | normal | Keywords: | good-first-bug has-patch has-unit-tests changes-requested |
| Cc: | Focuses: |
Description
Using permalink, I suppose that all query_var entered manually on URL or using $_GET will be redirected to proper permalink. Apparently not all being redirected at all. AFAIC:
- /?post_format=image : should be redirected to /type/image/
- /?pagename=blog : should be redirected to /blog/
- /?author_name=admin : should be redirected to /author/admin/
Unfortunately, they are not.
It can be done by filtering redirect_canonical() but it will be better if it's being done by default as we can see that /?category_name=cat will be redirected to /category/cat/
Attachments (7)
Change History (24)
#3
@
14 years ago
- Keywords has-patch needs-testing dev-feedback added; needs-patch removed
20920.diff fixes all three scenarios, mainly by adding logic to canonical.php. As for the post format canonical redirects, the current tax query redirects seems to handle tags, categories and custom taxonomy well, but the extra post_type query argument added in _post_format_request prevented it from doing so.
I feel weird that categories, tags and custom post types won't append the post_type argument, even if added to other object types with register_taxonomy_for_object_type, so I don't understand the reason why it's there for post formats, since post formats is a taxonomy, just like everything else, except that it has a custom admin interface. Would like feedback on this from Andrew and/or Mark, see [17249].
#4
@
12 years ago
@davejesch, @jdangl, and I tested this at the ocwp meetup on 4.1-alpha-30299 and the patch didn't apply. We updated the patch and applied it. As @kovshenin said, it fixes all three scenarios.
This ticket was mentioned in Slack in #core by brandondove. View the logs.
12 years ago
This ticket was mentioned in Slack in #core by brandondove. View the logs.
12 years ago
#7
@
12 years ago
- Keywords needs-unit-tests added; dev-feedback removed
The patch still looks like it's the right direction to go in, although we should also add some unit tests for these new redirections, covering both has-posts and no-posts for authors.
#9
@
11 years ago
- Keywords needs-unit-tests removed
I tested the patch, it works for me.
I've added the unit tests, adding elements to the shared fixtures.
G
#11
@
16 months ago
- Keywords needs-refresh needs-patch good-first-bug added; has-patch needs-testing early removed
It appears that this persists, but the patch is not applying cleanly any more.
Refresh or new patch needed.
cc @jubstuff
This ticket was mentioned in PR #9029 on WordPress/wordpress-develop by @sainathpoojary.
16 months ago
#12
- Keywords has-patch has-unit-tests added; needs-refresh needs-patch removed
Trac ticket: #20902
@
14 months ago
This patch ensures that all existing $_GET query parameters are preserved during canonical redirects in redirect_canonical(). Previously, some custom query args could be lost when permalinks are enabled. This fix merges all current $_GET arguments into the redirect URL, solving the issue.
#15
@
14 months ago
Bug Report
Description
Canonical redirects from query vars (post_format, pagename, author_name, category_name) to pretty permalinks are inconsistent.
Pagination requests with paged=1 on post_format and category_name remain on the query-string form instead of redirecting to the canonical pretty URL.
Environment
- WordPress: 6.9-alpha-60093-src
- PHP: 8.2.28
- Server: nginx/1.27.5
- Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
- Browser: Chrome 138.0.0.0
- OS: Linux
- Theme: Twenty Twenty-Five 1.2
- MU Plugins: None activated
- Plugins:
- Test Reports 1.2.0
Steps to Reproduce
Enable pretty permalinks (/%postname%/).
Visit the URLs listed below in a browser or with curl -I.
Expected Results
✅ /?pagename=blog → /blog/
✅ /?author_name=admin → /author/admin/
✅ /?category_name=cat → /category/cat/
✅ /?post_format=image → /type/image/
✅ Pagination should canonicalize:
&paged=1 should either redirect to the base archive or /page/1/ consistently.
&paged>1 should redirect to /.../page/{n}/.
✅ Extra query args should be preserved in redirect.
✅ Invalid values (nopage, unknown author, random post_format, nocat) should not redirect and return 404.
Actual Results
✅ http://localhost:8889/type/image/ → redirects correctly
❌ http://localhost:8889/?post_format=image&paged=1 → stays on query form (no redirect)
✅ http://localhost:8889/?post_format=random → 404 as expected
✅ http://localhost:8889/?pagename=blog → redirects correctly
✅ http://localhost:8889/?pagename=blog&utm_source=test&foo=bar → redirects correctly preserving parameters
✅ http://localhost:8889/?pagename=nopage → 404 as expected
✅ http://localhost:8889/?author_name=admin → redirects correctly
✅ http://localhost:8889/?author_name=admin&paged=1 → redirects correctly with pagination
✅ http://localhost:8889/?author_name=unknown → 404 as expected
✅ http://localhost:8889/?category_name=cat → redirects correctly
❌ http://localhost:8889/?category_name=cat&paged=1 → stays on query form (no redirect)
✅ http://localhost:8889/?category_name=nocat → 404 as expected
Additional Notes
The issue is happening with paged=1 requests for post_format and category_name.
Either it should redirect with paged=1 query parameter or normalize it to /page/1/.
Supplemental Artifacts
This ticket was mentioned in PR #13824 on WordPress/wordpress-develop by @akrocks.
12 days ago
#16
Trac ticket: https://core.trac.wordpress.org/ticket/20902
Description
With pretty permalinks enabled, /?category_name=…, /?p=…, /?page_id=… and /?author=… are redirected to their canonical URLs. The three cases from the ticket were not:
| Request | Before | After |
|---|---|---|
/?post_format=image | no redirect | → /type/image/
|
/?pagename=blog | no redirect | → /blog/
|
/?author_name=admin | no redirect | → /author/admin/
|
Changes in redirect_canonical():
- Pages:
?pagename=is handled like?page_id=, including the static front page (→/), the posts page (the ticket's/?pagename=blogexample, when "blog" is the posts page) and multi-page pages (&page=2→/slug/2/). - Authors:
?author_name=follows the rule already used for?author=: only authors with at least one published post are redirected. - Post formats:
_post_format_request()adds apost_typequery var to every post format request.redirect_canonical()took that for a var coming from a rewrite rule and skipped the redirect. That var is now ignored for thepost_formattaxonomy._post_format_request()itself is unchanged (itsis_admin()guard exists because of Trac #18475), which is where this differs from WordPress/wordpress-develop#9029.
Paging and feeds are added to the resulting URL, as they already are for categories:
| Request | Result |
|---|---|
/?post_format=audio&paged=2 | /type/audio/page/2/
|
/?author_name=admin&paged=2 | /author/admin/page/2/
|
/?author_name=admin&feed=rss2 | /author/admin/feed/
|
/?pagename=blog&paged=2 (posts page) | /blog/page/2/
|
/?pagename=about&feed=rss2 | /about/feed/
|
The paging code rebuilds the URL from the requested path, which used to discard the URL found for ?author=. That is why /?author=2&paged=2 went to /page/2/ and /?author=2&feed=rss2 to /feed/. Both now keep the author. ?pagename= on a regular page is left alone when paged is present, because paging does not apply to a page.
Extra query args are kept (/?pagename=x&foo=bar → /x/?foo=bar). Nothing is redirected for content that does not exist (unknown page, author or post format) or is not public (a draft page, an author without published posts).
About paged on the Docker development environment
On a site whose URL has a port, such as the Docker environment on localhost:8889, no request that carries paged or feed is redirected: not even /?paged=1 or /?category_name=uncategorized&paged=1 on trunk. The paging code builds $redirect_url without the port, so the "protect against chained redirects" check compares it with the requested URL and skips the redirect. That is Trac #57724 and is not changed here. Without a port in the site URL, /?post_format=image&paged=1 redirects to /type/image/.
Testing instructions
- Enable pretty permalinks. Publish a post with the Image format, and make sure a page and an author with a published post exist.
- Request
/?post_format=image,/?pagename=<page-slug>and/?author_name=<author-slug>(for example withcurl -I): each should return a 301 to the pretty URL. - Repeat with an extra argument (
&foo=bar, kept in the redirect), with values that do not exist and with a draft page (no redirect, still a 404). - Optionally set a static front page or a posts page:
/?pagename=<that-page>redirects to/or to the posts page URL. - Run the tests:
npm run test:php -- --group canonical.
Tests added: the three scenarios above with extra args, paging, feeds and negative cases in Tests_Canonical, the front page and posts page in Tests_Canonical_PageOnFront, and tests that an author without published posts is not redirected for ?author_name= (same as ?author=) and that post format requests keep their paging and feed. The audio fixture post now has the Audio format.
---
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Sonnet 5.5
Used for: Analysing the ticket, the earlier patches, reproducing the issue on a local site, drafting the redirect_canonical() changes and the PHPUnit tests, and running the test suites and PHPCS. I reviewed the code and tests and re-checked the redirects on a local site.
#17
@
12 days ago
Pull request for this ticket: https://github.com/WordPress/wordpress-develop/pull/13824 (also attached as 20902.2.diff). It applies to the current trunk (7.2-alpha) and adds tests.
What it does
With pretty permalinks, ?post_format=image, ?pagename=blog and ?author_name=admin redirect to /type/image/, /blog/ and /author/admin/ (the three cases in the PR description). Extra query args are kept, and nothing is redirected for content that does not exist or is not public. Paging and feeds are added to the resulting URL as they are for categories (?post_format=audio&paged=2 -> /type/audio/page/2/, ?author_name=admin&feed=rss2 -> /author/admin/feed/). The static front page and the posts page are covered too.
How it relates to the earlier feedback
- comment:7: tests are included for authors with posts and authors without.
- comment:14 and the review on https://github.com/WordPress/wordpress-develop/pull/9029 (removing the
post_typeoverride, see #18475 and #15378):_post_format_request()is not touched.redirect_canonical()now ignores thepost_typevar that it adds, which is what stopped post formats from redirecting. - comment:15: the four redirects, kept args and no redirect for unknown values are covered by tests. The
paged=1cases that stayed on the query form were run onlocalhost:8889. There is no request withpagedorfeedis redirected, not even/?paged=1or/?category_name=…&paged=1ontrunk, because the site URL has a port (#57724). Without a port,/?post_format=image&paged=1redirects to/type/image/with this patch.
Testing
npm run test:php -- --group canonical,query,rewrite passes and PHPCS is clean on the changed files.
@SirLouen could you have another look at your concerns from comment:14? @chriscct7 you own this ticket: are you still working on it? If not, I am happy to take it. @sainathpoojary FYI, this is related to your PR - https://github.com/WordPress/wordpress-develop/pull/9029.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)

Confirmed all 3 issues are still current, There are existing tickets for 2 & 3 I believe.