[Validator] added BIC (SWIFT-BIC) validation constraint #15519

mvhirsch · 2015-08-12T13:34:23Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	none
License	MIT
Doc PR	symfony/symfony-docs#5623

I've added the BIC validator, because we do often need validation for IBAN and BIC values. Since the IBAN validation was already included into Symfony, I was asking myself: why not contribute my BIC validator to the community? So here we go ...

It depends on ISO 9362 as described on Wikipedia. It validates the structure based on alphabetic/alphanumeric values and the value's length.

Todo-list:

submit changes to the documentation

stof · 2015-08-12T13:37:26Z

src/Symfony/Component/Validator/Constraints/Bic.php

+    const INVALID_CHARACTERS_ERROR = 2;
+    const INVALID_BANK_CODE_ERROR = 3;
+    const INVALID_COUNTRY_CODE_ERROR = 4;
+    const INVALID_CASE_ERROR = 5;


UUIDs should be used

How can I produce some UUIDs?

stof · 2015-08-12T13:38:45Z

I'm not sure this should be in the core (I'm also not sur ethe IBAN deserves being in core, but it is there already and so needs to stay for BC reasons)

sstok · 2015-08-12T13:46:50Z

src/Symfony/Component/Validator/Constraints/Bic.php

+        self::INVALID_CASE_ERROR => 'INVALID_CASE_ERROR',
+    );
+
+    public $message = 'This is not a valid Business Identifier Codes (BIC).';


Business Identifier Code (non-plural)

mvhirsch · 2015-08-12T13:57:39Z

Added UUIDs and fixed constraint message (non-plural).

OskarStark · 2015-08-12T14:05:09Z

src/Symfony/Component/Validator/Tests/Constraints/BicValidatorTest.php

