Problem/Motivation

Currently the redirect path is used as-is without validation.

It should be validated first and I think it could use #type => 'path'.
Here's an example from contact module.

$form['redirect'] = [
      '#type' => 'path',
      '#title' => $this->t('Redirect path'),
      '#convert_path' => PathElement::CONVERT_NONE,
      '#default_value' => $contact_form->getRedirectPath(),
      '#description' => $this->t('Path to redirect the user to after submission of this form. For example, type "/about" to redirect to that page. Use a relative path with a slash in front.'),
    ];

We should add a test for the settings form and ensure the validation works and the resulting path in config is as expected (not converted to absolute or something like that).

Furthermore the value should be validated, so if the form element doesn't do that out of the box, we need to validate the form value sth. like this (with DI):

  // Validate the URL.
  $path_validator = \Drupal::service('path.validator');
  $url = $path_validator->getUrlIfValid($url);
  
  if ($url) {
    // Set the redirect URL if it is valid.
  } else {
     // Mark form error
  }

But I hope the form element allows to do that via a parameter or in general automatically.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

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

anybody created an issue. See original summary.

anybody’s picture

Issue summary: View changes
anybody’s picture

Issue tags: +Novice, +Needs tests
anybody’s picture

Category: Task » Feature request

zaryab_drupal made their first commit to this issue’s fork.

zaryab_drupal’s picture

Status: Active » Needs review

Updated the RedirectAfterRegistrationConfigForm to implement proper dependency injection for the path validator service. Added validation for the destination field to ensure it is a valid internal path.

anybody’s picture

Thanks @zaryab_drupal! What about using '#type' => 'path', for the form field?

zaryab_drupal’s picture

You're correct. By avoiding the use of '#type' => 'path', you enable users to enter relative paths such as /abc rather than requiring a complete URL like https://www.example.com/abc. Instead, using '#type' => 'textfield' provides greater flexibility and aligns more closely with Drupal's standard handling of internal paths.

anybody’s picture

@zaryab_drupal I can't really believe that explanation. where is that from?

See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21... for code. Here only internal URLs should be allowed.

I think CONVERT_ROUTE might be interesting, so we might be able to use a route in further processing. But yes, let's implement that in a separate, experimental branch here.

Would you like to try?

anybody’s picture

@lrwebks: Could you please test both MRs manually and check which one works better? Also please add tests with a correct and with an incorrect path entered.

Examples:

Correct:

  • /
  • /admin
  • Eventually: <front> (unsure, please try, else leave out)

Incorrect:

I'd prefer the '#type' => 'path', one - and you?

lrwebks made their first commit to this issue’s fork.

lrwebks’s picture

Status: Needs review » Needs work

After a bit of testing, it seems that both approaches work identically in practice. Considering that path already is a valid form element, I would definitely use this one as it's existence to me proves that this approach is the Drupal way in contrast to #9.

What I noticed however in both implementations: When entering an absolute URL like https://www.example.com or similar to that, instead of giving a warning, the page says that the config was saved, but the input is replaced by the previous valid path, e.g., `/admin` that was saved before. That feels like unexpected / unwanted behavior to me. What do you think?

I'll implement the necessary tests into MR!13 (the one with 'type' => 'path' now.

anybody’s picture

Thanks @lrwebks! Yes, that also looks unexpected to me. Please check out other places in core that use this type. How do they implement the validation or save?

So it treats the absolute URL as valid path? What's treated as invalid path then?

lrwebks’s picture

It appears that the path form element is only ever used in the ContactFormEditForm.php that you also linked in the issue summary, where it is then additionally validated in the validateForm() function via the pathValidator, (makes no sense to me, this works in our module without it being there entirely) and then submitted. But since I know first-hand that the approach with the pathValidator also has this weird behavior, I think that this would not be the right solution for us…

lrwebks changed the visibility of the branch 3482631-validate-redirect-path to hidden.

lrwebks’s picture

Status: Needs work » Needs review

Hopefully everything should work now! The problem was related to the fact that the setting was named destination, the name was already taken somewhere in the parent hierarchy.

anybody’s picture

Status: Needs review » Reviewed & tested by the community

Great work @zaryab_drupal and @lrwebks! All tests are green! 🎉

anybody’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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