I have a large number of redirects that are not redirecting if a trailing slash is added. I'm trying to debug this to figure out if there is just a issue with my redirects but can't figure out where I should look.

Both "Enforce clean and canonical URLs" and "Redirect from non-canonical URLs to the canonical URLs" are turned on. It seems like this is dying when it gets to RouteNormalizerRequestSubscriber::onKernelRequestRedirect() where it checks if it is a master request. I'm guessing somewhere up the chain the event's request is a sub-request, but I'm not sure how to check that (or figure out if it is the intended behavior).

For testing, I am copying a redirect_source__path from the DB, adding a trailing slash onto my site's url. For example, "http://localhost/2002/some-article/555/" (the path in the DB is 2002/some-article/555. The redirect_redirect__uri is set to an internal path (internal:/node/555) and there are no options set in the DB.

Drupal version: 8.3.1

CommentFileSizeAuthor
#10 redirect_with_trailing-2882881-10.patch4.71 KBamatzies
#2 redirect_with_trailing-2882881-2.patch637 bytespurushotam.rai

Comments

rballou created an issue. See original summary.

purushotam.rai’s picture

purushotam.rai commented
Status: Active » Needs review
StatusFileSize
new redirect_with_trailing-2882881-2.patch637 bytes
rballou’s picture

rballou commented
Status: Needs review » Reviewed & tested by the community

This appears to work for me (I had to alter the patch to apply against our specific version as opposed to the dev version). Thank you for the patch!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)
Issue tags: +Needs tests

Hm, not sure about this change.

Needs tests at least.

This should be resolved as two different redirects actually, which is not optimal but it will also handle all kinds of other unclean things. First the generic clean-up and then it should match the explictly stored redirect (while both things are provided by this module, they are technically not related at all). Do other non-canical redirects work for you, like upper/lower case or missing language prefixes in case you use multiple languages? What about redirecting to aliases, does node/1 redirect to /whatever-the-alias-is? What about node/1/ and actually existing aliases with a trailing slash?

rballou’s picture

rballou commented

The URL clean up is not happening on source URLs, just destinations. So if someone goes to /whatever-the-alias-is/ it will redirect to /whatever-the-alias-is. But if I add a redirect with a source of /whatever, going to /whatever/ will trigger a 404 (without the trailing slash will go to the destination).

In general, I can add the duplicate redirects if the URL cleanup options are not intended to cleanup source requests. This will be means adding another 100k+ redirects in this case (or adding custom EventSubscriber to handle the 404). If adding an option to account for this is a possibility it would save this extra work. The only other fix I can really think of is checking with and without the slash, but I can't really think of a good way for that to work on a generic module level.

I couldn't find the original issues that led me to create the issue, but here is one similar one: https://www.drupal.org/node/2746221

Thanks

dandaman’s picture

dandaman commented

I've also found this issue on a site we just launched. And the patch worked for me. If I get a chance sometime, I'll try to write some tests.

I'd like to have to have redirects from /subscribe and /subscribe/ both go to one node or URL. I'd rather just make the first redirect and the second one just works as well. That's what the patch seems to do and hopefully tests will verify that.

killes@www.drop.org’s picture

killes@www.drop.org commented

IMO this issue needs fixing for consistency of redirect with path aliases.

For a path alias "/foo" "/foo/" will redirect to "/foo", so if I redirect "/bar" to "/node/1", "/bar/" should also redirct to "/node/1".

heyehren’s picture

heyehren commented

I love the redirect module but this issue really cause me some headache. I had some redirects that would just not work. It took me a while to figure out that it may have to do with the trailing slashes. Applying the patch fixed the issue for me. I think it would prevent some confusion by site maintainers if this would be implemented in a stable release.

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Postponed (maintainer needs more info) » Needs work

Since the maintainer wants tests I set it to "Needs work". I am using that patch and it does work.

amatzies’s picture

amatzies commented
Status: Needs work » Needs review
StatusFileSize
new redirect_with_trailing-2882881-10.patch4.71 KB

I added a unit test for this issue. I refactored the existing test class and added a data provider which provides with both source url and target url. This might be useful in the future.

  • Berdir committed 1aa7894 on 8.x-1.x authored by amatzies 
    Issue #2882881 by purushotam.rai, amatzies: Redirect with trailing slash...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Fixed
Issue tags: -Needs tests

Thanks for the test, looks good. Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.