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

Allow to chose behavior if country is not supported #65

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

VincentLanglet
Copy link
Contributor

Closes #40

cc @gemal @dannyvankooten

The option doesn't make sens for validateVatNumber, since the validateVatNumberExistence only work for EU country since the check is done on https://ec.europa.eu/taxation_customs/vies/#/vat-validation

@dannyvankooten
Copy link
Member

dannyvankooten commented Nov 23, 2023

@VincentLanglet Personally I am fine throwing a something like an InvalidArgumentException or LogicException (after a major version bump because of the BC break) in case the method is called with a country not supported by this library. That would help keep the API a bit cleaner. What are your thoughts on that?

@VincentLanglet
Copy link
Contributor Author

@VincentLanglet Personally I am fine throwing a something like an InvalidArgumentException or LogicException (after a major version bump because of the BC break) in case the method is called with a country not supported by this library. That would help keep the API a bit cleaner. What are your thoughts on that?

Currently the VAT validator check mainly EU countries VAT but

IMHO, in the futur this lib should support all possible VAT prefix and have the related regex.
One issue we currently have with the Validator is the fact that we don't know if the error is coming from an invalid pattern FR12 (for instance), an invalid country XX or a not yet supported country RU (for instance).

Instead it is better for the API to have

if (! isset($this->patterns[$country])) {
     throw new \InvalidArgumentException('The country prefix is invalid or not implemented yet');
}

but this still require to expose the hasSupportedCountryPrefix method.

We can also throw a custom VatException (?)

@VincentLanglet
Copy link
Contributor Author

Currently the VAT validator check mainly EU countries VAT but

IMHO, in the futur this lib should support all possible VAT prefix and have the related regex. One issue we currently have with the Validator is the fact that we don't know if the error is coming from an invalid pattern FR12 (for instance), an invalid country XX or a not yet supported country RU (for instance).

Instead it is better for the API to have

if (! isset($this->patterns[$country])) {
     throw new \InvalidArgumentException('The country prefix is invalid or not implemented yet');
}

but this still require to expose the hasSupportedCountryPrefix method.

We can also throw a custom VatException (?)

WDYT @dannyvankooten

@dannyvankooten
Copy link
Member

dannyvankooten commented Jan 29, 2024

Hi @VincentLanglet,

I'd like to go with throwing an InvalidArgumentException() when any of the validateVatNumber* methods are called with an unsupported country prefix. I'm fine with having an additional method to check for support before this method is called, but alternatively people could also catch the exception to accomplish the same.

Since we're not in need of this functionality ourselves and I am starting a new job soon, I don't have any time to work on this for the next few months though.

@VincentLanglet VincentLanglet force-pushed the hasPattern branch 3 times, most recently from 235d6a9 to 9717868 Compare January 29, 2024 13:59
@VincentLanglet
Copy link
Contributor Author

Hi @VincentLanglet,

I'd like to go with throwing an InvalidArgumentException() when any of the validateVatNumber* methods are called with an unsupported country prefix. I'm fine with having an additional method to check for support before this method is called, but alternatively people could also catch the exception to accomplish the same.

Hi, I changed to the throw exception behavior.

@VincentLanglet
Copy link
Contributor Author

HI @dannyvankooten ; do you have some time to review/merge this ? Thanks.

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

Successfully merging this pull request may close these issues.

add hasPattern to Validator
2 participants