Validation issue on adding url redirect

@N.kishorekumar, Thanks for the patch. Your patch is working fine as it solves the issue. I reproduced the issue in my local using the mentioned steps. PFA

Duplicate issue - #2807375

Thanks. This Resolves the issue.
But, is it a good idea even if there is no change in the input, the ajax callback gets triggered every time mouse leave event is triggered?

Yeah, that doesn't seem like a good idea, the ajax requests is being triggered dozens of times when moving the mouse over that field.

This is a core issue: https://www.drupal.org/project/drupal/issues/1736308

Seems the only way to resolve this would be to roll a small bit of js code that does the status check manually (using a new route etc) rather that using #ajax.

This might be too radical an approach, but I don't see why we need the ajax functionality here at all (I'm probably overlooking X number of scenario's where this is actually useful).

This moves the 'existing path' logic to the form validation, the 'existing redirect' logic was already there too, so no need to move that.

Now without the test classes....

Patch #11 worked for me. Didn't need the AJAX functionality and this simplified things.

I couldn't get #11 to apply. Here is a version against 8.x-1.5 in case anyone needs it.

Working with patch in #13. Didn't test #11. Thanks, all.

Working with patch in #13. Path #11 couldn't apply. Thanks

+++ b/src/Plugin/Field/FieldWidget/RedirectSourceWidget.php
@@ -48,49 +48,6 @@ class RedirectSourceWidget extends WidgetBase {
-        // Warning about the path being already redirected.
-        $parsed_url = UrlHelper::parse($source_path);
-        $path = isset($parsed_url['path']) ? $parsed_url['path'] : NULL;
-        if (!empty($path)) {
-          /** @var \Drupal\redirect\RedirectRepository $repository */
-          $repository = \Drupal::service('redirect.repository');
-          $redirects = $repository->findBySourcePath($path);
-          if (!empty($redirects)) {
-            $redirect = array_shift($redirects);
-            $element['status_box'][]['#markup'] = '<div class="messages messages--warning">' . $this->t('The base source path %source is already being redirected. Do you want to <a href="@edit-page">edit the existing redirect</a>?', ['%source' => $source_path, '@edit-page' => $redirect->url('edit-form')]) . '</div>';
-          }
-        }
-      }

That's not the same message, only the base path is compared here, and it's a warning, something that we can't replicate as a validation error.

Not quite sure if it's really needed.

We also need to fix the tests.

It's actually the same with the other message. It's not a error right now, just a suggestion. There are sometimes cases when you want to do it anyway.

I don't like the ajax stuff here at all and this test is actually currently failing on D9, but I don't think this is really an option.

@Berdir, thanks for taking a look.

but I don't think this is really an option.

By 'this' you mean removing the AJAX bit? Would it be an option to add a checkbox 'do this anyway' when validation fails? Would that tackle the edge case you are concerned about?

Yeah, I mean replacing the ajax warnings with a validation error by "this".

Asking for a verification might be an option but you might realize that this is surprisingly hard with form API as there is no obvious way to alter the form *after* it has been validated. In a client project, what we did in a similar case is just let the form be submitted again and if hte values haven't changed since the last submission, we didn't validate again.

Issuing an updated patch that can be applied with latest version of redirect.

Functionally, the patch is working.

The fail state is due to code standards. Once those are sorted I would consider this to be RTBC.

I also updated the Issue Summary with a second path to reproduce the issue.

Let me working on #24

It looks like some new code changes have been commited. When I try running on local I am not seeing the same failure. So adding one more test run on 8.9.x and 9.1.x to check new failures.

After checking all the code and comments, it looks we like also need to rewrite Test as we are using form validation instead of ajax here. Will be happy to know the approach to proceed accordingly.

Meantime I am adding the coding standard fixies as mentioned on #24, Please review the updated patch.

It looks like the url alias url was changed at some point to 'entity.path_alias.add_form'. I made that change and removed some unused code (use lines in the widget and the element ajax callback lines in the form).

I can confirm #29 works with drupal 9.1.
I guess the route change to entity.path_alias.add_form has been introduced in 8.8.0: https://www.drupal.org/node/3013865

Re-roll of #29 in an attempt to get the testbot to apply it.

This patch seems to resolve the bug for me in Drupal 9.1.

None of those codesniffer changes are relevant to this patch, so they shouldn't be included here. Moving back to Needs Review.

