UrlValidator should mark URLs w/ crap in them as invalid. #134
Conversation
|
It's not immediately clear to me why Travis barfed on this PR -- the 🔴 tests do not at first glance appear to be related to the changes I made, but I'm happy to be convinced otherwise and to make the changes to get this 💚. |
There are tests that rely on live, 3rd party servers so they fail quite often. 😞 If those are the only ones failing I would not worry about it. |
| end | ||
| raise URI::InvalidURIError unless URI.parse(value).kind_of?(URI::HTTP) | ||
| rescue URI::InvalidURIError | ||
| record.errors[attribute] << (options[:message] || "is an invalid URL") |
There was a problem hiding this comment.
Raising an exception and then immediately rescuing it seems bad. What if you separated the URL parsing from the bit dealing with the validation error?
unless valid_url?(value)
record.errors[attribute] << (options[:message] || "is an invalid URL")
end
def valid_url?(value)
URI.parse(value).kind_of?(URI::HTTP)
rescue URI::InvalidURIError
false
end
|
Thanks for your contribution! 💖 |
Currently, if you enter a string like 'http://example.com/ (this is kinda crappy)' as your website, the UrlValidator throws which was not previously rescued. The validator now raises that error in two cases: 1. When there is a crappy URL per above; and 2. When a non-HTTP URI is entered as a website
mjgiarlo
force-pushed
the
check_for_different_sort_of_invalid_URIs
branch
from
9620cfc
to
6f46361
Compare
|
Changes made, @phiggins. Could use another review. |
|
Looks good to me. Thanks! 👍 |
|
I just deployed this. Please let me know if everything's not working as you'd expect! |
|
Worked like a charm. Thanks, @phiggins! |
Currently, if you enter a string like 'http://example.com/ (this is kinda crappy)' as your website, the UrlValidator throws
URI::InvalidURIErrorwhich was not previously rescued. The validator now raises that error and marks the record as invalid in two cases: