Don't allow adding source paths with trailing slashes, they won't ever work

Note that this is due to a change that was made in #2882881: Redirect with trailing slash is not working. Redirect treats /source and /source/ as two distinct redirects, but the change in that issue prevents /source/ from ever being used.

The patch could use a bit of work to update the wording and logic - will roll a new one this afternoon.

Updated the language of the error message, changed the logic a bit, and updated all the validate functions to operate on a trimmed value of the redirect before testing their use cases. This is important because the value is trimmed before it's saved. We should only check the trimmed version.

Also changing this to a bug since without it, users can be very confused as to why redirects are not working.

Hmm I think those are unrelated test failures.

Hey there,

I've read all the various issues and to be honest can't make sense of them. Is this patch the best way to make business-directory/example/ redirect to a new page on the site?

thanks in advance for the help!

Hi droddis,

No, this patch prevents the use of a trailing forward slash when adding a redirect.

I agree that Redirect should standardize how it stores redirects, and that is should enforce redirects w/out leading and trailing forward slashes.

This came up for our team b/c we implemented fast404 module that queries the redirect table directly before bootstrapping Drupal. See #2838359: Fast404 path checking is incompatible with Redirect module. Standardizing how the redirect is stored will improve compatibility with fas404 module.

Also, in previous fast404 work, see comment #7 #2743965: Fast 404 results in 404s on valid routes when using fast404_path_check, I found that Drupal core is removing leading and trailing slashes when generating path aliases, see PathautoGenerator::createEntityAlias() use of AliasCleaner::cleanAlias().

I'd argue that Redirect should normalize how it stores redirects...without leading and trailing slashes to be aligned with core's alias storage.

Setting to Needs Review to get a maintainer to review this. Looks the status update in #7 did not take.

If Redirect continues to allow adding source paths with trailing slashes, then we should reconsider the patch committed in #2882881: Redirect with trailing slash is not working, which uses $path = trim($path, '/'); to remove leading and trailing slashes in the RedirectRequestSubscriber::onKernelRequestCheckRedirect() before calling RedirectRepository::findMatchingRedirect().

As of right now, if you add a redirect with a trailing slash, redirect does not match the redirect with requested path that includes the redirect.

And, if you're using fast404 with this patch, you're getting fast404 response as expected, b/c it trims the path before querying the redirect table.

I fully agree that we should normalize but I also think that we should simply fix user input when saving instead of making them do something that we can automate.

I could not apply Patch #7 using composer-patches, seems this related to git version, I applied the patch from git and recreate it.

I had to add one more case for sources with a query string.

Sometimes you have a source with trailing slash and query string, for example, source/?query-string=5, This case was not covered by the original patch.

I add the trilling slash case with a query string case to the test.

Wouldn't a simple solution be to add a label "Do not use trailing slashes" under the source field?

Wouldn't a simple solution be to add a label "Do not use trailing slashes" under the source field?

I think a better UX is to not allow a configuration that is not valid. That's what form validations are for

I still prefer to just clean it instead of adding validation, there is no reason to require user interaction, if anything we could do both for an API-level validation. That's what #3032976: Remove trailing spaces from source url is apparently doing, lets combine these two issues, as this adds a test, we just need to adjust the expectation then.

I think this is a major UX issue. We've had lots of users confused thinking redirect was broken because they add the source redirect with a trailing slash

Here's an approach that Berdir wants - stripping the trailing slash for the user instead of throwing a validation error.

I'm removing the trailing slash in the validation method, but maybe it should be in an entity presave? If Berdir agrees I will change it to do that.

As I was writing this, I figured that most of this validation logic should probably happen in entity constraints so that programmatically created redirects can benefit from the same protections. But that's certainly out of scope for this issue.

No tests yet, but I can write them once I get approval on the direction from maintainer.

Re-rolled to work with latest version.

Tested with 2932615-22.patch: and it works.
Also mentioned issue Redirect with trailing slash is not working is fixed and trims the path from both sides(l/r)

RE: #23, the issue you mentioned is already added a related issue here. That issue trims the trailing path from the request path but only when applying the path matching logic to the request. Redirect module still allows users to add a redirect with a trailing slash, even though that redirect will never work.

RE: #24, 'already added a related issue here' - I saw issue in related ones, just mentioned it (because it was discussed above) that issue is already fixed and it allows navigating to path with trailing slashes (ex. test//) and after inbound processing it will trim path to (ex. test) and redirect (if founds matching)...
After applying your patch and try to Edit URL redirect the path will 'rtrim' and save path without trailing slashes... so it works as intended along with fixed related issue

Try this patch it will work for both trailing and without trailing slash path urls.

Try this patch it will work for both trailing and without trailing slash path urls.

I tried applying the patch and noticed when i import an csv that contains redirects with trailing slashes all the redirects are not working but redirects without trailing slashes in the csv file work as expected.

The expected behaviour is that redirects should work if the csv file contains redirects with or without trailing slashes.

Maybe this is an issue in the path_redirect_import module.

I have tried the patch which does not work for me on first submit but if i edit the redirect and save it then works.

I've updated this in an issue branch to include a test, and also return more or less to the approach in #22, but moved that logic to the pre-save method. Using pre-save will address the issue noted above with redirect imports, etc.

Merge request !12 is simple and efficient. Moving to RTBC.

we have tested the patch 38 and it is working like a charm

Makes sense and thanks for the test coverage! Committed to 8.x-1.x.