Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Validator] Do URL validation according to RFC's #12514

Closed
wants to merge 1 commit into from

Conversation

boekkooi
Copy link
Contributor

Q A
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? no
Fixed tickets #10467
License MIT
Doc PR -

This PR is still very rough but I just want to throw it out there so people can comment etc.

This PR makes the URL validator RFC aware and more configurable (question should it be more configurable?).

The idea is that we take RFC3986 as a base for the validator and let people configure it according to there needs.
I was mainly thinking of not allowing ip addresses or using custom host name constraints based on different RFC's.

One of the main problems with the new implementation is that none of the RFC's (as far as i can see) allow UTF-8 characters in the host name and this gives some trouble with this current tests. (aka IDN domains).

P.S. the RFC3986 regex can be found at https://gist.github.com/boekkooi/d24fcee6f7082e802ce1/revisions (this is not a full regex since i didn't implement hier-part fully)


$hostPattern = array();
foreach ($hostTypes as $type) {
switch(strtolower($type)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch is not extendable. You can use some prepared mapping array with ex. ['ipv6' => PATTERN_HOST_IP_V6] etc.

@boekkooi
Copy link
Contributor Author

@piotrpasich @stof Thanks for the feedback.

I changed the schema code, it now only checks the scheme when the general structure of the value is correct.
For extending I added a getHostType method.
Also fixed the tests and added IDNA2008/RFC5892 hostname checks.

I'm not really happy with the IDNA2008 support but my knowledge is very much lacking in the area of \p{} regex stuff so any help would be great!

Let me know what you guys think and what should be changed or for example where I should move the const PATTERN_* to.

@boekkooi
Copy link
Contributor Author

@fabpot I don't get the error given here http://fabbot.io/report/symfony/symfony/12514/23fdb09dcf484a9a2c2d9e28bda0904e688db680 Why would I change return null; to return;? and should the suggestion not be to remove return?

@piotrpasich
Copy link

@boekkooi according the PATTERN_* I'm thinking about something like decision manager or voter - create separated classes for each pattern, go to foreach and check if one of them will return true. If this happened, the url is ok.

But I try to figure out the method how to register those classes to the validator to make it more extensible. What do you think @stof?

@boekkooi
Copy link
Contributor Author

boekkooi commented Dec 4, 2015

I'm closing this because I don't have the time to create a nice implementation for this.

@boekkooi boekkooi closed this Dec 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants