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] support protocolless urls validation #24308

Merged
merged 1 commit into from Feb 14, 2018

Conversation

Projects
None yet
9 participants
@MyDigitalLife
Contributor

MyDigitalLife commented Sep 24, 2017

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.

@xabbuh

xabbuh approved these changes Sep 24, 2017

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Sep 24, 2017

I'm not sure we can do this change as is: protocol-relative URLs have only a relative meaning.
What if I want to validate that an URL is not-relative, as this validation rule used to provide?
In a Web form, many won't want to accept protocol-relative URLs, isn't it?

@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Sep 24, 2017

@MyDigitalLife

This comment has been minimized.

Contributor

MyDigitalLife commented Sep 24, 2017

@nicolas-grekas Good point. I see 2 possible ways to fix this:

  1. Create a RelativeUrlValidator class
  2. Add a option to the current validator that can enable or disable the check for relative URL's

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

This comment has been minimized.

Member

nicolas-grekas commented Sep 24, 2017

Let's go for option 2, with a default that doesn't change the behavior for sure.
Should be submitted as a new feature, so against 3.4.

@MyDigitalLife MyDigitalLife changed the base branch from 2.8 to 3.4 Sep 24, 2017

@fabpot

This comment has been minimized.

Member

fabpot commented Sep 24, 2017

Can you rebase this PR to get rid of the merge commit? Thanks.

public function testValidRelativeUrl($url)
{
$constraint = new Url(array(
'relativeProtocol' => true,

This comment has been minimized.

@ro0NL

ro0NL Sep 25, 2017

Contributor

should also test absolute urls are still valid

This comment has been minimized.

@MyDigitalLife

MyDigitalLife Sep 25, 2017

Contributor

The test has both getValidRelativeUrls and getValidUrls as a data provider. That covers both absolute and relative URL's. Or am I missing something?

This comment has been minimized.

@ro0NL

ro0NL Sep 25, 2017

Contributor

@MyDigitalLife nice :) didnt noticed that.

public function getInvalidRelativeUrls()
{
return array(

This comment has been minimized.

@asmshaon

asmshaon Sep 25, 2017

Any reason why we are not using [] instead of array

This comment has been minimized.

@javiereguiluz

javiereguiluz Sep 25, 2017

Member

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.

This comment has been minimized.

@MyDigitalLife

MyDigitalLife Sep 25, 2017

Contributor

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.

@MyDigitalLife

This comment has been minimized.

Contributor

MyDigitalLife commented Sep 26, 2017

@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):';

This comment has been minimized.

@ro0NL

ro0NL Oct 11, 2017

Contributor

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):)?';

This comment has been minimized.

@ro0NL

ro0NL Oct 11, 2017

Contributor

What about (?:'.$protocolPattern.')? to reuse previous pattern. Also no real reason for capturing here.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 24, 2017

Moving to 4.1 as 3.4 is in beta, thus closed for new feats.

@nicolas-grekas nicolas-grekas modified the milestones: 2.8, 4.1 Oct 24, 2017

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Nov 1, 2017

Could you please rebase against master ?

@nicolas-grekas nicolas-grekas added Feature and removed Bug labels Nov 26, 2017

@nicolas-grekas nicolas-grekas changed the title from [Validator] Updated Validator to support protocolless urls. to [Validator] support protocolless urls validation Feb 14, 2018

@nicolas-grekas nicolas-grekas changed the base branch from 3.4 to master Feb 14, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Feb 14, 2018

Thank you @MyDigitalLife.

@nicolas-grekas nicolas-grekas merged commit d845406 into symfony:master Feb 14, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Feb 14, 2018

feature #24308 [Validator] support protocolless urls validation (MyDi…
…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

@xabbuh xabbuh referenced this pull request Feb 14, 2018

Closed

Protocol-less URL support #9266

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment