{"action":"created","issue":{"url":"https://api.github.com/repos/mathiasverraes/money/issues/95","labels_url":"https://api.github.com/repos/mathiasverraes/money/issues/95/labels{/name}","comments_url":"https://api.github.com/repos/mathiasverraes/money/issues/95/comments","events_url":"https://api.github.com/repos/mathiasverraes/money/issues/95/events","html_url":"https://github.com/mathiasverraes/money/pull/95","id":74014663,"number":95,"title":"[RFC] Add CurrencyInterface and CurrencyLoader","user":{"login":"giosh94mhz","id":5219975,"avatar_url":"https://avatars.githubusercontent.com/u/5219975?v=3","gravatar_id":"","url":"https://api.github.com/users/giosh94mhz","html_url":"https://github.com/giosh94mhz","followers_url":"https://api.github.com/users/giosh94mhz/followers","following_url":"https://api.github.com/users/giosh94mhz/following{/other_user}","gists_url":"https://api.github.com/users/giosh94mhz/gists{/gist_id}","starred_url":"https://api.github.com/users/giosh94mhz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/giosh94mhz/subscriptions","organizations_url":"https://api.github.com/users/giosh94mhz/orgs","repos_url":"https://api.github.com/users/giosh94mhz/repos","events_url":"https://api.github.com/users/giosh94mhz/events{/privacy}","received_events_url":"https://api.github.com/users/giosh94mhz/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2015-05-07T14:52:34Z","updated_at":"2015-05-07T18:16:49Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mathiasverraes/money/pulls/95","html_url":"https://github.com/mathiasverraes/money/pull/95","diff_url":"https://github.com/mathiasverraes/money/pull/95.diff","patch_url":"https://github.com/mathiasverraes/money/pull/95.patch"},"body":"I've shared with you my fork, since I really think this will improve the library usability (i.e. integration is existing systems).\r\n\r\nFirst and foremost: the implementation of the `Money` class is great! Invariant Final Value Object are great for money, and prevent silly update-by-reference errors.\r\n\r\nThe main limitation I found is the `Currency` class. The current limitation are:\r\n* all currencies are pre-loaded, no matter how or when you use it (see my issue #94);\r\n* currencies include all(?) and only ISO code, so you cannot use \"imaginary currencies\" (gold, credit, HyruleRupees, ... you said in another PR that can be done, but I can't see how probably)\r\n* currency only property is the code, and... well... that's not very useful.\r\n\r\nThe solution seems simple at first: replace or wrap (not extend!) the default currency, and provide my decorated version!\r\n\r\nBut this is not possibile, for multiple reasons:\r\n* Money class depends on `Currency` implementation, not to abstraction, and so there is no way of providing new behaviour (no Dependency inversion);\r\n* Money class allocate Currency instances directly (Multiple Responsibilities);\r\n* it is bound to a specific set of currencies, since only so I cannot even add currency if I use this implementation (not Open/Closed).\r\n\r\nI know, that you did this to provide Immutable Value Object instances, and this is great when it comes to money, but for Currency, the Immutability condition is not required (as long you don't change the currency code, of course); as in the [original Value Object Pattern](http://martinfowler.com/bliki/ValueObject.html), value Object only require that object are compared by value, immutability is just a good heuristic.\r\n\r\nSo, what have I done to the library?\r\n* Currency is a `final` implementation of `CurrencyInterface`;\r\n* Money depends on `CurrencyInterface`;\r\n* Money class use an instance of `CurrencyLoader` to allocate currency (preconfigured with ISO code for backward compatibility);\r\n\r\nAll tests still pass and it is fully backward compatible. The only drawback I can think of is that if an application use a custom implementation of Currency, it should not change the currency code at runtime if there are Money instances.\r\n\r\nTo avoid BC I left the following things which I'm not \"proud\" of:\r\n* use `Currency`+`CurrencyInterface` instead of `Currency`+`ISOCurrency` (or `DefaultCurrency`);\r\n* `Money::registerLoader` automatically initialize with `ISOCurrencyLoader`;\r\n* config file `currencies.php` is included twice in  `ISOCurrencyLoader` and `ISOCurrencies`;\r\n* `Currency::getName` is deprecated for code, and should be reused to return the ISO name.\r\n\r\nI hope to hear your feedback, and maybe the feedback from the community.\r\n\r\nRelated PR:\r\n* #71 for iso loader implementation\r\n* #94 lazy loading\r\n* #72 where you and @sagikazarmark discussed this same patch.\r\n"},"comment":{"url":"https://api.github.com/repos/mathiasverraes/money/issues/comments/99965878","html_url":"https://github.com/mathiasverraes/money/pull/95#issuecomment-99965878","issue_url":"https://api.github.com/repos/mathiasverraes/money/issues/95","id":99965878,"user":{"login":"sagikazarmark","id":1226384,"avatar_url":"https://avatars.githubusercontent.com/u/1226384?v=3","gravatar_id":"","url":"https://api.github.com/users/sagikazarmark","html_url":"https://github.com/sagikazarmark","followers_url":"https://api.github.com/users/sagikazarmark/followers","following_url":"https://api.github.com/users/sagikazarmark/following{/other_user}","gists_url":"https://api.github.com/users/sagikazarmark/gists{/gist_id}","starred_url":"https://api.github.com/users/sagikazarmark/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sagikazarmark/subscriptions","organizations_url":"https://api.github.com/users/sagikazarmark/orgs","repos_url":"https://api.github.com/users/sagikazarmark/repos","events_url":"https://api.github.com/users/sagikazarmark/events{/privacy}","received_events_url":"https://api.github.com/users/sagikazarmark/received_events","type":"User","site_admin":false},"created_at":"2015-05-07T18:16:49Z","updated_at":"2015-05-07T18:16:49Z","body":"Actually you confused me a little bit.\r\n\r\n> In fact I think that currency should be always passed in the constructor.\r\n\r\nIsn't it right now?\r\n\r\n> The only reason I've introduced this loading login is to allow \"Money::CURRENCY\"\r\n\r\nCan't you do this currently?\r\n\r\n> be forced to only have the \"code\" property\r\n\r\nThat's where I disagree. The point of currency is to provide information about the money value and nothing more. Any further information should be stored elsewhere, where you can actually use the Currency object as the key.\r\n\r\n> I have currency name, number of decimal precision\r\n\r\nI admit that this is related to the currency. However the point of the Currency object is to let you identify. So this kind of data should be stored elsewhere. (Since you mentioned doctrine: Currency is an association, not an entity). At least this is the case now, but we can argue about it.\r\n\r\n> Also, these informations are saved on a backend (i.e. Doctrine), so always duplicating force the application to have and use adapters.\r\n\r\nSorry, what? :smile: I have my own doctrine types and extensions for money which work just fine.\r\n\r\n> you can load all the currency at once, or load only the one use need (e.g. from DB).\r\n\r\nThat's again something that is out of scope. If I were you, I would create a factory wrapper which contains all your custom knowledge to load currencies and any further custom logic. Also, check the ISOCurrencies, which itself does the currency loading and can check if it is in your custom set.\r\n\r\nIt seems to me that (almost) everything you want is already in the library. That extra which is not should be implemented as custom logic IMHO."}}