Skip to content
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

[Validator] Checking a BIC along with an IBAN #28479

Merged
merged 1 commit into from Dec 1, 2018

Conversation

@sylfabre
Copy link
Contributor

commented Sep 16, 2018

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

A BIC comes usually with an IBAN so it's better to check that they are associated. This PR provides an iban option to Symfony\Component\Validator\Constraints\Bic to check the BIC against an IBAN.

It also provides an ibanPropertyPath to retrieves the IBAN using the property accessor like with comparison constraints.

@sylfabre sylfabre force-pushed the sylfabre:issue_28166 branch 2 times, most recently from 7dc5554 to 67a7ea3 Sep 16, 2018

@xabbuh xabbuh added this to the next milestone Sep 17, 2018

@xabbuh xabbuh added the Validator label Sep 17, 2018

@nicolas-grekas
Copy link
Member

left a comment

Thanks for working on this! Here are some random comments :)

public $ibanMessage = 'This Business Identifier Code (BIC) is not associated with IBAN {{ iban }}.';
public $iban;

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Sep 17, 2018

Member

I'd suggest to remove the blank lines between the added properties

public $ibanPropertyPath;
public function __construct($options = null)

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Sep 17, 2018

Member

missing "array" type hint?

} else {
$iban = $constraint->iban;
}
if ($iban) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Sep 17, 2018

Member

if (!$iban) => return early?

This comment has been minimized.

Copy link
@sylfabre

sylfabre Sep 18, 2018

Author Contributor

@nicolas-grekas no because the logic of validators is to return null on the first violation.

If tomorrow we add some more checks after this new code then we don't want to return early in this check.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Sep 18, 2018

Member

We shouldn't base current code on hypothetical future-based arguments. Here it would make the code clearer now.

if ($iban) {
$ibanCountryCode = substr($iban, 0, 2);
if (ctype_alpha($ibanCountryCode)) {
if (substr($canonicalize, 4, 2) !== $ibanCountryCode) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Sep 17, 2018

Member

the two "if"s should be combined in one using &&

->setCode(Bic::INVALID_IBAN_COUNTRY_CODE_ERROR)
->addViolation();
return;

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Sep 17, 2018

Member

not needed

This comment has been minimized.

Copy link
@sylfabre

sylfabre Sep 18, 2018

Author Contributor

@nicolas-grekas I can remove it but I've done like the other violations raised above

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Sep 18, 2018

Member

Please remove it yes, the code elsewhere is unrelated.

Show resolved Hide resolved src/Symfony/Component/Validator/Tests/Constraints/BicValidatorTest.php
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 17, 2018

Could you improve the PR and commit title please also? (I'd suggest to squash commits while doing so)

@sylfabre sylfabre changed the title Fix #28166 Checking a BIC along with an IBAN Sep 18, 2018

@sylfabre sylfabre force-pushed the sylfabre:issue_28166 branch 4 times, most recently from 2d5d72d to 9fb42ba Sep 18, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 18, 2018

Thanks, LGTM, except the test failure of course.

@sylfabre sylfabre force-pushed the sylfabre:issue_28166 branch from 9fb42ba to 41b422b Sep 18, 2018

@nicolas-grekas nicolas-grekas changed the title Checking a BIC along with an IBAN [Validator] Checking a BIC along with an IBAN Sep 18, 2018

@nicolas-grekas
Copy link
Member

left a comment

thanks

} else {
$iban = $constraint->iban;
}
if (!isset($iban) || !$iban) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Sep 20, 2018

Member

I'd suggest to rewrite the previous this way

        $iban = $constraint->iban;
        $path = $constraint->ibanPropertyPath;
        if ($path && null !== $object = $this->context->getObject()) {
            try {
                $iban = $this->getPropertyAccessor()->getValue($object, $path);
            } catch (NoSuchPropertyException $e) {
                throw new ConstraintDefinitionException(sprintf('Invalid property path "%s" provided to "%s" constraint: %s', $path, \get_class($constraint), $e->getMessage()), 0, $e);
            }
        }
        if (!$iban) { 

@sylfabre sylfabre force-pushed the sylfabre:issue_28166 branch 3 times, most recently from 98bb643 to 645b016 Sep 20, 2018

Show resolved Hide resolved src/Symfony/Component/Validator/Constraints/Bic.php
Show resolved Hide resolved src/Symfony/Component/Validator/Constraints/BicValidator.php
}
}
private function getPropertyAccessor()

This comment has been minimized.

Copy link
@ro0NL

ro0NL Sep 21, 2018

Contributor

getPropertyAccessor(): PropertyAccessor

use Symfony\Component\Validator\Test\ConstraintValidatorTestCase;
class BicComparisonTest_Class

This comment has been minimized.

Copy link
@ro0NL

ro0NL Sep 21, 2018

Contributor

i believe for consistency we should put this at the bottom of the file. Not sure about using _ in the class name, camelcase is preferred.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

Thanks @ro0NL for the comments, good catch!
@sylfabre OK to you with the comments? They all make sense to me (please rebase also.)

Status: needs work

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

we need to decide if we add the $constraint instanceof Bic and throw (BC break-ish) or trigger deprecation

easy to decide: no BC breaks :)

