Add money factory

[sagikazarmark]

Fixes #415

[FlintCIBot]

Lint errors were found. A patch is also available.

Please see the report: https://flintci.io/repositories/3351/analyses/2547

This comment was posted by FlintCI. It can be disabled in the repository settings.

[willemstuursma]

Looks great, good naming too.

Only thing I can add is that I think the list of @method declarations should be sorted.

[Gummibeer]

The only thing I miss is the ability to create the MoneyFactory with custom Currencies classes.
At the moment they are hardcoded. 😕

[willemstuursma]

That's not possible or needed, since the generated file is part of this repository anyway.

[sagikazarmark]

@willemstuursma Great idea, will do.

@Gummibeer users of this library are not supposed to run the generator script. If you need that kind of functionality you either need to write your own factory or live with the IDE not recognizing the currency.

[Gummibeer]

@sagikazarmark that's bad - my PR supported custom Currencies which ist very handy if create custom ones.

[sagikazarmark]

Again, this file is part of this library, any modification to it should be considered a serious anti-pattern and it's behavior is undefined, not supported.

You are free to do that though, but I think the solution I provided above (creating your own factory) is the best way to achieve the same functionality.

[frederikbosch]

@sagikazarmark Great work here, I like the solution. However, I am still not convinced we should be doing this. It just feels as a bad decision to create classes for the sake of type completion in an IDE. I totally get what the advantages are, but I just feel that we should not make ourselves responsible for IDE usage. Our responsibility should stick to creating an awesome value object.

A simple alternative could be the following. Create a new repo/package called moneyphp/money-ide. Add a file with the following.

namespace Money;

final class Money {
/**
 * @method static Money AED(string $amount)
 * @method static Money ALL(string $amount)
 * ....
**/
}

So a user then has two Money files, the one from this library and the new one from moneyphp/money-ide. PHPStorm will complain about it (Multiple definitions exist for class Money), but is only a notice (not an error). Furthermore, it will give all the required type completions. Give it a try. Then people that really want it, can use it by requiring the package and our value object stays clean from IDE related features.

[frederikbosch]

I dont know if he likes it, but I am going to mention @Haehnchen in this conversation. He is the one who is responsible for creating the Jetbrains Symfony plugin. Maybe he knows a better method for creating type completion for our dynamic method problem.

p.s. Sorry Daniel if you don't like the @ mention.

[sagikazarmark]

I already tried it, I guess people could live with that.

But: I'm not a huge fan of this whole static factory thing, especially of the callStatic method. If we ever decide to rather generate static factory methods...with this solution we can.

Furthermore, I believe the Money class becomes a bit cleaner without the whole static factory thing, that's why I moved the callstatic to the generated trait as well.

The alternative would be writing a PHPStorm plugin, which would probably address all your concerns. I looked into it, did some research, and I needed half a day cooldown period after that, because my mind was blown.

To sum up: I think this is an acceptable compromise, even makes things a bit cleaner in terms of static hack moved out of money. It makes a huge step forward in DX, which at least at some level should be our goal too.

[frederikbosch]

Furthermore, I believe the Money class becomes a bit cleaner without the whole static factory thing, that's why I moved the callstatic to the generated trait as well.

I agree.

To sum up: I think this is an acceptable compromise, even makes things a bit cleaner in terms of static hack moved out of money. It makes a huge step forward in DX, which at least at some level should be our goal too.

I don't agree there. I feel our research was not broad enough. There must be more possibilities.

[frederikbosch]

Isn't there something we can do with this: https://confluence.jetbrains.com/display/PhpStorm/PhpStorm+Advanced+Metadata?

[sagikazarmark]

AFAIK it cannot add new methods, only the old API which is now deprecated.

[sagikazarmark]

I'm okay with saying we should keep looking to see if there is a better solution, but I think rejecting this solution without a viable alternative is not the way forward.

[frederikbosch]

I am not rejecting without a viable alternative. I gave one. Since we both agree on the support for the whole static factory thing, especially of the callStatic method, I rather feel more for dropping support for the whole __callStatic than extending it. At least that is a discussion we never had.

[frederikbosch]

I rather add this Money::fromString('EUR1000') and (string)$money; // EUR1000, which also gives convenience and is a similar factory, than we go forward on __callStatic.

[frederikbosch]

And/or something like Money::amount('EUR', 1000);. Then we would be rid of the __callStatic ugliness and the IDE completion.

[sagikazarmark]

Although it's not a nice solution, I believe it improves DX. I guess I will have to take my chances with brain damage and look at phpstorm plugins again. 😄

https://www.jetbrains.org/intellij/sdk/docs/phpstorm/php_open_api.html

This looks exactly what we need

[sagikazarmark]

The reason why Money::EUR is great is because it's domain specific. Any change to this will be less domain specific. The callStatic method is not a nice way of doing anything, but in this case I don't see any other option.

I spent another hour staring at Java code and I believe besides auto-completion, we need to cheat PHPStorm to handle the references well.

The more I think about this, the more I'm willing to just accept this solution as is. I believe thinking of it as a "change for the sake of IDEs" is a limited point of view. The change is for the sake of developer experience and in this case it doesn't hurt any design principles (like all the criticism we get because of the final classes). It is tested, so if something goes wrong, we will know about it. And the code is literally the same.

The solution you proposed @frederikbosch IMO is not a good one, because it doesn't make DX any better. The IDE will just complain about a different thing.

The good thing about this solution is that any time in the future if we find a better one we can just remove this madness without breaking any API. But currently I'm not convinced if that's even possible or worth the effort. (Please take into consideration that I've just spent an hour looking at java code, which probably affected my mood in a negative way 😄 ) Also, compared to a PHPstorm specific solution, (I believe) this one is universal, so it will work for those who are stuck with something else.

[frederikbosch]

Aight. Go for it!

[sagikazarmark]

Did my arguments really convince you or you just tired of the debate? 😄

[frederikbosch]

Well, I understand that this solution is OK for now and does not bring any possible problems for the future if we want to change something. That said, I’d rather have no responsibility for IDEs and DX at all.

[sagikazarmark]

I understand. We can revisit this in the future, for now this is a good enough solution. Thanks for working towards quality. 👍

[frederikbosch]

Thank you, for your recent massive contributions to the package!

[willemstuursma]

We're very happy at Mollie with this addition!