Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLDR has access to all currencies regardless of the current shop #15173

Merged
merged 3 commits into from Aug 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 31 additions & 4 deletions classes/Currency.php
Expand Up @@ -189,9 +189,9 @@ public function __construct($id = null, $idLang = null, $idShop = null)
}

if (is_array($this->name)) {
$this->name = ucfirst($this->name[$idLang]);
$this->name = Tools::ucfirst($this->name[$idLang]);
PierreRambaud marked this conversation as resolved.
Show resolved Hide resolved
} else {
$this->name = ucfirst($this->name);
$this->name = Tools::ucfirst($this->name);
}

$this->iso_code_num = $this->numeric_iso_code;
Expand Down Expand Up @@ -358,6 +358,27 @@ public function isInstalled($currencyId = null, $shopId = null)
return (bool) Db::getInstance(_PS_USE_SQL_SLAVE_)->getValue($sql);
}

/**
* Returns the name of the currency (using the translated name base on the id_lang
* provided on creation). This method is useful when $this->name contains an array
* but you still need to get its name as a string.
*
* @return string
*/
public function getName()
{
if (is_string($this->name)) {
return $this->name;
}

$id_lang = $this->id_lang;
if ($id_lang === null) {
$id_lang = Configuration::get('PS_LANG_DEFAULT');
}

return Tools::ucfirst($this->name[$id_lang]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to use ucfirst here and not above?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like $this->name has already been set in the constructor using Tools:Ucfirst(). I'm not sure if applying the transformation again in a getter context is overkill or not though... It seems fine to me as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if $this->name is a string then it means it already has been formatted in the constructor, so no need for an additional ucfirst
However in EditCurrencyHandler the $currency->name field is filled with an array, which is logical to allow updating the translated fields, but it caused an error when DefaultCurrencyInMultiShopException was thrown because the expected string field was then an array, hence this new getter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is useful but not a good idea because of the hidden context (aka the current language being inferred magically).

The proper getter would be getName($idLang) where $idLang cannot be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes except that if $this->name doesn't contain the array the method may return a string that doesn't match the asked language, which can be confusing..
Unless we store the localized names in another field $this->names in the constructor, similar to what I had to do in another PR:

if (is_array($this->name)) {

This would allow to get the name in any language regardless of the context

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, you're right. This sucks.

}

/**
* Return available currencies.
*
Expand All @@ -378,14 +399,20 @@ public static function getCurrencies($object = false, $active = true, $groupBy =
/**
* Retrieve all currencies data from the database.
*
* @param bool $active If true only active are returned
* @param bool $groupBy Group by id_currency
* @param bool $currentShopOnly If true returns only currencies associated to current shop
*
* @return array Currency data from database
*
* @throws PrestaShopDatabaseException
*/
public static function findAll($active = true, $groupBy = false)
public static function findAll($active = true, $groupBy = false, $currentShopOnly = true)
{
$currencies = Db::getInstance()->executeS('
SELECT *
FROM `' . _DB_PREFIX_ . 'currency` c
' . Shop::addSqlAssociation('currency', 'c') . '
' . ($currentShopOnly ? Shop::addSqlAssociation('currency', 'c') : '') . '
WHERE `deleted` = 0' .
($active ? ' AND c.`active` = 1' : '') .
($groupBy ? ' GROUP BY c.`id_currency`' : '') .
Expand Down
4 changes: 2 additions & 2 deletions src/Adapter/Currency/CommandHandler/EditCurrencyHandler.php
Expand Up @@ -226,7 +226,7 @@ private function assertDefaultCurrencyIsNotBeingRemovedOrDisabledFromShop(Curren
if (!in_array($shopId, $shopIds)) {
$shop = new Shop($shopId);
throw new DefaultCurrencyInMultiShopException(
$currency->name,
$currency->getName(),
$shop->name,
sprintf(
'Currency with id %s cannot be unassigned from shop with id %s because its the default currency.',
Expand All @@ -240,7 +240,7 @@ private function assertDefaultCurrencyIsNotBeingRemovedOrDisabledFromShop(Curren
if (!$currency->active) {
$shop = new Shop($shopId);
throw new DefaultCurrencyInMultiShopException(
$currency->name,
$currency->getName(),
$shop->name,
sprintf(
'Currency with id %s cannot be disabled from shop with id %s because its the default currency.',
Expand Down
4 changes: 2 additions & 2 deletions src/Adapter/Currency/CurrencyDataProvider.php
Expand Up @@ -70,9 +70,9 @@ public function getCurrencies($object = false, $active = true, $group_by = false
/**
* {@inheritdoc}
*/
public function findAll()
public function findAll($currentShopOnly = true)
{
return Currency::findAll(false);
return Currency::findAll(false, false, $currentShopOnly);
}

/**
Expand Down
8 changes: 5 additions & 3 deletions src/Core/Currency/CurrencyDataProviderInterface.php
Expand Up @@ -47,11 +47,13 @@ interface CurrencyDataProviderInterface
public function getCurrencies($object = false, $active = true, $group_by = false);

/**
* Return raw currencies data from the database reated to the current shop.
* Return raw currencies data from the database.
*
* @return array Installed currencies
* @param bool $currentShopOnly If true returns only currencies associated to current shop
*
* @return array[] Installed currencies
*/
public function findAll();
public function findAll($currentShopOnly = true);

/**
* Get a Currency entity instance by ISO code.
Expand Down
Expand Up @@ -76,7 +76,7 @@ public function isAvailable($currencyCode)
*/
public function getAvailableCurrencyCodes()
{
$currencies = $this->dataProvider->findAll();
$currencies = $this->dataProvider->findAll(false);
$currencyIds = array_column($currencies, 'iso_code');

return $currencyIds;
Expand Down