Closed steven-fox closed 1 year ago
Hi, classes a final for two reasons:
I'm a bit torn regarding your use case; usually I'd advise you to create your own Money class, wrapping the original Money instance, and forwarding method calls. You could then add whatever interface you need on top of it; but just to add an interface this seems like a big burden, I get it.
Let me think about it a bit more. I'd be interested to know what other people think as well, if anyone's listening.
Hey Ben,
Thanks for the quick reply.
Understood. Well, I will leave it up to you! Here's a few extra thoughts, just in case it matters:
@mixin Brick\Money\Money
, but that loses function return type hinting when in a composition context) and I find myself in the exact same situation that inheritance might put me in for internal changes.Due to the above, I'll proceed with forking the package and adjusting as needed. If you decide to remove the final designation within your version of the package, then it'll be easy to switch back over.
Cheers!
P.S. Feel free to close this issue at any time if no one else provides input soon. :-)
Thanks for the write up!
Note that this change would imply changing all new Money()
to new static()
. I'll let this settle a bit and hopefully other people will jump in to express their opinion.
I prefer to wrap third party VOs in my own namespace and treat them as a implementation detail.
This way I have full control about the public API of my VO and can add JsonSerializable
as needed.
Some of the underlying VOs methods have to be proxied, but that's a one-time-task to write.
Following a real word example from an older project of mine. The underlying Money\Money
is an implementation detail and does not leak outside my VO.
<?php
declare(strict_types=1);
namespace App\Domain;
use JsonSerializable;
use Money\Currencies\ISOCurrencies;
use Money\Formatter\DecimalMoneyFormatter;
use Money\Money as Moneyphp;
use Money\Parser\DecimalMoneyParser;
final class Money implements JsonSerializable
{
private Moneyphp $money;
/**
* @param string $currencyCode ISO 4217 Currency code
*/
public static function create(string $amount, string $currencyCode): self
{
$currencies = new ISOCurrencies();
$moneyParser = new DecimalMoneyParser($currencies);
$money = $moneyParser->parse($amount, $currencyCode);
return new self(
$money
);
}
private function __construct(Moneyphp $money)
{
$this->money = $money;
}
public function getCurrencyCode(): string
{
return $this->money->getCurrency()->getCode();
}
public function add(self $other): self
{
return new self($this->money->add($other->money));
}
public function equals(self $other): bool
{
return $this->money->equals($other->money);
}
public function jsonSerialize(): array
{
$currencies = new ISOCurrencies();
$moneyFormatter = new DecimalMoneyFormatter($currencies);
return [
'amount' => $moneyFormatter->format($this->money),
'currency' => $this->money->getCurrency(),
];
}
}
I prefer to wrap third party VOs in my own namespace and treat them as a implementation detail. This way I have full control about the public API of my VO and can add
JsonSerializable
as needed. Some of the underlying VOs methods have to be proxied, but that's a one-time-task to write.
Just to give my two cents: I fully agree with that statement and try to abstract my dependencies away adding functionality around it as needed.
prevent BC breaks when class internals are changed
Couldn't this be mitigated to a large extent by using private internal methods (as opposed to protected)? I could only see one protected method from a quick look through, so probably isn't a big change.
The only thing that would then be visible to child classes would be the public API. Arguably if this is the case you 'should' be using composition, but in the case of Arrayable etc it does feel like a lot of boilerplate code (unless you're using magic methods, which feels a bit nasty). There are of course other compatibility considerations with inheritance, but an 'extend at your own risk' note in the readme may suffice.
Personally, I don't have strong feelings either way.
Bump on this one.
Although this package is pure gold, that final keyword in this package overcomplicates the extending process creates some sort of authority on how package consumers are supposed to work with the code.
Any chance you could remove final keyword and add @return static
to phpdocs in methods?
That will help just about a lot.
In my use case, I just need a proper type-hinting (late static binding) and a couple of additional methods to make development easier. Class internals could be private and also final as well, if one wants to.
I'd also gladly rollout a PR with the update.
Created a fork and made a pull request: https://github.com/brick/money/pull/52 Will use this fork for a while.
Main arguments to make the Money class extendable are these:
Let know if more arguments / discussions are needed?
@sybbear Would you mind sharing the extra methods you're adding to the Money
subclass? I'm really curious if they belong to Money
itself.
@BenMorel
I'd like to add a few methods, that would allow me to format money in a specific way, depending on a country and use case. The formatting logic should be encapsulated in the methods and these would be the highest level functions to call without a need to provide parameters.
For example:
ExtendedMoney::setDefaultFormat();
ExtendedMoney::setDefaultLocale();
//Now these can use a default format
$money->formatWithSign();
$money->formatFloat();
//And other business/locale-aware formatting methods
There is for example a nesbot/carbon
package that is widely used in Laravel projects and it provides pretty much similar functionality, just with date/datetime values: subtraction, addition, comparison and formatting.
https://github.com/briannesbitt/Carbon.
Carbon is not final and is open for extension. It would be a breeze to follow a similar pattern.
The more precise explanation of the above example is actually PHP's DateTime class, which is in my opinion a value object. Carbon then extends it and augments with intuitive methods.
@sybbear This is actually a very good example of what I think people should not do with brick/money
. A default locale for formatting, IMO, does not belong to the Money
object, and does not make a good justification for inheritance.
What I would suggest in this case is:
MoneyFormatter
:class MoneyFormatter
{
public function __construct(private string $defaultLocale)
{
// ...
}
public function formatWithSign(Money $money): string
{
// ...
}
public function formatFloat(Money $money)
{
// ...
}
}
Regarding DateTime
, I think this is a very good counter-example as well: I will not comment on Carbon as I don't have experience with it, but my opinion is that extending this mess of a class is a bad idea, and I would definitely favour composition over inheritance to encapsulate it in a better API.
This is what I did in brick/date-time when I needed functionality that was provided by DateTime
, but didn't want to expose its poor API to the outside world. Most classes do provide a way to convert to/from DateTime
/DateTimeImmutable
though, for obvious compatibility reasons; but they never extend from it.
Hello there 🙌 Thank you for your reply. Btw, I just decided to give a bit of sponsoring to this project, because it's well done. 💪 Totally regardless of whether you agree or disagree with my arguments.
And back to the discussion.
And I do not disagree, that MoneyFormatter approach is a good one, but in our specific case I do not find it reasonable to create a new class every time we need more methods. I may need to add more methods like isNativeCurrencyOf(Country $country); isPrimaryCurrencyOf(Country $country);
So with the need for additional helpers we would need additional classes like NativeMoneyDetector
or some mixed MoneyHelper
that just holds one more two methods. To me, that creates a clutter with loads of additional classes and tests. I would like to have one child class and nicely augment it with lean business-domain logic, as a simple all-in-one solution, write one lean test class for it and just utilize across the codebase.
Another argument: Assume a method in your package contains a bug and affects business-critical functionality. The fix is not yet available, but it needs to be fixed asap. One would need to replace method/calls all around application and create a proxy for Money class, copying all the methods, just to fix the bug temporary. Allowing a consumer to extend a method would make it much easier to do a quick fix.
This post has a good discussion, both sides (also touches topic about open source libs, frameworks) https://matthiasnoback.nl/2018/09/final-classes-by-default-why/#4139236487
I see that you prefer one way of coding over another one and I see how final keyword has a value per project basis (it's wise and valid), but what is the need to enforce it in a public open-source package? 😀
I assume consumers of this package would like to have the actual functionality and not additional religion/opinions/views on how to structure THEIR code, because only they know better their needs and specs of their projects.
Thanks a lot for the support, @sybbear! I really appreciate it.
I just read the article & discussion on Matthias Noback's blog, and this only reinforced my opinion that Money
should stay final. I do understand the arguments in the opposition (comments), but I'm confident that if you need extra functionality, you should either:
MoneyService
if it's going to do more than formatting)brick/money
; at least you now have full control over your public API, and reduce the surface for bugs by not relying on Money's implementation details.I'm not very sensitive to the argument of being able to fix a bug by subclassing. If you have a fix for a bug and submit it as a GitHub PR, it will likely be merged & released within 24 hours on my projects. If you really need it now, then just use your fork temporarily in composer.json
.
The main argument against removing final
, as an open-source library maintainer, is the maintenance burden. It's complicated enough to keep backwards compatibility on the public API across versions, so adding to that backwards compatibility with internal structures is hell. And no, I don't think that you could reasonably say "remove final, but don't keep BC with internal stuff across versions, I'll deal with it", this is just opening a can of worms and making even more people unhappy on the next release.
I'll keep this issue open a little while to let things settle down and give me a chance to change my opinion, but I'm relatively confident that the current way is the right way!
Thank you for reply @BenMorel
I could imagine, if there would be a team of >10 people in one team and final keyword would communicate that "Hey, this class is supposed to work this way and this way only. Not for extension." Or "This class is still raw and I'm working on it". There is a possibility to contact a teammate and ask the exact reason why the class is final.
But I just cannot wrap my head around as to why having final in this repository is important when it is an open-source package and there is pretty much only person behind the development.
So I'd really like to hear a practical example, where not having a final keyword is a maintenance burden. Such an example would be seeing an unstoppable stream of pull requests with ChildClass extends Money
type of code. And the burden would be to keep rejecting these, as it takes time.
As for backwards compatibility, semantic versioning is the way of doing it, which is pretty much a standard nowadays, also given the fact that the package is being installed via composer. Developers are supposed to lock to a specific version and update dependencies consciously, trusting that maintainers follow the semantic versioning constraints.
As for now, there will be just a number of forks (I found 3 already) that remove the final keyword and they will be lacking automated updates, because they need to structure the code their own way. https://github.com/brick/money/network
Anyways, these are my two cents on the topic. Thank you again and hopefully we'd find a compromise one day between these ways of coding :)
As for backwards compatibility, semantic versioning is the way of doing it, which is pretty much a standard nowadays, also given the fact that the package is being installed via composer. Developers are supposed to lock to a specific version and update dependencies consciously, trusting that maintainers follow the semantic versioning constraints.
The issue here is the scope of the backwards compatibility – if you allow subclassing you should arguably ensure that all inheritable (i.e. protected) methods and properties are backwards compatible. Personally I wouldn't go that far, but I can see the merit of being explicit, especially on a popular package.
If you're being strict about it, the package may have to release a new major version when any new method is added, in case a user has already added that method in a subclass with a conflicting signature. For example, brick/money adds public function toArray(): array
and a user has already added public function toArray()
(no return type). This is a breaking change, which is avoided by making the class final.
@rdarcy1
Good point there, it is a concrete example and makes sense.
After doing some research, it came clear that adding a new method to a non-final class is considered a breaking change and it must increment a MAJOR
version, at least according to this: https://github.com/tomzx/php-semver-checker/blob/master/docs/Ruleset.md
More pieces start to fall in place, so I see what's the idea behind final here.
(Also it's time to start using a static code analyzer like https://github.com/phan/phan (scans method signatures), just in case if some packages don't follow that kind of SemVer 😄)
What do you think of adding static/self
returns in methods @BenMorel, so proxy class documentation would allow consistent autocompletion? Would it break anything?
This would be useful for overriding the default context and rounding mode as well.
I'd like to change that for my whole app to a custom context, and HALF_UP rounding mode.
I do all my calculations in rational, and I find it annoying that I have to specify these always when converting it to Money, or creating a new Money.
@sybbear mind updating your fork to latest main, would need it in a Laravel project with some additional methods 🙏
just for reference, a implementation capsulating money
and implementing Arrayable, Jsonable, Stringable, \JsonSerializable
on top of it
https://github.com/supplycart/money/blob/master/src/Money.php
https://github.com/supplycart/money/blob/master/src/Money.php
@simonbuehler This is a good example of how you can encapsulate Money
in your own VO, and doing whatever you like with it!
IMO this is the preferred way to augment the capabilities of Money.
https://github.com/supplycart/money/blob/master/src/Money.php
@simonbuehler This is a good example of how you can encapsulate
Money
in your own VO, and doing whatever you like with it!IMO this is the preferred way to augment the capabilities of Money.
Hi, jumping in the conversation. I totally understand the motivations behind final class, but I used for long time cknow/laravel-money wich wrap moneyphp and it was so painful to use because of class mixing (there were situations where both wrapper and native money class cohabited), ugly workaround and bad static analysis. I get rid of moneyphp because of this (and because your package is so beautifully written). Today, i have found a satisfying workaround with your package and my Laravel/Livewire implementation but it's still very frustating not being able to simply add one and only one method (for serialization and Livewire)
I agree on @steven-fox on this one - again :)
Also, with all respect @BenMorel , how would you suggest using composition instead of extension, if Money's methods are documented to return same Money and not static (current extension type or even mixining proxy class). And because it's final, we'd have to declare-override return type in a separate proxy decorator class for every single method to achieve this.
I mean, you cannot do method chaining with un-extended methods, because they point back to base class Money and not the actual extension.
Are you planning any time soon to upgrade return types to static?
I'm not planning to remove the final keyword from classes at the moment. As explained before, composition should be favoured over inheritance if you're planning to extend Money
. That is, you should create your own Money
class that encapsulates Brick\Money\Money
, create proxy methods for methods you use, and add your own methods.
I know this may sound cumbersome, but it is IMO the correct way to "extend" a class you don't control.
Extendability brings its own set of problems, that I don't want to have to deal with as a maintainer, most notably more surface for BC breaks.
I might miss some points but doesn't a pattern like this
public function __call(string $name, array $arguments)
{
if (! method_exists($this->money, $name)) {
throw new RuntimeException(
message: "Method [$name] does not exist.",
);
}
return $this->money->{$name}(...$arguments);
}
Allow for calling all methods of the capsulated object and adding own extension methods in the class quite easily?
No pun intended, do you guys use Notepad++ for coding? You do want to have a proper return typing when either extending or proxying classes. When having a proper IDE, we want to use method chaining to quickly see which methods are available.
So we can do
$new = $ext->baseMethod1()->newMethod2()
and still see in the IDE that $new
is still an extension (whether real or composition) and not the base class.
One can see, why you don't want to remove final
keyword.
But why wouldn't you want to upgrade return types to static
in the phpdoc, so we can use @mixin
and __call
and not redocument every method?
And apologies once again for bumping this issue in a hard-ish way, but hopefully you can also see why it is frustrating:
1) The package is extremely useful, however: 2) Money types should be easily extendable (in one way or another) , because all countries have their own laws and currency configurations. 3) Money should be easily extendable, because there are thousands of cryptocurrencies on the market.
And at the same time:
Instead, we are getting coached on software dev patterns, that we are supposed to decide on in our own
projects.
Hopefully you understand this point of view and give your package ability to be used more extensively -> and therefore more developers will use the package and donate resources, it's really in your interest if you think about it :)
But why wouldn't you want to upgrade return types to static in the phpdoc, so we can use @mixin and __call and not redocument every method?
I'm sorry this reply will fall under "getting coached on software dev patterns", but I definitely consider __call
an anti-pattern, that should only be used in rare cases and with great caution.
If anything, I'd be less strict about replacing every Money
return type with static
, rather than removing the final
keyword, but for the reason above, I'm a bit reluctant about doing something to the codebase that is only here to support (what I think is) an anti-pattern.
Money types should be easily extendable (in one way or another)...
Potentially agree, if you consider that extendability is not limited to extends
. brick/money
is extendable through custom Currency
instances, custom CurrencyConverter
implementations, custom Context
implementations, and whatever interface is/will be necessary to support controlled extendability.
...because all countries have their own laws and currency configurations.
The aim of this library is to provide the foundations to support all countries/currencies. What is it missing to support this use case? Shouldn't this be merged in brick/money
itself?
Money should be easily extendable, because there are thousands of cryptocurrencies on the market.
Brick/money already supports cryptos through custom Currency
instances. If you believe that it should provide more features to support cryptos, then maybe these features could be merged in brick/money
as well?
Finally, if you really need to decorate Money
, which I can understand, wouldn't a decorator generator solve your problem, which—if I understand correctly—is that you don't want to manually implement proxy methods?
@BenMorel Let’s proceed to close this issue to make your life easier. You make perfectly good arguments that favor keeping things the way they are. No need to add more comments to this issue, taking up more of your time, in my eyes. For those that want to extend, pick one of the options and move forward. If you desperately want to change something that doesn’t jive with Ben’s approach, maintain your own fork. Easy peasy lemon squeezy. Happy coding everyone. 👍
This is the reason why I'm gonna make my own money lib for the next project I need one.
The other money lib is weird to use with instanciating money objects with minor amounts by default, eg 500
meaning 5 in dollars.
This one is closed to the point that I would need to decorate almost every single method to use, and at that point I prefer to write my own.
How to solve the below "problem":
I need a Salary
VO. Obviously, a salary shouldn't be negative or zero and since Money
class allows for negative values I would do something like this:
class Salary extends Money
{
public function __construct(
int $amount,
string $currency
) {
if ($amount <= 0) {
throw new \Exception();
}
return self::of($amount, $currency);
}
}
Sure I can store Money
in the Salary
VO in a value
property but then I would have to rewrite all the methods (if needed) of the Money
class to be able to execute them on Salary
. Doing Salary->value->getAmount()
is breaking the law of Demeter.
Add __call
function to Salary
and forward all method calls to Money
in the value
property?
Thanks for this great library.
But I don't get why you don't want to change new Money()
to new static()
which would take out a lot of pain for users which want to add a little method, which is now only possible with composition and a huge amount of work.
E.g. if I want to use ->toRational()
, you have to composite the RationalMoney class as well.
While this is your code base and you are free to do whatever you want, I just don't get the advantage for you.
Would be thankful if you could consider this change again.
Just a few ideas about reaching a compromise:
Good afternoon,
I was wondering if we could safely drop the
final
definition from the classes within the package. I understand the value of defining certain classes as such, but I could see several valid use cases where extending theMoney
class, for example, would make sense.As a current use case, I'd like to do this in a Laravel app to add the Arrayable and JsonSerializable interfaces to the Money class. I don't think this should be a part of this package, so it makes more sense to permit extension by package consumers.
Without the ability to extend, I'll have to throw together a factory and forward calls using magic methods. Yuk. Or make a fork. Also seems unnecessary.
If there is a particular reason why the classes are defined as final in the package, I'd appreciate a quick explanation so that I can decide how to proceed from there.
Thanks for your time.