Validation issue on adding url redirect

Created on 27 May 2019, over 6 years ago
Updated 4 April 2023, over 2 years ago

Steps to reproduce

1.Navigate to admin/config/search/redirect/add
2. First add the TO field value,
3.Then click the PATH field and fill the values
4. Without using tab in the keyboard or clicking outside of the field. Click the save button(after filling the PATH field without clicking outside of that field, directly click the save button).
5. we will getting validation error saying that Path field is required.

Another more fluid way to replicate is to start at the target node, click Add redirect URL and you land on the pre-populated form. Clicking in Path, entering the URL and hitting Enter at this point feels 'natural' and reliably reproduces the issue.

๐Ÿ› Bug report
Status

Needs review

Version

1.0

Component

Code

Created by

๐Ÿ‡ฎ๐Ÿ‡ณIndia N.kishorekumar

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States tbcs

    Another +1 for #43 ๐Ÿ› Validation issue on adding url redirect Needs review

  • Status changed to RTBC over 2 years ago
  • Assigned to Kristen Pol
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States Kristen Pol Santa Cruz, CA, USA

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

  • Issue was unassigned.
  • Status changed to Needs review about 2 years ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States Kristen Pol Santa Cruz, CA, USA

    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.

  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 2 years ago
    63 pass
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

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

    Fixing comment lengths.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States Kristen Pol Santa Cruz, CA, USA

    Thanks @acbramley :)

    No more manual testing is needed.

    Still needs a proper code review.

  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 10.1.x + Environment: PHP 8.1 & pgsql-14.1
    last update about 2 years ago
    62 pass, 2 fail
  • First commit to issue fork.
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia kalash-j jaipur

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

  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 10.1.x + Environment: PHP 8.1 & pgsql-14.1
    last update about 2 years ago
    63 pass
  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 5.7
    last update about 2 years ago
    63 pass
  • 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.

  • Status changed to Needs work almost 2 years ago
  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 10.1.4 + Environment: PHP 8.1 & pgsql-14.1
    last update over 1 year ago
    63 pass
  • First commit to issue fork.
  • 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.

  • Status changed to Needs review over 1 year ago
  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update over 1 year ago
    63 pass
  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand jweowu

    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).

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States daddison

    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. ๐Ÿ‘๐Ÿป

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    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.

  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand jweowu

    I think it's in scope insofar as it's a regression of sorts -- the unpatched module doesn't hinder the user in the situation I've raised because the unhelpful warning message 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 now 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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States capysara

    As noted in #9, this is a core issue ๐Ÿ› Submitting a form during an ajax request: field data not in $_POST. Needs work . 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 ๐Ÿ› Submitting a form during an ajax request: field data not in $_POST. Needs work 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.

  • ๐Ÿ‡ฏ๐Ÿ‡ดJordan Rajab Natshah Jordan

    Attached a static redirect--2024-08-11--3057250-79.patch file
    After Redirect 8.x-1.10 โ†’ was released

    I was not able to push changes to the issue fork

  • Status changed to RTBC 10 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mark_fullmer Tucson

    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.

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    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.

  • Pipeline finished with Failed
    9 months ago
    Total: 207s
    #401129
  • Pipeline finished with Success
    9 months ago
    Total: 240s
    #401134
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    MR up and green.

  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand jweowu

    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 ๐Ÿ› Validation issue on adding url redirect Needs review

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

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

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

  • Status changed to Needs review 7 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

    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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States katebron

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

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mark_fullmer Tucson

    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!

  • Pipeline finished with Success
    6 months ago
    Total: 203s
    #474365
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany itothegore

    https://www.drupal.org/project/redirect/issues/3057250#comment-15723568 ๐Ÿ› Validation issue on adding url redirect Needs review

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

  • ๐Ÿ‡ท๐Ÿ‡ดRomania reszli Tรขrgu Mureศ™

    tested latest MR with drupal 10.4.3 and redirect 1.11

  • Status changed to RTBC 4 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mark_fullmer Tucson

    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!

  • ๐Ÿ‡ฏ๐Ÿ‡ดJordan alaa abbad

    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.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 184s
    #573056
  • Pipeline finished with Failed
    about 2 months ago
    Total: 190s
    #573066
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

    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.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 187s
    #573099
  • Pipeline finished with Success
    about 2 months ago
    Total: 187s
    #573102
  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    Posted a review.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States joegl

    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.

  • ๐Ÿ‡ฏ๐Ÿ‡ดJordan Rajab Natshah Jordan
  • Pipeline finished with Failed
    about 1 month ago
    #586068
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 242s
    #586069
  • Pipeline finished with Failed
    about 1 month ago
    #586075
  • Pipeline finished with Failed
    about 1 month ago
    Total: 199s
    #586084
  • ๐Ÿ‡ฏ๐Ÿ‡ดJordan Rajab Natshah Jordan

    rajab natshah โ†’ changed the visibility of the branch 3057250-fixes-with-coding-standard-and-DrupalPractice to hidden.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom tonypaulbarker Leeds

    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?

  • ๐Ÿ‡ฏ๐Ÿ‡ดJordan Rajab Natshah Jordan

    Tony, still testing with one more MR under Drupal 10.5.x and 11.2.x
    ๐Ÿ› Redirects from aliased paths aren't triggered Needs work
    ๐Ÿ› Validation issue on adding url redirect Needs review
    Noticed the big changes

          "drupal/redirect": {
            "Issue #2879648: Redirects from aliased paths aren't triggered":
            "https://raw.githubusercontent.com/vardot/varbase-patches/refs/heads/patches/redirect--2025-08-30--2879648--mr-109.patch",
            "Issue #3057250: Validation issue on adding url redirect":
            "https://raw.githubusercontent.com/vardot/varbase-patches/refs/heads/patches/redirect--2025-08-30--3057250--mr-133.patch"
          },
    
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    acbramley โ†’ changed the visibility of the branch 8.x-1.x to hidden.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 368s
    #589509
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    @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.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 179s
    #589534
  • Pipeline finished with Success
    about 1 month ago
    Total: 180s
    #589535
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley
  • ๐Ÿ‡ฏ๐Ÿ‡ดJordan Rajab Natshah Jordan

    Thanks, @acbramley, for pointing that out, and your follow-up fixes.
    Taken
    You are totally right, I got mixed up with fixes from ๐Ÿ› Redirects from aliased paths aren't triggered Needs work
    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.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium matdemeue

    Patch applies on 1.12 and works for me!

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland megadesk3000

    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
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    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.

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland megadesk3000

    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)
    
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom tonypaulbarker Leeds

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

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom tonypaulbarker Leeds

    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."

    Including the file here.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom tonypaulbarker Leeds
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    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.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom tonypaulbarker Leeds

    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 :)

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States amanire

    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... โ†’ .

Production build 0.71.5 2024