Problem/Motivation

In \Drupal\redirect\Form\RedirectForm::prepareEntity we do

      if (!empty($redirect_url)) {
        try {
          $redirect->setRedirect($redirect_url, $redirect_query, $redirect_options);
        }
        catch (MatchingRouteNotFoundException) {
          $this->messenger()->addMessage($this->t('Invalid redirect URL %url provided.', ['%url' => $redirect_url]), 'warning');
        }
      }

But MatchingRouteNotFoundException is not thrown but the called code.

Steps to reproduce

We should test set a redirect to an invalid URL and we should also improve the code comments in

  public function setRedirect($url, array $query = [], array $options = []) {
    $uri = $url . ($query ? '?' . UrlHelper::buildQuery($query) : '');
    $external = UrlHelper::isValid($url, TRUE);
    $uri = ($external ? $uri : 'internal:/' . ltrim($uri, '/'));
    $this->redirect_redirect->set(0, ['uri' => $uri, 'options' => $options]);
  }

Because $external = UrlHelper::isValid($url, TRUE); is very odd. What is happening here is if the URL is not an valid absolute URL we assume it is an interal URL and slap 'internal:/' on the front of it... that feels like code that needs a comment :)

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork redirect-3542970

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review

The code in question was mostly modified to work this way in #2845884: Redirect setRedirect only saves internal paths - I think the use of isValid() to detect externalness is wrong. We should be using UrlHelper::isExternal(). I think there is a further follow-up to consider whether or not we should allow invalid external URLs to be set. In my opinion this should cause an error.

berdir’s picture

Status: Needs review » Needs work

Looks sensible, fine with the change, just needs some phpcs fixes.

anirudhsingh19’s picture

Assigned: Unassigned » anirudhsingh19

working on it!

anirudhsingh19’s picture

Assigned: anirudhsingh19 » Unassigned
Status: Needs work » Needs review

Resolved all phpcs errors, pipeline is fixed now.

  • berdir committed 156ced40 on 8.x-1.x authored by alexpott
    Issue #3542970 by alexpott, anirudhsingh19, berdir:...
berdir’s picture

Status: Needs review » Fixed

Thanks, merged.

Status: Fixed » Closed (fixed)

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