Make WordPress Core

Opened 7 years ago

Last modified 5 years ago

#39535 new defect (bug)

Canonical redirects disallow tag named comments

Reported by: dwc's profile dwc Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Canonical Keywords: has-patch needs-testing needs-unit-tests
Focuses: Cc:

Description

The redirect_canonical function includes a regular expression replacement which effectively disallows tags named "comments" to have an RSS feed:

https://github.com/WordPress/WordPress/blob/master/wp-includes/canonical.php#L285

For example:

/blog1/tag/comments/feed/

This is redirected to:

/blog1/tag/feed/

Could / should the regular expression on line 285 could pay attention to the first placeholder it matched (comments/?) or check whether the page is a tag request?

Attachments (2)

39535.diff (1.4 KB) - added by asalce 7 years ago.
39535.unit-test.patch (1.8 KB) - added by asalce 7 years ago.

Download all attachments as: .zip

Change History (19)

@asalce
7 years ago

#1 @asalce
7 years ago

  • Keywords has-patch needs-testing added

@dwc I attached a patch which should work. The safest way was to check against the generated site-wide comments feed. Could you take look and let me know if that solves your issue?

#2 @asalce
7 years ago

@dd32 @markjaquith I looked a bit at the source code and this seems like it has been around for a while. Sorry to pull you in on this one, not sure who would be assigned :)

Here is the commit on Git:
https://github.com/WordPress/WordPress/commit/ecffc4649f4cb90b43fe0b38ab0f4b18967cb3fd

I was able to reproduce the bug myself, real easy to do so. Just create a tag named "comments" and then try to load the rss feed for the tag: http://URL/tag/comments/feed/

Edit: tagged the canonical gods

Last edited 7 years ago by asalce (previous) (diff)

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


7 years ago

#4 @swissspidy
7 years ago

  • Keywords needs-unit-tests added
  • Version 4.7 deleted

#5 @dwc
7 years ago

@asalce Thanks for the patch! I've confirmed it works in my setup.

#6 @asalce
7 years ago

@swissspidy I submitted the unit-test, The best way I could test this out was to test if the query vars were set correctly after a go_to("/tag/comments/feed/") the query vars should have a "feed" and "tag" should equal "comments" , I also added some other tests to make sure tests were running correctly.

@dwc perfect! :)

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


7 years ago

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


7 years ago

#9 follow-up: @SergeyBiryukov
7 years ago

  • Milestone changed from Awaiting Review to 4.8

#10 in reply to: ↑ 9 @asalce
7 years ago

Replying to SergeyBiryukov:

I ran into this bug with another site, Any chance this could be moved into 4.7.x ?

#11 @javix
7 years ago

I have tested this patch successfully, it works for me.

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


7 years ago

#13 @obenland
7 years ago

  • Milestone changed from 4.8 to Future Release

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


7 years ago

#15 @asalce
5 years ago

Can this one also be put up for review?
https://make.wordpress.org/core/2019/02/20/reverting-the-bulk-ticket-closing/ @obenland @JeffPaul

#16 @JeffPaul
5 years ago

@asalce I don't think this ticket relates to the bulk ticket closing, but if you're asking about getting this milestoned then your best bet is to check with the Canonical component maintainers (see https://make.wordpress.org/core/components/permalinks/canonical/).

#17 @asalce
5 years ago

@JeffPaul Thanks!

@markjaquith this ticket has been in my account for a bit. I supplied a patch and unit testing awhile ago. Let me know what else is needed to get this into a release. Thanks!

  • Alberto S.
Note: See TracTickets for help on using tickets.