jschaedl / iban-validation

:bank: A small library for validating International Bank Account Numbers (IBANs)
MIT License
88 stars 19 forks source link

Iban\Validation\Validator::validate without violations #81

Closed tugrul closed 1 year ago

tugrul commented 1 year ago

I was able to use like below but Iban\Validation\Validator class has the final keyword is blocking now and I can't extend anymore.

I don't need any violation list because it does not matter when one of these validations is failed.

namespace App\Service;

use Iban\Validation\Validator;

class IbanValidator extends Validator
{

    public function validate($iban)
    {
        $this->validateCountryCode($iban);
        $this->validateLength($iban);
        $this->validateFormat($iban);
        $this->validateChecksum($iban);
    }

}

Then I can handle validation issues in try catch like below.

class SomeController extends AbstractController {

    #[Route('/user/iban-review/{iban}', name: 'user-iban-preview')]
    public function previewAction(string $iban): JsonResponse
    {
        try {

            return $this->json([
                'success' => true,
                'result' => $this->ibanParser->parse(new Iban($iban))
            ], 200, [], ['groups' => ['public']]);

        } catch (Throwable $exception) {

            return $this->json([
                'success' => false,
                'message' => $exception->getMessage()
            ]);

        }

    }
}
jschaedl commented 1 year ago

Hi @tugrul

I made the Validator final on purpose. If I open it for extension, it would be much harder to change any internals in future releases.

I can imagine to add another option to specify, if the Validator should use the violations or if it should throw an exception.

$validator = new Validator(['throw_exceptions' => true]);
try {
    $validator->validate('some iban');
} catch (ValidationException $exception) {
    // ...
}

To your use case:

I don't need any violation list because it does not matter when one of these validations is failed.

Couldn't you use the provided violations instead of the exception messages:

class SomeController extends AbstractController {

    #[Route('/user/iban-review/{iban}', name: 'user-iban-preview')]
    public function previewAction(string $iban): JsonResponse
    {
            $validator = new Validator();

            return $this->json([
                'success' => $success = $validator->validate(new Iban($iban)),
                'result' => $success ? '' : implode(', ', $validator->getViolations()) // you could also just use the first array item
            ], 200, [], ['groups' => ['public']]);
    }
}
tugrul commented 1 year ago

Hi @jschaedl,

Your suggestion to add an option to the existing validation class was my other idea. I opted for the other one as it seems cleaner to make the base class but I can change it based on that idea.

What do you think about converting checksum staff to a trait class?

I can't prefer your suggestion for my use case because there are some other validations for region/bank specific validations (bank code, bank's internal account number structure, etc.).

Handling exceptions is an easier and cleaner way in this scenario.

tugrul commented 1 year ago

@jschaedl can you take an action? I also implemented your suggestion and created another pull request.

tugrul commented 1 year ago

I also created a pull request for the trait implementation.

You may choose one of the 3 pull requests you like and reject the other two.

jschaedl commented 1 year ago

Hi @tugrul thank you very much for taking action here and opening pull requests with different implementations. This is very much appreciated.

Nevertheless, I gave it another thought and I don't want to change so much regarding the current Validator implementation. That is why I implemented the following solution which is simpler imho and should solve your problem. Please checkout: https://github.com/jschaedl/iban-validation/pull/95

You can now pass true as a second argument to the validate() method of the Validator class if you want to handle the exceptions yourself:

try {
    $validator->validate(new Iban('DE89 3704 0044 0532 0130 00'), throw: true);
} catch (Exception $exception) {
    // ...
}

This also won't break userland code and upstream projects can smoothly migrate to version 2.0.0 (which I plan to release in the next couple of days).