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
Issue fork redirect_after_registration-3482631
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
Comment #2
anybodyComment #3
anybodyComment #4
anybodyComment #7
zaryab_drupal commentedUpdated 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.
Comment #8
anybodyThanks @zaryab_drupal! What about using
'#type' => 'path',for the form field?Comment #9
zaryab_drupal commentedYou'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.
Comment #10
anybody@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?
Comment #12
anybody@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:
<front>(unsure, please try, else leave out)Incorrect:
non-existing-path?I'd prefer the
'#type' => 'path',one - and you?Comment #14
lrwebks commentedAfter a bit of testing, it seems that both approaches work identically in practice. Considering that
pathalready 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.comor 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.Comment #15
anybodyThanks @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?
Comment #16
lrwebks commentedIt appears that the
pathform element is only ever used in theContactFormEditForm.phpthat you also linked in the issue summary, where it is then additionally validated in thevalidateForm()function via thepathValidator, (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 thepathValidatoralso has this weird behavior, I think that this would not be the right solution for us…Comment #18
lrwebks commentedHopefully 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.Comment #19
anybodyGreat work @zaryab_drupal and @lrwebks! All tests are green! 🎉
Comment #21
anybody