[Validator] Check the BIC country with symfony/intl #28473
Conversation
| @@ -63,7 +64,8 @@ public function validate($value, Constraint $constraint) | |||
| } | |||
|
|
|||
| // 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.
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.
@chalasr should it be checked for CountryValidator too?
Or should we check against IbanValidator::$formats making it public?
There was a problem hiding this comment.
#28513 does it for other validators, let's do the same here
|
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. |
sylfabre
force-pushed
the
issue_28167
branch
2 times, most recently
from
c5b174d
to
207ace5
Compare
sylfabre
changed the title
| @@ -63,7 +70,11 @@ public function validate($value, Constraint $constraint) | |||
| } | |||
|
|
|||
| // next 2 letters must be alphabetic (country code) | |||
| if (!ctype_alpha(substr($canonicalize, 4, 2))) { | |||
| if (!class_exists(Intl::class)) { | |||
| throw new RuntimeException('The "symfony/intl" component is required to use the Bic constraint.'); | |||
There was a problem hiding this comment.
should be a LogicException as it involves to fix your code by adding the missing dependency (discussion currently happening in #28513)
sylfabre
force-pushed
the
issue_28167
branch
from
207ace5
to
27bd3a8
Compare
nicolas-grekas
changed the title
|
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/A 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/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/intl Fix #28167
| @@ -63,7 +69,11 @@ public function validate($value, Constraint $constraint) | |||
| } | |||
|
|
|||
| // 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.
IMO this is a BC break. The constraint was working without the Intl component before.
There was a problem hiding this comment.
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.
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/a Commits ------- 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/a Commits ------- 10b8a5f [Validator] Add BC layer covering BicValidator without Intl
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.