Directory currency - performance optimalization

[Sekiphp]

Description (*)

Lowering count of SQL queries when someone use this methods:

• getConfigAllowCurrencies
• getConfigBaseCurrencies

Fix unique currency which is at the end of string for mthod getConfigAllowCurrencies.

Manual testing scenarios getConfigBaseCurrencies (*)

Call this code:

var_dump(Mage::getModel('directory/currency')->getConfigBaseCurrencies());
var_dump(Mage::getModel('directory/currency')->getConfigBaseCurrencies());

Database:

SQL queries before: 2
SQL queries after: 0
Output from method is in same format.

Manual testing scenarios getConfigAllowCurrencies (*)

**For this data in database: **

Call this code:

print_r(Mage::getModel('directory/currency')->getConfigAllowCurrencies());
print_r(Mage::getModel('directory/currency')->getConfigAllowCurrencies());

Output before changes:

Array
(
    [0] => BAM
    [2] => BGN
    [6] => CZK
    [9] => EUR
    [14] => HUF
    [15] => PLN
    [17] => PLN

    [18] => RON
    [20] => RSD
    [21] => RUB
    [22] => USD
)

Output afterchanges:

Array
(
    [0] => BAM
    [2] => BGN
    [6] => CZK
    [9] => EUR
    [14] => HUF
    [15] => PLN
    [18] => RON
    [20] => RSD
    [21] => RUB
    [22] => USD
)

Related SQL queries before:

Related SQL queries after:

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)

[midlan]

why not change only the Mage_Directory_Model_Resource_Currency::getConfigCurrencies to use config (which is cached) ?

[colinmollenhour]

Agreed, there doesn't seem to be any good reason to directly query the config table so getConfigCurrencies could simply be:

    /**
     * @deprecated
     */
    public function getConfigCurrencies($model, $path)
    {
        return (string) Mage::app()->getConfig()->getNode($path, 'default');
    }

[Sekiphp]

you can't do this return explode(',', (string) Mage::app()->getConfig()->getNode($path, 'default'));, bcs. you can customize settings per website/store scope

[tmotyl]

This sounds like a nice improvement.
What scenarios should be tested before the merge?

• product , cart, checkout flow
• switching currency by selector (the same store view)
• switching currency in code using simulated environment
• checkout when base currency is different with current
• what else?

[Sekiphp]

@colinmollenhour @midlan

Or alternative solution (in my opinion us ugly, but complete without any additional SQL query):

    public function getConfigCurrencies($model, $path)
    {
        $result  = [];

        // default
        $result = array_merge($result, explode(',', trim(Mage::app()->getConfig()->getNode($path, 'default'))));

        // stores
        foreach (Mage::app()->getStores(true) as $store) {
            $result = array_merge($result, explode(',', trim(Mage::app()->getConfig()->getNode($path, 'stores', $store->getCode()))));
        }

        // websites
        foreach (Mage::app()->getWebsites(true) as $website) {
            $result = array_merge($result, explode(',', trim(Mage::app()->getConfig()->getNode($path, 'websites', $website->getCode()))));
        }

        sort($result);

        return array_unique($result);
    }

[midlan]

@Sekiphp I like it! I would only create variable for Mage::app()->getConfig()

[Sekiphp]

@midlan Yeah, I can do it, it can improve readability of code

[Sekiphp]

@midlan @colinmollenhour @tmotyl @FreeWall Final solution is here :-)

[midlan]

approved
worth to the note that the trim seems fixing an issue that occurred elsewhere (probably the config edit form)

[colinmollenhour]

Very nice. May I suggest a few improvements?

1. Use array_filter on the final result in case the config value is empty for one or more stores. The result of explode(',','') is [''] not [] so the empty string would end up as one of the elements in result.
2. Cache the result in case the method is called multiple times in one process. (maybe it never is in which case don't bother)
3. Skip inactive stores/websites unless there is a reason to keep currencies that are only active for disabled stores.

The base branch was changed.

[github-actions]

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
2 runs  ±0  2 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit ca3d916. ± Comparison against base commit a4b82bf.

[Sekiphp]

@colinmollenhour Can you create issue for your suggestions?