Validation issue on adding url redirect

Created on 27 May 2019, over 5 years ago
Updated 4 April 2023, over 1 year 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

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 1 year ago
  • #53 is working well for me!

  • 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 over 1 year 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 over 1 year 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 over 1 year 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 1 year ago
    63 pass
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 5.7
    last update about 1 year 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 about 1 year ago
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.4 + Environment: PHP 8.1 & pgsql-14.1
    last update 11 months 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 11 months ago
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update 9 months 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 16 days 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.

Production build 0.71.5 2024