ecto_fields icon indicating copy to clipboard operation
ecto_fields copied to clipboard

EctoFields.URL does not support URLs with ports in them

Open joeapearson opened this issue 3 years ago • 4 comments

For example:

EctoFields.URL.cast("http://example.com:1234/")
:error

Problem appears to be at https://github.com/jerel/ecto_fields/blob/master/lib/fields/url.ex#L60

joeapearson avatar Mar 15 '21 23:03 joeapearson

Actually, looking at the code it seems like there are probably a great deal of valid URLs that wouldn't be correctly treated. However what EctoFields is doing is a sensible default for a great deal of use cases, was that the original intention?

Suitable fixes that I can think of:

  • Update EctoFields.URL to accept all valid URLs. AFAIK this is much more involved than it sounds. But there might be some work that could be borrowed from https://mathiasbynens.be/demo/url-regex for example.
  • Update EctoFields.URL to accept some sort of per-field configuration allowing a user to customise the validation behaviour of URLs.

joeapearson avatar Mar 17 '21 17:03 joeapearson

I used something like this:

  @spec valid_url?(any()) :: boolean()
  def valid_url?(url) when is_binary(url) do
    case URI.parse(url) do
      %URI{host: nil} -> false
      %URI{scheme: nil} -> false
      %URI{} -> true
    end
  end

  def valid_url?(_), do: false

sitch avatar Apr 30 '21 16:04 sitch

EctoFields.URL also doesn't support IP addresses for a host.

EctoFields.IP.cast("http://1.1.1.1")
:error

brettinternet avatar Oct 26 '21 04:10 brettinternet

I've added support for ports and IP addresses via these commits https://github.com/jerel/ecto_fields/compare/7d39c880a68e897bad0d89e97ac0ef0078fe1221...master

@joeapearson Yes, the idea is to have sensible handling for most use cases. In your estimation is the current implementation too restrictive or too permissive?

jerel avatar Nov 22 '21 05:11 jerel