@sylfabre sylfabre force-pushed the sylfabre:issue_28166 branch from dc8a9c1 to 25ce7de Oct 22, 2018

@@ -13,6 +13,7 @@ CHANGELOG
* deprecated using the `Bic`, `Country`, `Currency`, `Language` and `Locale` constraints without `symfony/intl`
* deprecated using the `Email` constraint without `egulias/email-validator`
* deprecated using the `Expression` constraint without `symfony/expression-language`
* added options iban and propertyPath to Bic constraint

This comment has been minimized.

Copy link
@ro0NL

ro0NL Oct 22, 2018

Contributor

ibanPropertyPath

also any symbol is put between backticks (``), here the symbols are: "iban", "ibanPropertyPath" and "Bic"

throw new ConstraintDefinitionException(sprintf('The "iban" and "ibanPropertyPath" of the Iban constraint cannot be used at the same time.', \get_class($this)));
}
if (isset($options['propertyPath']) && !class_exists(PropertyAccess::class)) {

This comment has been minimized.

Copy link
@ro0NL

ro0NL Oct 22, 2018

Contributor

'ibanPropertyPath'

@sylfabre sylfabre force-pushed the sylfabre:issue_28166 branch from 25ce7de to bfd52dd Oct 23, 2018

@OskarStark
Copy link
Contributor

left a comment

Anything todo here?

@ro0NL
Copy link
Contributor

left a comment

tests are red :)

{
if (!class_exists(Intl::class)) {
// throw new LogicException(sprintf('The "symfony/intl" component is required to use the "%s" constraint.', __CLASS__));
@trigger_error(sprintf('Using the "%s" constraint without the "symfony/intl" component installed is deprecated since Symfony 4.2.', __CLASS__), E_USER_DEPRECATED);
}
if (isset($options['iban']) && isset($options['ibanPropertyPath'])) {
throw new ConstraintDefinitionException(sprintf('The "iban" and "ibanPropertyPath" of the Iban constraint cannot be used at the same time.', \get_class($this)));

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 2, 2018

Contributor

... options of the Bic constraint ...

public function __construct($options = null)
public function __construct(array $options = null)

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 2, 2018

Contributor

not sure about this :)

This comment has been minimized.

Copy link
@sylfabre

sylfabre Nov 5, 2018

Author Contributor

looks good for me as we expect an array.

Why aren't you sure about this?

This comment has been minimized.

Copy link
@xabbuh

xabbuh Nov 5, 2018

Member

In this case, the change looks valid to me. Generally you can define a default option by overriding the getDefaultOption() method. But since that is not the case for the Bic constraint, it will already throw an exception when no array is passed.

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 5, 2018

Contributor

exactly, but now we'll never reach the exception at

throw new ConstraintDefinitionException(sprintf('No default option is configured for constraint %s', \get_class($this)));

so basically all constraints will throw this exception, except here it will cause a type error: https://3v4l.org/WjMuY

@sylfabre sylfabre force-pushed the sylfabre:issue_28166 branch from bfd52dd to 435489b Nov 5, 2018

@sylfabre

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

@ro0NL tests are now green :)

@@ -15,6 +15,7 @@ CHANGELOG
* deprecated using the `Bic`, `Country`, `Currency`, `Language` and `Locale` constraints without `symfony/intl`
* deprecated using the `Email` constraint without `egulias/email-validator`
* deprecated using the `Expression` constraint without `symfony/expression-language`
* added options `iban` and `ibanPropertyPath` to Bic constraint