+    {
+        // http://formvalidation.io/validators/bic/
+        return array(
+          //array('12345678901')


OskarStark · 2015-08-12T14:06:45Z

i like this feature and think, we have IBAN, so we should provide the BIC Constraint, too.

PR looks good to me

DracoBlue · 2015-08-12T14:11:29Z

👍

dkeller85 · 2015-08-12T14:11:49Z

👍

OskarStark · 2015-08-12T14:13:39Z

The [WIP] should be removed from the PR subject @mvhirsch

EDIT: sorry, didn't see the open todo for the docs....

javiereguiluz · 2015-08-12T14:59:22Z

src/Symfony/Component/Validator/Constraints/BicValidator.php

+        $canonicalize = str_replace(' ', '', $value);
+
+        // the bic must have either 8 or 11 characters
+        if (8 !== strlen($canonicalize) && 11 !== strlen($canonicalize)) {


~~is this condition correct?~~ Excuse me! The code is correct. I didn't understand the above comment correctly.

i would say yes, if it is not 8 AND not 11 characters long, then buildViolation

javiereguiluz · 2015-08-12T15:04:02Z

👍

mvhirsch · 2015-08-12T15:10:22Z

@javiereguiluz updated comment for clarity, because it was not English (I'm sorry).

mvhirsch · 2015-08-12T15:13:00Z

Oh no! I accidentially rebased on master branch, not 2.8.
How can I undo this mistake? Please help me!

mvhirsch · 2015-08-12T15:36:48Z

Okay, I fixed the commit :-)

OskarStark · 2015-08-12T15:40:22Z

👍

Mx-Glitter · 2015-08-12T17:27:42Z

src/Symfony/Component/Validator/Constraints/BicValidator.php

+        $canonicalize = str_replace(' ', '', $value);
+
+        // the bic must be either 8 or 11 characters long
+        if (8 !== strlen($canonicalize) && 11 !== strlen($canonicalize)) {


How about

if (in_array(strlen($canonicalize), array(8, 11))) {

?

@Triiistan I suspect looping over an array is not faster than performing a &&

@stof i think @Triiistan is talking about better readability

I was trying to save 1 call from strlen and I tried to find which style Symfony used most between

if (yyy !== xxxx && zzz !== xxxx) {

and

if (!in_array(xxxx, array(yyy, zzz)) {

and the latter was used a bit more.

With the first style, we can write

$canonicalizeLength = strlen($canonicalize); if (8 !== $canonicalizeLength && 11 !== $canonicalizeLength) {

to avoid the duplication.

[Updated: I inverted both styles]

However, my concern may not be very pragmatic, so go with the version most readable to your eyes 😃

mvhirsch · 2015-08-13T06:25:40Z

@Triiistan I've updated the code to use just one strlen call. It seems legit ;-)

mvhirsch · 2015-08-17T11:43:34Z

Why does the build fail on HHVM?
Can we please re-run the Test-Suite?

EDIT: Thanks to whoever ran the Test-Suite again, it now looks stable at all (even HHVM) :-)

mvhirsch · 2015-09-08T07:01:13Z

Status: Reviewed

jakzal · 2015-09-08T07:17:55Z

I'm also not convinced we should have this kind of validators in core, as this is smething that can be easily provided by a 3rd party extension. The IBAN validator gave us a few headaches.

xabbuh · 2015-09-08T07:18:59Z

I agree that a BIC validator is does not fit best in the core. However, we already have the IBAN validator and for consistency we imho should include this one too.

OskarStark · 2015-09-08T08:17:30Z

👍 @xabbuh

otherwise 3.0 could be the right moment to remove not wanted stuff from core and move this to a separate bundle, isn't it?

mvhirsch · 2015-09-08T09:24:43Z

@jakzal @xabbuh @OskarStark we could provide a ValidatorExtraBundle or ValidatorFinanceExtraBundle (name proposal) for symfony 3.0. What do you think?
For now we could add this in 2.8.

OskarStark · 2015-09-08T09:39:29Z

👍 @mvhirsch sounds good to me

mvhirsch · 2015-09-11T18:15:05Z

ping @jakzal @xabbuh

jakzal · 2015-09-14T13:50:46Z

@mvhirsch well, yes, but I don't think such a bundle (or component) needs to be provided in the Symfony organisation. It's a perfect candidate for a community extension.

OskarStark · 2015-09-14T18:26:24Z

anyway, i thin we should merge this as it is.

the Iban is already in so....

mvhirsch · 2015-09-21T08:59:43Z

@jakzal we could add it for 2.8 anyway and extract/remove the parts for 3.0 and provide a community extension for those validators. What do you think?

xabbuh · 2015-09-24T06:24:42Z

Can you please add translations for the message (at least add it to the validators.en.xlf file)?

mvhirsch · 2015-09-24T09:17:47Z

@xabbuh I added translations for English and German language.

xabbuh · 2015-09-24T11:02:17Z

src/Symfony/Component/Validator/Resources/translations/validators.de.xlf

@@ -310,6 +310,10 @@
                <source>This value does not match the expected {{ charset }} charset.</source>
                <target>Dieser Wert entspricht nicht dem erwarteten Zeichensatz {{ charset }}.</target>
            </trans-unit>
+            <trans-unit id="81">
+                <source>This is not a valid Business Identifier Code (BIC).</source>
+                <target>Dieser Wert ist keine gültige BIC-Nummer.</target>


I would change this to "[...] ist kein gültiger BIC."

fabpot · 2015-09-24T11:39:04Z

ping @symfony/deciders

xabbuh · 2015-09-24T12:15:34Z

👍 (besides my comment on the translation, but this shouldn't hold off merging imo)

dunglas · 2015-09-24T19:47:34Z

Adding a new feature to deprecate it immediately doesn't sound to me like a good idea. IBAN and BIC validation is not so easy and is a very specific use case. I'm in favor of deprecating the IBAN validator in 2.8, remove it from 3.0 and provide a new community library with the IBAN and BIC validators. It would be cool to have a library usable without the DIC component (so no bundle but it can still include an optional extension).

mvhirsch · 2015-09-25T07:17:43Z

@xabbuh I fixed the German translation. Thank you!

fabpot · 2015-09-25T12:07:21Z

As we don't this "community-driven" repository/bundle with such validators, I propose to merge this one for consistency.

dunglas · 2015-09-25T12:09:33Z

agree

fabpot · 2015-09-25T12:16:10Z

Thank you @mvhirsch.

…t (mvhirsch) This PR was squashed before being merged into the 2.8 branch (closes #15519). Discussion ---------- [Validator] added BIC (SWIFT-BIC) validation constraint | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | symfony/symfony-docs#5623 I've added the BIC validator, because we do often need validation for IBAN and BIC values. Since the IBAN validation was already included into Symfony, I was asking myself: why not contribute my BIC validator to the community? So here we go ... It depends on ISO 9362 as described on [Wikipedia](https://en.wikipedia.org/wiki/ISO_9362#Structure). It validates the structure based on alphabetic/alphanumeric values and the value's length. Todo-list: - [x] submit changes to the documentation Commits ------- d6471b3 [Validator] added BIC (SWIFT-BIC) validation constraint

OskarStark · 2015-09-25T12:27:23Z

yay, congrats @mvhirsch and thank you!

This PR was squashed before being merged into the 2.8 branch (closes #5623). Discussion ---------- [Validator] added BIC validator | Q | A | ------------- | --- | Doc fix? | no | New docs? | yes, PR: symfony/symfony#15519 | Applies to | 2.8 | Fixed tickets | none Commits ------- 7911fe1 [Validator] added BIC validator

carsonbot added the Status: Needs Review label Aug 12, 2015

stof reviewed Aug 12, 2015
View reviewed changes

sstok reviewed Aug 12, 2015
View reviewed changes

OskarStark reviewed Aug 12, 2015
View reviewed changes

mvhirsch mentioned this pull request Aug 12, 2015

[Validator] added BIC validator symfony/symfony-docs#5623

Closed

mvhirsch changed the title ~~[WIP] [Validator] added BIC (SWIFT-BIC) validation constraint~~ [Validator] added BIC (SWIFT-BIC) validation constraint Aug 12, 2015

javiereguiluz reviewed Aug 12, 2015
View reviewed changes

Mx-Glitter reviewed Aug 12, 2015
View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 8, 2015

xabbuh reviewed Sep 24, 2015
View reviewed changes

mvhirsch added 9 commits September 25, 2015 09:16

[Validator] added BIC (SWIFT-BIC) validation constraint

2fd8573

[Validator] fixed coding standards. refs #15519

99bc9f1

[Validator] using non plural constraint message. refs #15519

a2b612e

[Validator] Using UUIDs instead of scalar values. refs #15519

ec839e7

[Validator] removed unnecessary comment. refs #15519

398988d

[Validator] fixed comment for clarity. refs #15519

be41875

[Validator] use just one strlen call. refs #15519

f278cbb

[Validator] added english and german translation messages. refs #15519

71e11d7

[Validator] fixed german translation. refs #15519

fc432e4

fabpot closed this Sep 25, 2015

fabpot mentioned this pull request Nov 16, 2015

Release v2.8.0-BETA1 #16550

Merged

mvhirsch deleted the bic_validation_constraint branch July 4, 2017 12:52

[Validator] added BIC (SWIFT-BIC) validation constraint #15519

[Validator] added BIC (SWIFT-BIC) validation constraint #15519

Conversation

mvhirsch commented Aug 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Aug 12, 2015

Choose a reason for hiding this comment

mvhirsch commented Aug 12, 2015

Choose a reason for hiding this comment

OskarStark commented Aug 12, 2015

DracoBlue commented Aug 12, 2015

dkeller85 commented Aug 12, 2015

OskarStark commented Aug 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Aug 12, 2015

mvhirsch commented Aug 12, 2015

mvhirsch commented Aug 12, 2015

mvhirsch commented Aug 12, 2015

OskarStark commented Aug 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvhirsch commented Aug 13, 2015

mvhirsch commented Aug 17, 2015

mvhirsch commented Sep 8, 2015

jakzal commented Sep 8, 2015

xabbuh commented Sep 8, 2015

OskarStark commented Sep 8, 2015

mvhirsch commented Sep 8, 2015

OskarStark commented Sep 8, 2015

mvhirsch commented Sep 11, 2015

jakzal commented Sep 14, 2015

OskarStark commented Sep 14, 2015

mvhirsch commented Sep 21, 2015

xabbuh commented Sep 24, 2015

mvhirsch commented Sep 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Sep 24, 2015

xabbuh commented Sep 24, 2015

dunglas commented Sep 24, 2015

mvhirsch commented Sep 25, 2015

fabpot commented Sep 25, 2015

dunglas commented Sep 25, 2015

fabpot commented Sep 25, 2015

OskarStark commented Sep 25, 2015