- Issue created by @Anybody
- First commit to issue fork.
- Merge request !12Issue #3482631: Validate redirect path and use path form element → (Open) created by zaryab_drupal
- 🇮🇳India zaryab_drupal Bhopal
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.
- 🇩🇪Germany Anybody Porta Westfalica
Thanks @zaryab_drupal! What about using
'#type' => 'path',
for the form field? - 🇮🇳India zaryab_drupal Bhopal
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.
- 🇩🇪Germany Anybody Porta Westfalica
@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?
- Merge request !13Redirect after registration 3482631 3482631 validate redirect path → (Merged) created by zaryab_drupal
- 🇩🇪Germany Anybody Porta Westfalica
@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:
- https://www.example.com
non-existing-path
?
I'd prefer the
'#type' => 'path',
one - and you? - First commit to issue fork.
- 🇩🇪Germany lrwebks Porta Westfalica
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. - 🇩🇪Germany Anybody Porta Westfalica
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?
- 🇩🇪Germany lrwebks Porta Westfalica
It appears that the
path
form element is only ever used in theContactFormEditForm.php
that you also linked in the issue summary, where it is then additionally validated in the validate 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... - 🇩🇪Germany lrwebks Porta Westfalica
lrwebks → changed the visibility of the branch 3482631-validate-redirect-path to hidden.
- 🇩🇪Germany lrwebks Porta Westfalica
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. - 🇩🇪Germany Anybody Porta Westfalica
Great work @zaryab_drupal and @lrwebks! All tests are green! 🎉
-
anybody →
committed c7447ab5 on 1.x authored by
zaryab_drupal →
Issue #3482631 by lrwebks, anybody, zaryab_drupal: Validate redirect...
-
anybody →
committed c7447ab5 on 1.x authored by
zaryab_drupal →