This comment has been minimized.

Copy link
@xabbuh

xabbuh Nov 5, 2018

Member

needs to be added to a new 4.3.0 section now as 4.2 is in feature freeze

{
if (!class_exists(Intl::class)) {
// throw new LogicException(sprintf('The "symfony/intl" component is required to use the "%s" constraint.', __CLASS__));
@trigger_error(sprintf('Using the "%s" constraint without the "symfony/intl" component installed is deprecated since Symfony 4.2.', __CLASS__), E_USER_DEPRECATED);
}
if (isset($options['iban']) && isset($options['ibanPropertyPath'])) {
throw new ConstraintDefinitionException(sprintf('The "iban" and "ibanPropertyPath" options of the Iban constraint cannot be used at the same time.', \get_class($this)));

This comment has been minimized.

Copy link
@xabbuh

xabbuh Nov 5, 2018

Member

self::class instead of get_class($this)(same below)?

private function getPropertyAccessor(): PropertyAccessor
{
if (null === $this->propertyAccessor) {
if (!class_exists('Symfony\Component\PropertyAccess\PropertyAccess')) {

This comment has been minimized.

Copy link
@xabbuh

xabbuh Nov 5, 2018

Member

we should use the class constant here

This comment has been minimized.

Copy link
@sylfabre

sylfabre Nov 5, 2018

Author Contributor

@xabbuh like PropertyAccess::class ?
I haven't tried but I think it will throw an exception if the class does not exist, right?

This comment has been minimized.

Copy link
@xabbuh

xabbuh Nov 5, 2018

Member

yes like that, and using the class constant works even when the class is not present: https://3v4l.org/knSo3

This comment has been minimized.

Copy link
@sylfabre

sylfabre Nov 5, 2018

Author Contributor

@xabbuh thanks!

@sylfabre sylfabre force-pushed the sylfabre:issue_28166 branch from 435489b to 491bbe7 Nov 5, 2018

@sylfabre sylfabre force-pushed the sylfabre:issue_28166 branch from 491bbe7 to bb6be15 Nov 5, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

Thank you @sylfabre.

@nicolas-grekas nicolas-grekas merged commit bb6be15 into symfony:master Dec 1, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Dec 1, 2018

feature #28479 [Validator] Checking a BIC along with an IBAN (sylfabre)
This PR was merged into the 4.3-dev branch.

Discussion
----------

[Validator] Checking a BIC along with an IBAN

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28166
| License       | MIT
| Doc PR        | symfony/symfony-docs#10349

A BIC comes usually with an IBAN so it's better to check that they are associated. This PR provides an `iban` option to `Symfony\Component\Validator\Constraints\Bic` to check the BIC against an IBAN.

It also provides an `ibanPropertyPath` to retrieves the IBAN using the property accessor like with comparison constraints.

Commits
-------

bb6be15 [Validator] Checking a BIC along with an IBAN Fix #28166

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Dec 28, 2018

minor #10349 Validate a BIC along with an IBAN (sylfabre)
This PR was merged into the master branch.

Discussion
----------

Validate a BIC along with an IBAN

Doc for this PR: symfony/symfony#28479

Commits
-------

51f833b Validate a BIC along with an IBAN

issei-m added a commit to issei-m/symfony that referenced this pull request Jan 6, 2019

fabpot added a commit to issei-m/symfony that referenced this pull request Jan 6, 2019

fabpot added a commit that referenced this pull request Jan 6, 2019

minor #29799 [Validator] Add Japanese translation for #28479 (issei-m)
This PR was submitted for the master branch but it was merged into the 3.4 branch instead (closes #29799).

Discussion
----------

[Validator] Add Japanese translation for #28479

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Commits
-------

7a0bdde Add Japanese translation for #28479

fabpot added a commit that referenced this pull request Jan 6, 2019

Merge branch '3.4' into 4.1
* 3.4:
  Add Japanese translation for #28479

fabpot added a commit that referenced this pull request Jan 6, 2019

Merge branch '4.1' into 4.2
* 4.1:
  Add Japanese translation for #28479

fabpot added a commit that referenced this pull request Jan 6, 2019

Merge branch '4.2'
* 4.2:
  Add Japanese translation for #28479

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.