Make WordPress Core

Opened 14 years ago

Last modified 5 months ago

#20902 reviewing defect (bug)

redirect_canonical() on using permalink: Not all $_GET being redirected

Reported by: arieputranto's profile arieputranto Owned by: chriscct7's profile chriscct7
Milestone: Priority: normal
Severity: normal Version: 3.4
Component: Canonical Keywords: good-first-bug has-patch has-unit-tests changes-requested
Focuses: Cc:

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:

  1. /?post_format=image : should be redirected to /type/image/
  2. /?pagename=blog : should be redirected to /blog/
  3. /?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 (6)

20920.diff (2.4 KB) - added by kovshenin 13 years ago.
20902.1.diff (2.4 KB) - added by brandondove 11 years ago.
Updated patch for 4.1-alpha-30299
20902.patch (4.7 KB) - added by jubstuff 10 years ago.
Unit tests added
20902-preserve-get.patch (1.3 KB) - added by sachinrajcp123 6 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.
pagination-issue.png (56.6 KB) - added by kalpeshh 5 months ago.
20902-fix-redirect-canonical-query-vars.patch (1.6 KB) - added by sachinrajcp123 3 months ago.

Download all attachments as: .zip

Change History (21)

#1 @dd32
13 years ago

  • Component changed from Permalinks to Rewrite Rules

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

#2 @dd32
13 years ago

  • Component changed from Rewrite Rules to Canonical

@kovshenin
13 years ago

#3 @kovshenin
13 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].

@brandondove
11 years ago

Updated patch for 4.1-alpha-30299

#4 @brandondove
11 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.


11 years ago

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


11 years ago

#7 @dd32
11 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.

#8 @chriscct7
10 years ago

  • Keywords early added

@jubstuff
10 years ago

Unit tests added

#9 @jubstuff
10 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

#10 @chriscct7
10 years ago

  • Owner set to chriscct7
  • Status changed from new to reviewing

#11 @SirLouen
8 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.


8 months ago
#12

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

Trac ticket: #20902

#13 @sainathpoojary
8 months ago

  • Keywords needs-testing added

#14 @SirLouen
8 months ago

  • Keywords changes-requested added; needs-testing removed

@sainathpoojary I've added a code review

Reviewing the code, I'm not 100% confident this is the right way to proceed

Related #18475 and #15378

Needs further inspection and evaluating why these changes are appropiate.

@sachinrajcp123
6 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 @kalpeshh
5 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.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/9029.diff

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

Screenshots for failing requests.

Last edited 5 months ago by kalpeshh (previous) (diff)
Note: See TracTickets for help on using tickets.