[Form] UrlType should not add protocol to emails #43707
Conversation
01931e1 to
5e23cee
Compare
5e23cee to
320c757
Compare
|
Hey! I think @fre5h has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
|
Trying to detect what probably is an email address and what not, is a good idea in principle. But it will never work 100%, since e.g. So I would say: UrlType is essentially just a nice UX feature for "lazy" end-users ;-) So I would only prepend the |
| public function onSubmit(FormEvent $event) | ||
| { | ||
| $data = $event->getData(); | ||
|
|
||
| if ($this->defaultProtocol && $data && \is_string($data) && !preg_match('~^[\w+.-]+://~', $data)) { | ||
| if (preg_match('~^[^:/]+@[A-Za-z0-9-.]+\.[A-Za-z0-9]+$~', $data)) { |
There was a problem hiding this comment.
Maybe we could be even more aggressive and simply skip adding the default protocol if there is an userinfo part in the URL. Since entering an URL with userinfo is quite rare, forcing to specify the protocol in this case would be a big decrease in user experience.
There was a problem hiding this comment.
You mean "would not be a big decrease in user experience"
There was a problem hiding this comment.
~^[^:/?@]++@~ would fit
There was a problem hiding this comment.
Could be ~^([^:/?@]++@|[^./]+$)~ to exclude values like no, that would be converted to a valid url.
| /** | ||
| * @param bool $skipEmail the URL scheme is not added to values that match an email pattern | ||
| */ | ||
| public function skipEmail(): void |
There was a problem hiding this comment.
I would suggest to replace this setter by a constructor argument
|
I understand why this has been proposed, but I don't like adding a new required option to the constraint. It's going to make all this super verbose and noisy. Instead, I suggest that ppl stop entering emails in a field that asks for a URL Alternatively, what about just throw the deprecation when this happens, aka without the option? |
|
Oh, and ideally we'd switch to https by default in v6! |
|
I agree that the new option is a very preventive way to change the behaviour, while providing an optin for this fix asap. Without this option, is it better to change the behaviour in 5.4 or 6.0 ? |
b3c58ec to
0501d56
Compare
|
That's for 6.1. |
|
@nicolas-grekas if we want to change the UrlType, I'd rather change the In my own projects, I added a FormTypeExtension to change the default value of the option. |
This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [Form] UrlType should not add protocol to emails | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #43506 | License | MIT | Doc PR | n/a Replace #43707, since there is a bug in the `UrlType` auto-prepend behavior. The regex was suggested by `@nicolas`-grekas #43707 (comment) Commits ------- 4f99449 [Form] UrlType should not add protocol to emails
UrlTypefix URLs by adding the protocol (http://) to values that does not contains one. This is clever until users type their email in the field. Values likecontact@example.comget converted tohttp://contact@example.comwhich is a valid URL.Fixing this behavior being a breaking change, this PR adds the option
default_protocol_skip_emailthat needs to be set totrueto disable fix on email-like values.In 6.0, the new option will be deprecated and email-like values will always be ignored.
URLs that are still fixed:
user:pass@example.comuser@example.com/path