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

Add CVR API #2

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add CVR API #2

wants to merge 1 commit into from

Conversation

bilfeldt
Copy link
Owner

Implement the CVR API

@Hasnayeen
Copy link

@bilfeldt shouldn't be the format in a separate class rather than in the driver themselves as format doesn't not differ for different driver?

Comment on lines +67 to +69
if (! $this->isValidFormat($countryCode, $vatNumber)) {
return false;
}

Choose a reason for hiding this comment

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

@bilfeldt should we not throw InvalidVatException here as it is invalid format instead of returning false? same with other two exception down the line.

Should we not throw exception here and let the package consumer handle the exception how they want to response to those exception in their app as there are 3 different scenarios are at play here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

No the point here is that if the vat number has an invalid format, then we know that it is wrong, and we should return false.

This function should only throw an exception if it is not capable of determining if the vat number is valid or not, which can only happen because of two things:

  1. Problems with the driver - those exceptions are driver specific and should bobble up.
  2. The vat number was not found and the driver is not complete, so we do not know if the number is in fact invalid or if it is just missing from the drivers dataset.

Makes sense?

@bilfeldt
Copy link
Owner Author

@bilfeldt shouldn't be the format in a separate class rather than in the driver themselves as format doesn't not differ for different driver?

No the format should live in the driver. That is because only the driver knows about it. This will also make it extendable.

Anyone can then add a new driver, with formats for some other country. They can actually also add a driver for one of the already supported countries if that mean that the formats should be different.

We could consider making a very simple FormatOnlyDriver and a corresponding config, where the user can enter hardcoded formats and then use that driver for validation. It would not support searching, but only validation, on the other hand it would not require any api lookups.

@Hasnayeen
Copy link

@bilfeldt shouldn't be the format in a separate class rather than in the driver themselves as format doesn't not differ for different driver?

No the format should live in the driver. That is because only the driver knows about it. This will also make it extendable.

Anyone can then add a new driver, with formats for some other country. They can actually also add a driver for one of the already supported countries if that mean that the formats should be different.

We could consider making a very simple FormatOnlyDriver and a corresponding config, where the user can enter hardcoded formats and then use that driver for validation. It would not support searching, but only validation, on the other hand it would not require any api lookups.

I think my point was not clear enough, apology on my part.

Let's say we're validating vat number for country X, now country X may have one or multiple formats for the vat number (may also be possible of having no format at all). Now those formats will not change no matter which service we use, if we use service A for country x or service B for country X, the format are same for both.

Now if someone want to implement a driver for a country that is not on the package, we can make the format class in a way that they can extend with macro or some other pattern.

We can also make country based format list and add only the ones we need now, that way people can contribute with format for countries that are not in the package yet and the package will have additional use case similar to country and currency repository list, people can use the package just for the valid vat format list if they don't need the vat search functionality.

@bilfeldt
Copy link
Owner Author

Hmm.. Maybe you are right. Maybe we should simply remove the format from the driver and move it to the manager?

It could then be possible to specify formats either:

  1. Formats specified (hardcoded) in this package (would require a PR)
  2. Formats provided in a config file (should overrule those defined above)

You could also ask the manager for the valid formats:

$manager->getFormats('DK')

which would check 2 first and fallback to 1?

Note that if you call the drivers check:

$manager->driver('foo')->isValidFormat('DK', 'NOT-A-VALID-FORMAT');

Then this function can choose to respect the getFormats function on the manager, but it can also just perform an API request or always return true. That will be up to the driver.

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.

2 participants