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] Check the BIC country with symfony/intl#28473
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
| // next 2 letters must be alphabetic (country code) | ||
| if (!ctype_alpha(substr($canonicalize,4,2))) { | ||
| $countries = Intl::getRegionBundle()->getCountryNames(); |
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.
symfony/intl is not an hard dependency for the validator, we need to check if it is installed before using it
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.
@chalasr should it be checked forCountryValidator too?
Or should we check againstIbanValidator::$formats making it public?
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.
#28513 does it for other validators, let's do the same here
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.
@chalasr right
nicolas-grekas commentedSep 19, 2018 • 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.
Could you please enhance your commit+PR title? This is a requirement for us to generate proper changelogs. Thanks for your help keeping high quality standards. |
c5b174d to207ace5Compare| // next 2 letters must be alphabetic (country code) | ||
| if (!ctype_alpha(substr($canonicalize,4,2))) { | ||
| if (!class_exists(Intl::class)) { | ||
| thrownewRuntimeException('The "symfony/intl" component is required to use the Bic constraint.'); |
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 be aLogicException as it involves to fix your code by adding the missing dependency (discussion currently happening in#28513)
nicolas-grekas commentedSep 20, 2018
Thank you@sylfabre. |
…ylfabre)This PR was merged into the 4.2-dev branch.Discussion----------[Validator] Check the BIC country with symfony/intl| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#28167| License | MIT| Doc PR | N/ACheck the BIC country code against the list from Intl component instead of a simple check alphabetical test.This PR uses the Intl component which is not part of the required dependencies of the Validator component (https://github.com/symfony/validator/blob/master/composer.json): `symfony/intl` is only required for dev. So I'm making a PR against master because it may break existing code.But `CountryValidator` does the same so it may not be an issue after all.Commits-------27bd3a8 [Validator] Check the BIC country with symfony/intlFix#28167
| // next 2 letters must be alphabetic (country code) | ||
| if (!ctype_alpha(substr($canonicalize,4,2))) { | ||
| if (!class_exists(Intl::class)) { |
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.
IMO this is a BC break. The constraint was working without the Intl component before.
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.
and this exception will only be thrown at runtime if no test covers its usage meaning that it will result in a server error which is not good from my point of view
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.
I agree. Reading the issue, I thought this was accepted as a bugfix.
I'm going to submit a BC layer
…ntl (chalasr)This PR was merged into the 4.2-dev branch.Discussion----------[Validator] Add BC layer covering BicValidator without Intl| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |symfony/symfony#28473 (review)| License | MIT| Doc PR | n/aCommits-------10b8a5f041 [Validator] Add BC layer covering BicValidator without Intl
…ntl (chalasr)This PR was merged into the 4.2-dev branch.Discussion----------[Validator] Add BC layer covering BicValidator without Intl| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#28473 (review)| License | MIT| Doc PR | n/aCommits-------10b8a5f [Validator] Add BC layer covering BicValidator without Intl
Uh oh!
There was an error while loading.Please reload this page.
Check the BIC country code against the list from Intl component instead of a simple check alphabetical test.
This PR uses the Intl component which is not part of the required dependencies of the Validator component (https://github.com/symfony/validator/blob/master/composer.json):
symfony/intlis only required for dev. So I'm making a PR against master because it may break existing code.But
CountryValidatordoes the same so it may not be an issue after all.