Adding feature to set default currency on CakeNumber, to make repetetive... #891

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
Member

ceeram commented Oct 7, 2012

... calls to CakeNumber::currency() more DRY

@ADmad ADmad commented on the diff Oct 7, 2012

lib/Cake/Utility/CakeNumber.php
@@ -348,4 +358,17 @@ public static function addFormat($formatName, $options) {
self::$_currencies[$formatName] = $options + self::$_currencyDefaults;
}
+/**
+ * Getter/setter for default currency
+ *
+ * @param string $currency Default currency string used by currency() if $currency argument is not provided
+ * @return string Currency
+ */
+ public static function defaultCurrency($currency = null) {
+ if ($currency) {
+ self::$_defaultCurrency = $currency;
@ADmad

ADmad Oct 7, 2012

Member

Perhaps add a check for valid currency using CakeNumber::$_currencies and return false in case of invalid value.

@ceeram

ceeram Oct 7, 2012

Member

That would make it impossible to first set a default and later addFormat(), imo its up to the developer to use a sane value, but if you think it should do the check i can add it

@ADmad

ADmad Oct 7, 2012

Member

If someone else shares my opinion add it, else ignore 😃

@markstory

markstory Oct 7, 2012

Owner

Wouldn't setting the default to a value that doesn't exist cause a number of errors downstream?

@ceeram

ceeram Oct 7, 2012

Member

sure, just like passing not existing value as 2nd parameter to currency() all the method does is predefine the default value for 2nd parameter, so you dont need to write it out in the application.

For instance i used burzums Cart plugin, this method allows to change the default currency without the need to change the plugin code or use a Configure::write() in the app, and Configure::read() in the plugin.

Owner

markstory commented Oct 7, 2012

Would it also be useful to expose the default currency on the helper? That would save people having to define it in several places. It might also make sense to allow formats to be defined in the helper definition. All of this feels like a separate change though :)

Member

ceeram commented Oct 7, 2012

Changed the helper to use no default argument value anymore, and use the default defined in CakeNumber

@markstory doesnt the helper already allow formats to be defined?

Owner

markstory commented Oct 8, 2012

@ceeram Sure, but only via method calls, I was thinking it could be helpful to define the default and additional currencies in a controller's $helpers array.

Member

ceeram commented Oct 8, 2012

diff --git a/lib/Cake/View/Helper/NumberHelper.php b/lib/Cake/View/Helper/NumberHelper.php
index e286c36..ba1861a 100644
--- a/lib/Cake/View/Helper/NumberHelper.php
+++ b/lib/Cake/View/Helper/NumberHelper.php
@@ -63,6 +63,9 @@ class NumberHelper extends AppHelper {
                } else {
                        throw new CakeException(__d('cake_dev', '%s could not be found', $engineClass));
                }
+               if (!empty($settings['defaultCurrency'])) {
+                       $this->defaultCurrency($settings['defaultCurrency']);
+               }
        }

 /**

Along these lines you mean?

Member

ceeram commented Oct 8, 2012

While we are at it, do we even want to keep number helper? as its just a wrapper for CakeNumber? one can use CakeNumber::currency() just as easily in the views?

Owner

markstory commented Oct 8, 2012

NumberHelper has some HTML formatting bits that CakeNumber doesn't. I think if we can keep that separation, then its worth its weight.

Member

ceeram commented Oct 9, 2012

I must be missing something then, i dont see where it does that.

Member

dereuromark commented Oct 10, 2012

NumberHelper does not. But Time and Text do. But to keep things unified we should leave the NumberHelper. Especially with helper lazyloading the access is way more convenient than the manual Lib call.
Also, if you as a developer plan on extending and aliasing it having the helper in place makes things easier (as with the other ones).

Owner

markstory commented Oct 11, 2012

Bah, I'm sorry. I mixed up the TextHelper and NumberHelper in my head when replying last time.

Member

ceeram commented Oct 24, 2012

apart from the separate changes @markstory proposed, what more needs to be done with this?

Owner

markstory commented Oct 25, 2012

Nothing as far as I know 😄

Member

ceeram commented Oct 26, 2012

Then i like to merge this into 2.3 if no one objects

Member

dereuromark commented Oct 26, 2012

👍

Owner

markstory commented Oct 26, 2012

@ceeram go for it.

ceeram closed this Oct 26, 2012

Member

ceeram commented Oct 26, 2012

merged as single commit: f4f4aa4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment