spaze / vat-calculator

[EOL, use driesvints/vat-calculator] Handle all the hard stuff related to EU MOSS tax/vat regulations, the way it should be. Fork of mpociot/vat-calculator, standalone, PHP 7.3+, modernized, with more features.
MIT License
48 stars 2 forks source link

Invalid VAT throws `VatCheckUnavailableException` #13

Closed JakubJachym closed 4 years ago

JakubJachym commented 4 years ago

Hi @spaze, not sure if it's a bug report or a feature request or just a BS. But I'll report it anyway and you decide what to do with it. 😉

If user enters invalid VAT ID (e.g. "RANDOMIDLOL") and it goes through getVatDetails() function the VatCheckUnavailableException is thrown. However, the triggering / parent SoapFault exception returns Invalid_input string as an exception's message, which makes me believe we can have something like this:

if ($e->getMessage() === "Invalid_input") {
    return new VatDetails(false, $countryCode, $vatNumber, null);
    // Or `throw new InvalidVatIdException(...);` if it makes more sense
}

To simply state the fact the VAT ID is invalid. It feels to me like the VatCheckUnavailableException should be only thrown if the EU API is down = unavailable. But if it actually worked and said: "Dude... what?" we can pass that info back, or?

And yeah, I've read the documentation.

The VAT number should be in a format specified by the VIES.

But still... 😏

Cheers, and thanks for a great fork.

spaze commented 4 years ago

Hey Jakub!

Thanks for the "bug report or a feature request" :-) It's definitely not a BS, I like the idea and you're totally right, throwing VatCheckUnavailableException in this case doesn't make much sense.

However and in general, I don't like comparing strings that are labeled as message ($e->getMessage()) because they might change and before you know it, the server returns Invalid_yabbadabbadoo (or oh come on, that's an invalid id! or something) and this check stops working. I'd be fine if there would be a "contract" of some sort but the only documentation I was able to find is in the WSDL itself:

In case of problems, the returned FaultString can take the following specific values:

  • INVALID_INPUT: The provided CountryCode is invalid or the VAT number is empty;

That WSDL doc seems like a "weak contract" to me and I'd rather not rely on that. I'd consider it a "strong contract" if there would be a special method like getErrorCode() or something. But! Reading that I've realized that the reason for "Invalid_input" is because the country code is invalid. I mean, they just literally say that, right? :-)

A short test with a valid country but invalid VAT id:

[...]
>>> $p = ['countryCode' => $countryCode, 'vatNumber' => $vatNumber, 'requesterCountryCode' => null, 'requesterVatNumber' => null]
=> [
     "countryCode" => "CZ",
     "vatNumber" => "NDOMLOLFOOBARBAZ1234657981212584",
     "requesterCountryCode" => null,
     "requesterVatNumber" => null,
   ]
>>> $c->checkVatApprox($p)
=> {#2361
     +"countryCode": "CZ",
     +"vatNumber": "NDOMLOLFOOBARBAZ1234657981212584",
     +"requestDate": "2020-09-09+02:00",
     +"valid": false,
     +"traderName": "---",
     +"traderCompanyType": "---",
     +"traderAddress": "---",
     +"requestIdentifier": "",
   }

And you see that the request went through and it returns "valid": false.

So maybe what I could do instead of

if ($e->getMessage() === "Invalid_input") {

is to check whether the country code is supported. The country codes are defined as array keys in VatRules::$taxRules, so maybe a simple isset(taxRules[countryCode]) check would do. We'd even save a SOAP call which would provide the same information too.

I just need to decide whether to return false in this case or throw some "InvalidCountry" exception. I think it should be the latter but I'll think about it a bit more.

What do you think?

For the sake of completeness, these are all the error states available in the WSDL doc:

  • GLOBAL_MAX_CONCURRENT_REQ: Your Request for VAT validation has not been processed; the maximum number of concurrent requests has been reached. Please re-submit your request later or contact TAXUD-VIESWEB@ec.europa.eu for further information": Your request cannot be processed due to high traffic on the web application. Please try again later;
  • MS_MAX_CONCURRENT_REQ: Your Request for VAT validation has not been processed; the maximum number of concurrent requests for this Member State has been reached. Please re-submit your request later or contact TAXUD-VIESWEB@ec.europa.eu for further information": Your request cannot be processed due to high traffic towards the Member State you are trying to reach. Please try again later.
  • SERVICE_UNAVAILABLE: an error was encountered either at the network level or the Web application level, try again later;
  • MS_UNAVAILABLE: The application at the Member State is not replying or not available. Please refer to the Technical Information page to check the status of the requested Member State, try again later;
  • TIMEOUT: The application did not receive a reply within the allocated time period, try again later.

And they all look like VatCheckUnavailableException to me.

Happy also that you like the fork, thank you!

JakubJachym commented 4 years ago

Hey Michal, yeah, I agree with you. The if ($e->getMessage() === "Invalid_input") part hurts me too.

I have to admit I haven't checked the WSDL doc myself. 😞 So thank you for doing it for me. Yeah, I agree that checking if the Country Code is supported should be good enough. And the InvalidCountryException sounds great!

But anything will work for me. :smile: I just need to distinguish if the VAT ID is booboo because of the API went tits up (as it did today...) or the user had a fun time in the input field.

Cheers!

spaze commented 4 years ago

You'll now (once released) get UnsupportedCountryException when you pass an invalid country.

Thanks again.

spaze commented 4 years ago

After manually adding a non-EU country like Norway with addRateForCountry('NO'), UnsupportedCountryException wouldn't be thrown with calls like getVatDetails('NO123456'). Reopening.