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

@sylfabre sylfabre 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.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

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;

Copy link
Member

Choose a reason for hiding this comment

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

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


public $ibanPropertyPath;

public function __construct($options = null)
Copy link
Member

Choose a reason for hiding this comment

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

missing "array" type hint?

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

Choose a reason for hiding this comment

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

if (!$iban) => return early?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

return;
Copy link
Member

Choose a reason for hiding this comment

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

not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Please remove it yes, the code elsewhere is unrelated.

@nicolas-grekas
Copy link
Member

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 issue_28166 branch 4 times, most recently from 2d5d72d to 9fb42ba Compare September 18, 2018 08:02
@nicolas-grekas
Copy link
Member

Thanks, LGTM, except the test failure of course.

@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
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

thanks

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

Choose a reason for hiding this comment

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

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 issue_28166 branch 3 times, most recently from 98bb643 to 645b016 Compare September 20, 2018 12:31
}
}

private function getPropertyAccessor()
Copy link
Contributor

Choose a reason for hiding this comment

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

getPropertyAccessor(): PropertyAccessor

use Symfony\Component\Validator\Test\ConstraintValidatorTestCase;

class BicComparisonTest_Class
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Member

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

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

'ibanPropertyPath'

Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Anything todo here?

Copy link
Contributor

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

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

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)));
Copy link
Contributor

Choose a reason for hiding this comment

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

... options of the Bic constraint ...


public function __construct($options = null)
public function __construct(array $options = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure about this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks good for me as we expect an array.

Why aren't you sure about this?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

sylfabre 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
Copy link
Member

Choose a reason for hiding this comment

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

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)));
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

we should use the class constant here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xabbuh thanks!

@nicolas-grekas
Copy link
Member

Thank you @sylfabre.

@nicolas-grekas nicolas-grekas merged commit bb6be15 into symfony:master Dec 1, 2018
nicolas-grekas added a commit that referenced this pull request Dec 1, 2018
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
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 pushed 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
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
* 3.4:
  Add Japanese translation for #28479
fabpot added a commit that referenced this pull request Jan 6, 2019
* 4.1:
  Add Japanese translation for #28479
fabpot added a commit that referenced this pull request Jan 6, 2019
* 4.2:
  Add Japanese translation for #28479
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@fabpot fabpot mentioned this pull request May 9, 2019
@sylfabre sylfabre deleted the issue_28166 branch September 25, 2020 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants