deal with integer limits

[frederikbosch]

Solves #111.

Probably has quite some impact on the library. Review carefully. Rather see some more comments on the design decisions than on some PSR-2 compliance. The latter will be solved - if there is any compliance oversights - when there is agreement on the design.

Hard aspect of this PR was rounding in GMP and BC Math libraries. Had a look at some other libraries for this to implement.

[frederikbosch]

In the next few days I will write something on the design decisions I made. This will help reviewing the PR (I guess).

[sagikazarmark]

Sounds good. Merry Christmas 😄

[frederikbosch]

While creating this PR, I had the following things in mind.

1. It must be possible to change money calculations per math library. Therefore there should be an interface that has to be implemented per library: CalculatorInterface.
2. The calculators we are looking for are: PHP, BC and GMP.
3. A calculator is a static property of the Money value object. I cannot see any scenario where you want to mix calculators.
4. It should be possible by using the API to change the (static) calculator.
5. All calculations inside Money class should be replaced by calls to the active calculator
6. String are now allowed inside Money because libraries like GMP and BC express numbers as strings.
7. Checks that are related to native php calculations, like assertIntegerBounds, should be moved to the PhpCalculator.
8. When possible all calculators must implement all functionality that is now in use by the Money class

Things I came across into when creating this PR.

1. Both BC and GMP libraries do not have every rounding method build-in. The GMP library does not have rounding tie-breakers. But since we do not need extreme precision - we want (big) integers without decimals - rounding can be done by simple string conditionals.
2. The BC library does not convert '0.500000' to '0.5'
3. The GMP library does not recognize decimals at all, calculations like multiply should convert numbers to non-decimals before calculation, and convert back after finishing calculations. This is done by removing the decimal sign (multiply by eX) and the reinsert the decimal sign after calculation.
4. Divisions in GMP are done by multiplying with 1 / divisor.
5. I changed the body of the allocateTo method, with a simpler (one-line) call to allocate.

Please let me know whether you need more clarification.

[sagikazarmark]

CalculatorInterface

No need for the Interface suffix.

It should be possible by using the API to change the (static) calculator.

Is there a reason for that? Does it make sense to allow the user to change the priority order we define?

String are now allowed inside Money because

Soes it mean strings are allowed from the public API as well? I am not a big fan of the idea, because it requires more validation logic than necessary.

When possible all calculators must implement all functionality that is now in use by the Money class

If we have an interface then this assumption is moved to the fact that the interface must be implemented by all calculators.

The BC library does not convert '0.500000' to '0.5'

Is this a problem? With don't need decimal precision.

[sagikazarmark]

Please add yourself as an author (if you are OK with that) and remove the rest from here.

[sagikazarmark]

Nice PR @frederikbosch.

I added some comments, mostly related CS thing, but there are some design questions as well.

[frederikbosch]

@sagikazarmark Thanks for the comments, fixed most of the issues you brought up. Regarding your questions.

1. No need for the Interface suffix. Removed it.
2. Does it make sense to allow the user to change the priority order we define? Changed it to registerCalculators as you suggested.
3. Does it mean strings are allowed from the public API as well? I am not a big fan of the idea, because it requires more validation logic than necessary. The string is validated
4. If we have an interface then this assumption is moved to the fact that the interface must be implemented by all calculators. Right, but libraries have limitations themselves, like I said in my previous comments, e.g. GMP does not have rounding build-in. I tried to create a workaround for the limitations.
5. Is this a problem? With don't need decimal precision. It is not, but it should be taken into account.

[sagikazarmark]

I think having a singular method would be better. I would also add a check whether the class exists at all and implements our Calculator interface. Also, why default? It could be just calculators IMO

[sagikazarmark]

Also, a new calculator should be added to the beggining of the list to make sure custom calculators are checked first.

[sagikazarmark]

The string is validated

But is it necessary to be a public detail? IMO we can do it internally, but from a public API point of view, accepting integers is fine.

[frederikbosch]

@sagikazarmark No, that is whole point of no integer limits, right? Big integer (bigger than php's max int) can only be expressed in string, not in integers.

[sagikazarmark]

Ok, rebased. Make sure to checkout your nextrelease branch as a new local branch.

[frederikbosch]

@sagikazarmark Thanks! Will see if I can finish the PR today.

[frederikbosch]

I think the PR should be ready. Added documentation to the Calculator class. Used {@inheritdoc} blocks in the calculator implementations.

[sagikazarmark]

Missing newline

[frederikbosch]

I don't get why this is necessary, but OK, I will add it.

[sagikazarmark]

Consistency.

[sagikazarmark]

Looks good to me. I added a few more CS related things, can you please correct those? The rest can be automatically fixed by CS fixer.

[frederikbosch]

Fixed things, except for the docblock. You sure you want to remove @param and @return there?

[sagikazarmark]

For now, yes. I am aware of the inheritdoc recommended usage, but it is a common pattern to use it for all kinds of docblocks. IDEs recognize it, even if phpDocumentor says otherwise. There is demand for complete docblock inheritance, and phpDocumentor is working on it for some years now.

[frederikbosch]

OK, removed the @param and @return blocks where {@inheritdoc} is used.

[sagikazarmark]

Cool, thanks. Ready for merge then?

[frederikbosch]

I would say so :)

[sagikazarmark]

Thanks @frederikbosch

[frederikbosch]

Great! Thanks @sagikazarmark for reviewing! Maybe we can create an alpha release? I would be happy with some sort of tag.

#111 can also be closed.

[sagikazarmark]

#111 should automatically be closed.

Tagging sounds good, but we should first rebase/merge into master.

[frederikbosch]

I would just rename branches in such cases ;) But I do not if that has any negative side-effects.