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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Surrounded requirements regex by parenthesis #1110

Merged
merged 1 commit into from Sep 13, 2015

Conversation

Projects
None yet
3 participants
@b-viguier
Contributor

b-viguier commented Sep 8, 2015

Currently, the regex used for requirements is automatically surrounded by characters ^ and $ (see ParamFetcher.php code). It seems legit and very helpful, since wen can easily write A+ to match at least one A, or even A if we expect only one occurence.

Unfortunately, this helper may introduce a misunderstanding of what really happens in the case where we want to match an A or a B character. Our first guess will be A|B, but is is wrong. This requirement will produce the regex ^A|B$, matching all strings starting by A or finishing by B.

This behavior is absolutely normal, this is not a bug, but IMHO it is counter-intuitive (then, dangerous). In the previous example, I should use parenthesis to reach my goal: ^(A|B)$. So here my proposal, to add by default these surrounding parenthesis, in order to make requirement string A|B to match exactly one A, or exactly one B.

According to my tests, even if you already used some ^ or $ in your regex, this should be retro-compatible. Nevertheless, this is a breaking change as soon as you were really expecting the string A|B to match Abc (or );TRUNCATE Users;#B 馃槈 )

I updated corresponding tests in this PR, I am looking forward for your feedback, and thanks for all the work done for this library 馃憤

Show outdated Hide outdated Request/ParamFetcher.php
@@ -213,7 +213,7 @@ public function cleanParamWithRequirements(Param $config, $param, $strict)
return $default;
}
$constraint = new Regex(array(
'pattern' => '#^'.$config->requirements.'$#xsu',
'pattern' => '#^('.$config->requirements.')$#xsu',

This comment has been minimized.

@stof

stof Sep 8, 2015

Member

please use a non-capturing group (adding a new capturing group makes it slower for all cases, and can break cases using advanced regex features)

@stof

stof Sep 8, 2015

Member

please use a non-capturing group (adding a new capturing group makes it slower for all cases, and can break cases using advanced regex features)

This comment has been minimized.

@b-viguier

b-viguier Sep 8, 2015

Contributor

馃憤

@b-viguier

b-viguier Sep 8, 2015

Contributor

馃憤

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 8, 2015

Member

please add a test using a regex A|B to ensure it indeed rejects Ac

Member

stof commented Sep 8, 2015

please add a test using a regex A|B to ensure it indeed rejects Ac

@b-viguier

This comment has been minimized.

Show comment
Hide comment
@b-viguier

b-viguier Sep 8, 2015

Contributor

@stof Done
I tried to mock the validator in order to cover 99% of the use cases, but since this service is injected we cannot guarantee that a standard Symfony RegexValidatorwill always be used.
I am still open to your remarks if you are not totally satisfied, thanks 馃槃

Contributor

b-viguier commented Sep 8, 2015

@stof Done
I tried to mock the validator in order to cover 99% of the use cases, but since this service is injected we cannot guarantee that a standard Symfony RegexValidatorwill always be used.
I am still open to your remarks if you are not totally satisfied, thanks 馃槃

lsmith77 added a commit that referenced this pull request Sep 13, 2015

Merge pull request #1110 from b-viguier/feature/regex-parenthesis
Surrounded requirements regex by parenthesis

@lsmith77 lsmith77 merged commit 711fe5d into FriendsOfSymfony:master Sep 13, 2015

2 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@b-viguier

This comment has been minimized.

Show comment
Hide comment
@b-viguier

b-viguier Sep 14, 2015

Contributor

Nice!
馃槃

Contributor

b-viguier commented Sep 14, 2015

Nice!
馃槃

@omansour omansour referenced this pull request Jan 4, 2016

Merged

Update oss.md #115

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