Remove final definition from classes

[steven-fox]

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 the Money 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.

[BenMorel]

Hi, classes a final for two reasons:

• favour composition over inheritance
• prevent BC breaks when class internals are changed

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.

[steven-fox]

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:

• You use type-hinting for function args and returns rigorously. I'm personally a fan of this, especially when one wants to extend, because it ensures any inheritance overrides are still (mostly) compatible with the expected call stack at runtime. This reduces the risk of simple internal changes leading to external breaking changes.
• If you believe there are internal portions of a class that need to stay internal to prevent future BC changes, perhaps those could be abstracted to private functions instead.
• As the package author, I don't believe it's your responsibility to worry about how your package will be consumed (within reason) and how that may produce breaking changes from internal alterations for certain consumers. The package should provide ease of development for the consumers to achieve X, Y, and Z. It's up to them to interface with the package properly to do so. If they want to use composition, cool. If they want to use inheritance, that's their choice. At the end of the day, they should use testing to ensure that changes to the internals of the package (with any alterations they've made on top of it) don't result in unexpected breakage. At the same time, if a breaking change has to occur with the package, that's par for the course and it's once again up to consumers to adjust their logic to suit (if they want to upgrade, that is).
• In my particular use case, inheritance would be preferred over composition purely for ease of development. When using composition, I either need to write a large amount of simple, adds-little-value code to pass calls (so the value of the package to jump-start functionality is reduced), or I can leverage magic methods to generalize the forwarding. If I use magic methods, I lose a lot of the valuable type hinting you've provided (sure, I can @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.
• Last but not least, this doesn't appear to be a package that will be frequently altered, so that again reduces the potential burden of breaking 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. :-)

[BenMorel]

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.

[bcremer]

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(),
        ];
    }
}

[andreas-freitag]

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.

[rdarcy1]

 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.

[sybbear]

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.

[sybbear]

I'd also gladly rollout a PR with the update.

[sybbear]

Created a fork and made a pull request: #52
Will use this fork for a while.

Main arguments to make the Money class extendable are these:

• Avoid forcing package users to follow a specific pattern (they should decide for themselves). People will more eagerly adopt the package if they can decide how to consume it
• Remove the need to white unnecessary code when using composition (copying all the methods and docs, when all you need is just one additional method?)
• Late static binding and autocompletion
• http://ocramius.github.io/blog/when-to-declare-classes-final/ (Scroll to when not to use final keyword - there is no interface provided)
• MIT License used by this project already removes any liability if errors happen, so package maintainers have nothing to worry about. Perhaps an additional chunk of text to README.md could add an additional check to recommend one way of consuming.

Let know if more arguments / discussions are needed?

[BenMorel]

@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.

[sybbear]

@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.

[sybbear]

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.

[BenMorel]

@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:

• if you really want to do that: encapsulation
• or, a better approach IMO would be to create a 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.

[sybbear]

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.

[BenMorel]

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:

• try to make it upstream (if it's generic enough to be included in the library)
• create a Service to perfom extra operations (name it MoneyService if it's going to do more than formatting)
• or, if you're really going to type-hint against your own class anyway (you will do this if you're subclassing!), then it does make sense to create your own Money class, and encapsulate 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!

[sybbear]

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 :)

[rdarcy1]

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.

[sybbear]

@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 😄)

[sybbear]

What do you think of adding static/self returns in methods @BenMorel, so proxy class documentation would allow consistent autocompletion? Would it break anything?

[Padam87]

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.

[simonbuehler]

@sybbear mind updating your fork to latest main, would need it in a Laravel project with some additional methods 🙏

[simonbuehler]

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

[BenMorel]

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.

[simonbuehler]

fwiw: https://laravel-news.com/composition-over-inheritance-in-final-classes

[QuentinGab]

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)

[sybbear]

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?

[BenMorel]

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.

[simonbuehler]

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?

[sybbear]

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?

[sybbear]

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 :)

[BenMorel]

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?

[steven-fox]

@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. 👍

[Padam87]

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.

[j4r3kb]

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?

[ndberg]

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.

[Arkemlar]

Just a few ideas about reaching a compromise:

1. Remove final from class but put it on methods
2. Make some ci/cd config that dynamically adds final on release so those who wants to extend must use master branch and update it manually (that is relevant when you give no guarantees about BC breaks when class internals are changed)