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

[3.x] Allow RationalMoney instances as base price #14

Merged
merged 11 commits into from
Mar 6, 2023

Conversation

voidgraphics
Copy link
Member

Almost all of the \Brick\Money\Money typehints have been replaced with \Brick\Money\AbstractMoney, so that instances of \Brick\Money\RationalMoney can also be used as a base for Price instances.

This is necessary when doing operations without rounding in order to avoid errors in the end result.

An example of the problem:

We have a base price of 1000 minor units, that we need to divide by 12 and then multiply by 11.

1000 / 12 * 11 = 916,6666666667

Using the regular Brick\Money\Money class forces us to specify a rounding mode when doing the division, which means we have a rounded result before doing the multiplication, which introduces an error in the result:

CleanShot 2023-03-02 at 11 44 08@2x

The solution is to build the Price instance with a base Brick\Money\RationalMoney instance instead, which represents the amount as a fraction and thus does not require rounding.

CleanShot 2023-03-02 at 11 44 45@2x

Unfortunately, the change to AbstractMoney typehints in the Whitecube\Price\PriceAmendable interface is a breaking change, so this requires publishing a new major version of this package (3.x ?).

My changes in this PR are quite conservative, they require the developer to pay attention to what they are doing and puts the responsibility on them to know when they will need to use RationalMoney instead of regular Money instances.

I think this is dangerous. Maybe this package should instead always use RationalMoney under the hood, so that division/multiplication is always handled correctly. I don't think there would be any downsides to that approach.

@toonvandenbos
Copy link
Member

Maybe this package should instead always use RationalMoney under the hood, so that division/multiplication is always handled correctly.

Good point. I'll need to check the implications first.

@voidgraphics
Copy link
Member Author

Tests fail on PHP 7.4 because of the static return type.

@@ -3,28 +3,23 @@
namespace Whitecube\Price;

use Brick\Money\Money;
Copy link
Member

Choose a reason for hiding this comment

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

Remove use statement if unused?

Copy link
Member Author

Choose a reason for hiding this comment

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

Used on lines 114 & 131

Comment on lines 5 to 9
use Whitecube\Price\Price;
use Whitecube\Price\Modifier;
use Whitecube\Price\PriceAmendable;
use Brick\Money\AbstractMoney;
use Brick\Money\Money;
Copy link
Member

Choose a reason for hiding this comment

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

Remove unused use statements.

Copy link
Member Author

Choose a reason for hiding this comment

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

Used on lines 110, 118, 124 & 128

*/
protected function applyModifier(PriceAmendable $modifier, array $result, $perUnit, $postVat = false, Money $exclusive = null, Vat $vat = null)
protected function applyModifier(PriceAmendable $modifier, array $result, $perUnit, $postVat = false, AbstractMoney $exclusive = null, Vat $vat = null): array
Copy link
Member

Choose a reason for hiding this comment

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

Missing types for $perUnit & $postVat

*/
static public function formatUsing($formatter) : CustomFormatter
static public function formatUsing(mixed $formatter): CustomFormatter
Copy link
Member

Choose a reason for hiding this comment

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

$formatter can be string, callable or CustomFormatter, which could be specified for IDE users instead of mixed.

Suggested change
static public function formatUsing(mixed $formatter): CustomFormatter
static public function formatUsing(string|callable|CustomFormatter $formatter): CustomFormatter

* @throws \InvalidArgumentException
*/
protected function makeModifier($type, $modifier, $arguments = [])
protected function makeModifier(string $type, mixed $modifier, array $arguments = []): PriceAmendable
Copy link
Member

Choose a reason for hiding this comment

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

$modifier can either be string, callable, AbstractMoney , Price or PriceAmendable

Suggested change
protected function makeModifier(string $type, mixed $modifier, array $arguments = []): PriceAmendable
protected function makeModifier(string $type, string|callable|AbstractMoney|Price|PriceAmendable $modifier, array $arguments = []): PriceAmendable

*/
public function addModifier($type, $modifier, ...$arguments)
public function addModifier(string $type, mixed $modifier, ...$arguments): static
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function addModifier(string $type, mixed $modifier, ...$arguments): static
public function addModifier(string $type, string|callable|AbstractMoney|Price|PriceAmendable $modifier, ...$arguments): static

*/
public function addDiscount($modifier, ...$arguments)
public function addDiscount(mixed $modifier, ...$arguments): static
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function addDiscount(mixed $modifier, ...$arguments): static
public function addDiscount(string|callable|AbstractMoney|Price|PriceAmendable $modifier, ...$arguments): static

*/
public function addTax($modifier, ...$arguments)
public function addTax(mixed $modifier, ...$arguments): static
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function addTax(mixed $modifier, ...$arguments): static
public function addTax(string|callable|AbstractMoney|Price|PriceAmendable $modifier, ...$arguments): static

*/
public function setUnits($value)
public function setUnits(mixed $value): static
Copy link
Member

Choose a reason for hiding this comment

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

(Not sure about this one)

Suggested change
public function setUnits(mixed $value): static
public function setUnits(float|int|string $value): static

@toonvandenbos
Copy link
Member

... and so on. Same remarks can be applied everywhere.

@voidgraphics voidgraphics merged commit 92988a1 into master Mar 6, 2023
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.

None yet

2 participants