A separate coding standards patch for the codebase in general would be appropriate. (Edit: and indeed exists at #2957751: Maintaining drupal coding standards. Also relevant here is #3214466: Add a drupalci.yml file in order to inhibit coding standards checks (as these are not being addressed)).

I presume we can just remove the failing test for the (now-removed) AJAX behaviour. That was testing code which has been eliminated in this refactor.

What about something like this? Using setRebuild and a warning message instead and form_state's storage to make sure we only trigger it once as per #20

I wasn't able to get the full test suite to pass with seemingly random failures in a few spots but that just may be my local setup.

Removes double save

We need to make sure the warning is only shown when there aren't any other errors in the form. Also need to delete the test redirect so assertions further down don't fail.

Here's the interdiff, wouldn't let me attach it before for some reason

Fixes final fail 🤞

Tagging for BSI

+++ b/src/Form/RedirectForm.php
@@ -143,6 +144,30 @@ class RedirectForm extends ContentEntityForm {
+    // If creating new URL throw a one off warning when the source path is
+    // a valid path.
...
+        $this->messenger()->addWarning($this->t('The source path %path is likely a valid path. It is preferred to <a href="@url-alias">create URL aliases</a> for existing paths rather than redirects.', [

The warning doesn't tell the end user that this is a one-time warning, maybe we should let the user know?

Also, what happens when the user gets the warning, then changes the source path, and the new source path should get the warning too? I think that will just go through to submit right? Maybe set 'redirect_source_warning' to the source path instead of just TRUE and validate that it didn't change?

very good points @Lendude

BSI is core only, woops!

While waiting for this fix to finalize and be released, here is a simple workaround.

While playing in D9.0.3 locally, I'm having this same issue.

I discovered if you hit the browser's back button while in Chrome, the path field is auto filled with my text and the error is cleared. I then re-submit and it takes. I tried this method 3 times on a test article and it worked each time.

Tested patch from #43.

With #43 was impossible to create a redirect from user/register to https://example.com, because that source path is denied to an authenticated user, and the catch for AccessDeniedHttpException was removed. I think this should be restored; we shouldn't prevent creating redirects from paths which are inaccessible to the current user? Interested though to understand why the AccessDeniedHttpException was removed.

Patch here addresses that. Also updated a couple of comments for clarity, changed "new URL" to "new Redirect" since we're referring here to an existing valid path.

EDIT: Tests failed on this patch, so don't use that for now :)

+1 for #43. Worked well.

Leaving as Needs Work for now, while considerations from @xurizaemon is address, in particular the Access Denied issue.

Re-role for the #49 patch to work with Redirect 1.8

Fixed the test and coding standards.

Patch in #53 works for us. Thanks! Hopefully we can resolve the test failure and get this rolled into an official release soon.

The steps to reproduce the error were a little different for us: I found that simply entering a Path and hitting enter didn't trigger the error. Instead, I have to mouse over the To field and then click on the submit button to submit the form.

The test that failed in #53 passes locally, I can't see anything in there that would break based on the changes in this patch so I'll rerun it.

Patch from #53 also applies and works for me. Thank you.

Patch #53 worked for me. Thank you!

Patch #53 works like a charm, thank you also for pointing how to reproduice this bug.

Another +1 for #53

#53 is working well for me!

Assigning to myself as I'm triaging all RTBC issues.

Several people have tested #53 and confirmed it's working for them. I do see that the tests were updated so that's good. I have confirmed that patch still applies.

I'm not seeing anyone who obviously reviewed the code in #53. So that's the next step here.

No more manual testing is needed.

I did a quick scan of the code but it still needs a proper code review.

Some nitpicks to fix:

1. +++ b/src/Form/RedirectForm.php
   @@ -143,6 +145,32 @@ class RedirectForm extends ContentEntityForm {
   +    // If creating a new Redirect and the source path is an existing path, recommend an alias instead.
   

   More than 80 chars.

2. +++ b/src/Form/RedirectForm.php
   @@ -143,6 +145,32 @@ class RedirectForm extends ContentEntityForm {
   +      //
   

   Empty comment.

3. +++ b/src/Form/RedirectForm.php
   @@ -143,6 +145,32 @@ class RedirectForm extends ContentEntityForm {
   +      // @todo Exception driven logic. Find a better way to determine if we have a valid path.
   

   More than 80 chars.

The code in #53 is basically the same as all previous patches, which have been through several rounds of review.

Fixing comment lengths.

Thanks @acbramley :)

No more manual testing is needed.

Still needs a proper code review.

The patch 3057250-65.patch is working fine , i have tried and tested it .

Reviewed code. One issue with AccessDeniedHttpException - with the catch in place, the user can create the redirect, but will not receive the warning that it's a valid path or the suggestion to use an alias instead. Is it possible for the user to get AccessDeniedHttpException for a path that doesn't exist?

If not, the same warning could be added inside the catch.

If so, the message could be modified to indicate that *if* it's a valid path an alias is recommended.

ETA: VS Code is also indicating some formatting standards:

• put use statements in alphabetical order
• adding class doc comment
• use null coalescing operator instead of ternary
• misc: 1 space after closing parenthesis, end inline comments with full stops, 1 blank line after functions, etc.

Applied existing patch #65 to the fork and changed the warning message as suggested in #69 also added that the message shows on both access denied and if the route exists. Any non existant routes still have no message. I was not entirely sure where i could find the formatting standards so i did not make any changes to fix those.

I'm not sure what's going on in #72 as the issue fork commit is identical to #65.

This patch is the same as #65 plus these additional changes:

* Allow the user to add a redirect from an internal path to an external URL without an unhelpful validation warning to consider using a URL alias instead.
* Remove some use statements from RedirectSourceWidget.php (their usage having been eliminated in previous revisions of this patch).

I applied the patch at #74 to the 8.x-1.x branch.

The module no longer presents the "unhelpful validation warning to consider using a URL alias instead" when adding a redirect from an internal path to an external URL.

The patch removes the unnecessary use statements, as indicated in the second point. 👍🏻

Allow the user to add a redirect from an internal path to an external URL without an unhelpful validation warning to consider using a URL alias instead.

Is this in scope for this issue? It sounds like it's separate.

I think it's in scope insofar as it resolves a problem introduced by the patch for this issue.

Although the unpatched module does display the same unhelpful warning in the scenario I've raised, it doesn't hinder the user in that situation because the warning was displayed automatically using AJAX -- before the form was even submitted.

With the patch, the warning doesn't happen until the user submits the form, which then means they need to submit the form a second time to make it stick, when previously once was enough.

I.e. Because we've removed the AJAX in this patch, I think we need to also remove any false-positive warnings.

As noted in #9, this is a core issue. The description in that ticket differs from this description, but it comes down to the same thing.
Can confirm that the core patch in comment 57 addresses the issue reported here.
I know that core issues can drag on forever (12 years and counting), but ideally any further efforts should go towards moving along the resolution there instead of in this module (though, as I noted in that ticket, I'm not convinced that core is currently applying the best fix).

Attached a static redirect--2024-08-11--3057250-79.patch file to work
After Redirect 8.x-1.10 was released
to be used with composer patches

I was not able to push changes to the issue fork

This fixes this long-running bug, and I agree with #78 that it is prudent to fix this in the contrib module rather than wait for core to fix the underlying issue. Marking as RTBC.

That's OK with me, but this has no merge request, so tests didn't run. Please ensure there is a merge request with the latest and tested version of the patches here.

MR up and green.

I note that the functional change in #74 has been lost (it wasn't included in #79).

https://www.drupal.org/project/redirect/issues/3057250#comment-15524338

Perhaps that issue was dealt with in some other way in the interim. Either way, it probably needs to be reviewed.

@jweowu sorry I missed that, feel free to contribute to the MR.

This is a major bug because if you spend time entering a url and then press submit you lose the URL you've just entered which is quite frustrating and data loss of a sort because you have to re-enter it completely. Also the problem is very bad if you add a redirect from an existing node because you have no reason to enter information in any other field.

If anyone is here looking for a temporary workaround, using the tab key instead of a mouse/trackpad worked for me.

I note that the functional change in #74 has been lost (it wasn't included in #79).

I added the additional check to the MR via https://git.drupalcode.org/project/redirect/-/merge_requests/133/diffs?c... .

This is again ready for review!

https://www.drupal.org/project/redirect/issues/3057250#comment-15723568

Works perfectly with Drupal version 10.4.5 and Redirect module version 1.11.0.

tested latest MR with drupal 10.4.3 and redirect 1.11

Just bumping visibility on this issue. It's now RTBC. This would be a great item to merge and included in a new release, given that it has been indicated as a "Major" level bug. Thanks for the consideration, maintainers!

Redirects with source paths starting with /media result in a 404 after saving. This happens because the module attempts to resolve the path, and if no route match is found (as with file-system paths), it throws an uncaught ParamNotConvertedException.

I've fixed the MR to redo the check if the change the source path and I've added test coverage to ensure that you can save a redirect that overrides a path still.

Posted a review.

Looks like the 1.12.0 release introduces merge conflicts into this merge request and any patches included here fail to apply. I am locking redirect to 1.11.0 for now and may re-visit this if/when I get a chance.

Thank you for this work everyone. As #95 it looks as though old patches cannot apply to 1.12.0. Is this ready for a new patch after the latest commits from @RajabNatshah or still needing some more work before review?

Tony, still testing with one more MR under Drupal 10.5.x and 11.2.x
#2879648: Redirects from aliased paths aren't triggered
#3057250: Validation issue on adding url redirect
Noticed the big changes in both issues

@rajab natshah please be careful when resolving conflicts that you don't break the changes. You had reverted large chunks of the MR here and left it in a broken state. It's usually best to just do a single merge commit when resolving conflicts, or try to rebase the branch.

It then becomes very hard for other contributors because we must then painstakingly go through each file and compare changes before the conflict resolution to see what was been broken.

Thanks, @acbramley, for pointing that out, and your follow-up fixes.
Taken
You are totally right, I got mixed up with fixes from #2879648: Redirects from aliased paths aren't triggered
As I had both fork issues in the same folder.
I will use the rebase method next time and create a separate folder for each issue fork.

MR from #102 applies on 1.12 and works for me!

For me the the MR133 does not apply anymore (tested against 1.11.0 and 1.12.0):

Gathering patches for dependencies. This might take a minute.
  - Installing drupal/redirect (1.12.0): Extracting archive
  - Applying patches for drupal/redirect
    https://git.drupalcode.org/project/redirect/-/merge_requests/133.patch (#3057250: Validation issue on adding url redirect)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://git.drupalcode.org/project/redirect/-/merge_requests/133.patch

I have used the MR diff as a patch against 1.12 and #105 reports it is working for them as well.

FYI it's not a good idea to use .diff MR urls directly in your composer patch files. This is a security risk as anyone can gain push access and push code to it which could cause all sorts of issues.

Instead, download the diff and store it locally and reference the local patch file instead.

EDIT: Also just noticed you are using the .patch Url, that will be why. Use .diff instead.

Thank you acbramley, you are completely right about the local patch files.
In addition i was not aware of the .diff url which indeed applys against 1.12.0.

Gathering patches for dependencies. This might take a minute.
  - Installing drupal/redirect (1.12.0): Extracting archive
  - Applying patches for drupal/redirect
    patches/redirect/3057250-mr133.diff (#3057250: Validation issue on adding url redirect)

@acbramley thank you for resolving those conflicts. Regarding guidance in #107 - any reason for us not to upload a patch from the MR diff here for folks to use ( as was the case previously in #79 ) ?

I see the guidance (Making changes locally point 18) still says this is okay: "Consider uploading a patch file to the issue in addition to making a merge request, especially if your changes solve a problem. Although the recommended way to contribute source code changes is via merge requests, patches can be easier for others to install via Composer."

Edit: Re-uploaded with an improved filename

any reason for us not to upload a patch from the MR diff here for folks to use

The main reason is it confuses contributors, if someone sees a patch they may start contributing fixes via patches again rather than the MR. It then becomes hard to track which changes have been made and whether they have been pushed to the MR or not. It's also very frowned upon in the core queue as it can confuse the needs-review-bot which will put an issue into needs work if the patch no longer applies even if there is an MR, therefore I usually discourage it.

The main reason is it confuses contributors, if someone sees a patch they may start contributing fixes via patches again rather than the MR. It then becomes hard to track which changes have been made and whether they have been pushed to the MR or not. It's also very frowned upon in the core queue as it can confuse the needs-review-bot which will put an issue into needs work if the patch no longer applies even if there is an MR, therefore I usually discourage it.

Good to know! Thanks :)

FYI it's not a good idea to use .diff MR urls directly in your composer patch files. This is a security risk as anyone can gain push access and push code to it which could cause all sorts of issues.

Yes! or on a lesser note, I have seen it result in surprise breaking code changes. Documentation is here:
https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr....

Patched and tested. This looks good to me.

RTBC+1
Tested MR133, thank you :)

With out the patch , in MR133, I get the validation error...

I'm on Drupal 11.3.5

"drupal/redirect": "1.x-dev@dev",

using the patch from the MR133 ... and this still gives me problems.

Now it results in a 404 being generated.

On this page : /admin/config/search/redirect/add

Clicking SAVE results in a broken, looking 404 page inside of Gin.

This is pulling the content from the "404" page I have on the front end, in the gin theme.

Watchdog has a page not found error.

Not sure about #117, maybe a separate issue that only appears once the validation issue is resolved, this was tested by multiple people. I suggest you create a new issue, with clear steps to reproduce not on your site but a new install to rule out that it's an issue with your specific setup.

Finally merging this. Long issue with many people, so challenging to get credits right, I tried. If I missed you, let me know through the contact form to not notify 75 followers about it.

Same issue as #117 and #92.
#92 actually has the solution posted in redirect--2025-08-07--3057250-80.patch but it doesn't appear to be included in your merge.

Issue creating a redirect from:
- a deleted /node/123
- a deleted /media/123
- or in my case a deleted /document/123 -- route from media_entity_file_redirect
Goes to a 404 after submitting the form.

Drupal 10.6.10 and redirect 8.x-1.13

A small patch takes care of my use case.