[Validator] support protocolless urls validation #24308
Conversation
|
I'm not sure we can do this change as is: protocol-relative URLs have only a relative meaning. |
|
@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. |
|
Let's go for option 2, with a default that doesn't change the behavior for sure. |
|
Can you rebase this PR to get rid of the merge commit? Thanks. |
d1707ac to
9085c40
Compare
| public function testValidRelativeUrl($url) | ||
| { | ||
| $constraint = new Url(array( | ||
| 'relativeProtocol' => true, |
There was a problem hiding this comment.
should also test absolute urls are still valid
There was a problem hiding this comment.
The test has both getValidRelativeUrls and getValidUrls as a data provider. That covers both absolute and relative URL's. Or am I missing something?
There was a problem hiding this comment.
@MyDigitalLife nice :) didnt noticed that.
|
|
||
| public function getInvalidRelativeUrls() | ||
| { | ||
| return array( |
There was a problem hiding this comment.
Any reason why we are not using [] instead of array
There was a problem hiding this comment.
It's because Symfony coding standards. We can't easily change the existing code from array() to [] ... so using [] only for the new code would be inconsistent and that's why we use array() everywhere.
There was a problem hiding this comment.
This started as a patch for 2.8 so I needed to use the array 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.
|
@fabpot The PR has been rebased. |
| @@ -61,7 +61,13 @@ public function validate($value, Constraint $constraint) | |||
| return; | |||
| } | |||
|
|
|||
| $pattern = sprintf(static::PATTERN, implode('|', $constraint->protocols)); | |||
| $protocolPattern = '(%s):'; | |||
There was a problem hiding this comment.
non-capturing, i.e. (?:%s):? Might be patched in 2.7 for groups currently in PATTERN. 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.
What about (?:'.$protocolPattern.')? to reuse previous pattern. Also no real reason for capturing here.
|
Moving to 4.1 as 3.4 is in beta, thus closed for new feats. |
|
Could you please rebase against master ? |
be0b1b4 to
d845406
Compare
|
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 | MIT 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. Commits ------- d845406 [Validator] support protocolless urls validation
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.