Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Validator] support protocolless urls validation#24308
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedSep 24, 2017
I'm not sure we can do this change as is: protocol-relative URLs have only a relative meaning. |
MyDigitalLife commentedSep 24, 2017
@nicolas-grekas Good point. I see 2 possible ways to fix this:
I think option 2 is the better one. And to not break BC it should probably by default not allow relative URL's. |
nicolas-grekas commentedSep 24, 2017
Let's go for option 2, with a default that doesn't change the behavior for sure. |
fabpot commentedSep 24, 2017
Can you rebase this PR to get rid of the merge commit? Thanks. |
d1707ac to9085c40Compare| publicfunctiontestValidRelativeUrl($url) | ||
| { | ||
| $constraint =newUrl(array( | ||
| 'relativeProtocol' =>true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should also test absolute urls are still valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The test has bothgetValidRelativeUrls andgetValidUrls as a data provider. That covers both absolute and relative URL's. Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@MyDigitalLife nice :) didnt noticed that.
| publicfunctiongetInvalidRelativeUrls() | ||
| { | ||
| returnarray( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Any reason why we are not using[] instead ofarray
javiereguiluzSep 25, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's becauseSymfony coding standards. We can't easily change the existing code fromarray() to[] ... so using[] only for the new code would be inconsistent and that's why we usearray() everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This started as a patch for 2.8 so I needed to use thearray keyword to not break code in PHP 5.3 seeing this is supported with Symfony 2.x. I definitely prefer the brackets and could changes it.
MyDigitalLife commentedSep 26, 2017
@fabpot The PR has been rebased. |
| } | ||
| $pattern =sprintf(static::PATTERN,implode('|',$constraint->protocols)); | ||
| $protocolPattern ='(%s):'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
non-capturing, i.e.(?:%s):? Might be patched in 2.7 for groups currently inPATTERN. Not a real blocker though.
| $pattern =sprintf(static::PATTERN,implode('|',$constraint->protocols)); | ||
| $protocolPattern ='(%s):'; | ||
| if ($constraint->relativeProtocol) { | ||
| $protocolPattern ='((%s):)?'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about(?:'.$protocolPattern.')? to reuse previous pattern. Also no real reason for capturing here.
nicolas-grekas commentedOct 24, 2017
Moving to 4.1 as 3.4 is in beta, thus closed for new feats. |
Simperfit commentedNov 1, 2017
Could you please rebase against master ? |
be0b1b4 tod845406Comparenicolas-grekas commentedFeb 14, 2018
Thank you@MyDigitalLife. |
…gitalLife)This PR was merged into the 4.1-dev branch.Discussion----------[Validator] support protocolless urls validation| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#24287| License | MITAs specified in issue#24287 URL's starting with no protocol but with just `//` are valid URL's and should be accepted by the validator. This pull request fixes the regex used to validate URL's.Commits-------d845406 [Validator] support protocolless urls validation
Uh oh!
There was an error while loading.Please reload this page.
As specified in issue#24287 URL's starting with no protocol but with just
//are valid URL's and should be accepted by the validator. This pull request fixes the regex used to validate URL's.