Skip to content
This repository has been archived by the owner on Dec 11, 2020. It is now read-only.

Fix IBAN generator #590

Merged
merged 5 commits into from
Jun 15, 2015
Merged

Fix IBAN generator #590

merged 5 commits into from
Jun 15, 2015

Conversation

okj579
Copy link
Contributor

@okj579 okj579 commented May 24, 2015

This fixes:

@@ -225,22 +225,20 @@ protected static function iban($countryCode, $prefix = '', $length = null)
}
}
}
if ($format === null) {
return false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove

@fzaninotto
Copy link
Owner

Please add unit tests to prove the bugs, and to show that your PR fixes them.

@okj579
Copy link
Contributor Author

okj579 commented Jun 6, 2015

Ok, I'm working on this. The format is not to hard to test, but to test the checksum, the generator code would essentially have to be copied into the unit test, which sort of seems to defeat the purpose. An IBAN Checker (e.g. okj579/swiftery or globalcitizen/php-iban) could be added to the project to do the testing. This seems like a lot for a unit test, but I think probably the best option. Should I do that?

@fzaninotto
Copy link
Owner

You should move the checksum calculation to a standalone Calculator class (see the Luhn class), and reuse that class in your test.

@okj579
Copy link
Contributor Author

okj579 commented Jun 13, 2015

That's a much better idea. I've refactored it, so the checksum logic is done in Faker\Calculator\Iban (it is its own algorithm), which can also be tested with unit tests.

fzaninotto added a commit that referenced this pull request Jun 15, 2015
@fzaninotto fzaninotto merged commit 9eed6c2 into fzaninotto:master Jun 15, 2015
@fzaninotto
Copy link
Owner

Awesome, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants