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] added BIC (SWIFT-BIC) validation constraint#15519
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
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.
UUIDs should be used
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.
How can I produce some UUIDs?
stof commentedAug 12, 2015
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) |
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.
Business Identifier Code (non-plural)
mvhirsch commentedAug 12, 2015
Added UUIDs and fixed constraint message (non-plural). |
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.
?
OskarStark commentedAug 12, 2015
i like this feature and think, we have IBAN, so we should provide the BIC Constraint, too. PR looks good to me |
DracoBlue commentedAug 12, 2015
👍 |
1 similar comment
dkeller85 commentedAug 12, 2015
👍 |
OskarStark commentedAug 12, 2015
The EDIT: sorry, didn't see the open todo for the docs.... |
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.
is this condition correct? Excuse me! The code is correct. I didn't understand the above comment correctly.
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 would say yes, if it is not 8 AND not 11 characters long, then buildViolation
javiereguiluz commentedAug 12, 2015
👍 |
mvhirsch commentedAug 12, 2015
@javiereguiluz updated comment for clarity, because it was not English (I'm sorry). |
mvhirsch commentedAug 12, 2015
Oh no! I accidentially rebased on master branch, not 2.8. |
mvhirsch commentedAug 12, 2015
Okay, I fixed the commit :-) |
OskarStark commentedAug 12, 2015
👍 |
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.
How about
if (in_array(strlen($canonicalize), array(8, 11))) {?
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.
@Triiistan I suspect looping over an array is not faster than performing a&&
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.
@stof i think@Triiistan is talking about better readability
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 was trying to save 1 call fromstrlen 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]
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.
However, my concern may not be very pragmatic, so go with the version most readable to your eyes 😃
mvhirsch commentedAug 13, 2015
@Triiistan I've updated the code to use just one strlen call. It seems legit ;-) |
mvhirsch commentedAug 17, 2015
Why does the build fail on HHVM? EDIT: Thanks to whoever ran the Test-Suite again, it now looks stable at all (even HHVM) :-) |
mvhirsch commentedSep 8, 2015
Status: Reviewed |
jakzal commentedSep 8, 2015
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 commentedSep 8, 2015
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 commentedSep 8, 2015
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 commentedSep 8, 2015
@jakzal@xabbuh@OskarStark we could provide a ValidatorExtraBundle or ValidatorFinanceExtraBundle (name proposal) for symfony 3.0. What do you think? |
OskarStark commentedSep 8, 2015
👍@mvhirsch sounds good to me |
mvhirsch commentedSep 11, 2015
jakzal commentedSep 14, 2015
@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 commentedSep 14, 2015
anyway, i thin we should merge this as it is. the Iban is already in so.... |
mvhirsch commentedSep 21, 2015
@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 commentedSep 24, 2015
Can you please add translations for the message (at least add it to the |
mvhirsch commentedSep 24, 2015
@xabbuh I added translations for English and German language. |
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 would change this to "[...] ist kein gültiger BIC."
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.
👍
fabpot commentedSep 24, 2015
ping @symfony/deciders |
xabbuh commentedSep 24, 2015
👍 (besides my comment on the translation, but this shouldn't hold off merging imo) |
dunglas commentedSep 24, 2015
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 commentedSep 25, 2015
@xabbuh I fixed the German translation. Thank you! |
fabpot commentedSep 25, 2015
As we don't this "community-driven" repository/bundle with such validators, I propose to merge this one for consistency. |
dunglas commentedSep 25, 2015
agree |
fabpot commentedSep 25, 2015
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#5623I'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 documentationCommits-------d6471b3 [Validator] added BIC (SWIFT-BIC) validation constraint
OskarStark commentedSep 25, 2015
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 | noneCommits-------7911fe1 [Validator] added BIC validator
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 onWikipedia. It validates the structure based on alphabetic/alphanumeric values and the value's length.
Todo-list: