-
-
Notifications
You must be signed in to change notification settings - Fork 59
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add the support for SWIFT Business Identifier Code (BIC) format #58
feat: Add the support for SWIFT Business Identifier Code (BIC) format #58
Conversation
dc11913
to
f1c422b
Compare
f1c422b
to
56c6fc6
Compare
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.
@codisart Thanks a lot for your contribution. I've added some comments.
We need also rebase and target develop as it is new feature and can be released in next minor. Thanks 👍
56c6fc6
to
69f9ae6
Compare
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.
Good extension for this component! 👍
I added some small comments and suggestions.
src/BusinessIdentifierCode.php
Outdated
self::NOT_VALID_COUNTRY => 'Invalid country code', | ||
]; | ||
|
||
private const REGEX_BIC = '/^[a-z]{4}(?<country>[a-z]{2})[0-9a-z]{2}([0-9a-z]{3})?$/i'; |
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.
The same here, if there are an official pattern then a link should be added.
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.
The pattern is defined in the wikipedia page. Would it be sufficient ?
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.
If the pattern is adopted, we should add the reference, and if the official specification is behind a paywall, Wikimedia is good.
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've found a regex provided by the "Bundesbank" in this PDF:
Page 39. Maybe we might use that regex instead or we have a better "official" resource.
69f9ae6
to
18838b0
Compare
eb5d64a
to
4a0d359
Compare
Hello @froschdesign and @michalbundyra, are my last changes ok with your recommendations ? |
Hello @froschdesign and @michalbundyra, are there changes needed to this pull request ? |
Hello @boesing, I see that you removed the PR from the 2.14 milestone. |
@codisart I did not dive very deep into this but there are some change requests in here which will probably block the release of 2.14.0. After the release, we can re-target this branch to 2.15. |
Signed-off-by: codisart <[email protected]>
- 1 line per sentence - Use `-` for bullets (not `*`) - Add whitespace after headings Signed-off-by: Matthew Weier O'Phinney <[email protected]>
This patch updates the REGEX_BIC constant to follow the documentation from the bundesbank. The main changes are that the two characters following the country code have more restrictions (the first cannot contain the digit 1, the second cannot contain the letter "o"). A comment with a link to the documentation (and reference to the page) is provided. CS was also indicating we had two unused import statements, so I removed those. Signed-off-by: Matthew Weier O'Phinney <[email protected]>
4a0d359
to
5ff51fb
Compare
I updated the REGEX to follow the BundesBank documentation (it was just some minor changes). I also did some cleanup on the documentation. |
Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Description
Implementation proposal for #20