8003: Using System Value for Base Currency Results in Config Error. #11809

nmalevanec · 2017-10-27T11:12:18Z

Description

Fix for "Using System Value for Base Currency Results in Config Error".

Fixed Issues (if relevant)

Using System Value for Base Currency Results in Config Error #8003: Using System Value for Base Currency Results in Config Error

Manual testing scenarios

Log into Admin.
Navigate to Stores > Settings > Configuration > General > Currency Setup.
Select Use system value for Base Currency.
Click Save Config if necessary and flush the cache.
Navigate to Stores > Currency > Currency Rates.
Try Importing Currency Rates via any of the three services.
Check currency rates are successfully imported for Allowed Currencies.

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

magento-cicd2 · 2017-10-30T09:30:09Z

All committers have signed the CLA.

dmanners

I think that this plugin is not needed. From what I can see the methods getConfigDefaultCurrencies and getConfigBaseCurrencies should behave like getConfigAllowCurrencies. This method reads the data from the database but then also checks for the value set in the config. In this case it means that you will get all currencies set in the DB but also the default. Does that make sense?

nmalevanec · 2017-11-06T15:31:57Z

@dmanners correct me, if I'm wrong, but in https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Directory/Model/Currency.php#L348 I see that allowed currencies reads only from DB, and later adds base currency(which indeed reads from db and system config) to allowed currencies array. So, if in admin I'll check 'Use system value' for Allowed Currencies, I'll get only base currency code.
I've created plugin, because both Model/Currency and ResourceModel/Currency marked with @api. And as far as I know, removing or marking as deprecated methods from such classes should be avoided(I mean ResourceModel/Currency::getConfigCurrencies()). And add system config, which reads config files, as dependency to ResourceModel, which interacts with DB looks like not the best idea for me.

dmanners · 2017-11-09T14:00:42Z

@nmalevanec with regards to getConfigAllowCurrencies yes that does seem odd that it will add the base currency to the allowed currencies but I think that is another issue.

My concern with using the plugin is that this is a bug with the code code but fixing it with a plugin is not that obvious to all developers. If someone is interested in finding out how this code works it seems to be hidden.

I would actually love to see the following either been added to app/code/Magento/Directory/Model/Currency.php::getConfigDefaultCurrencies and app/code/Magento/Directory/Model/Currency.php::getConfigBaseCurrencies

Call resource model that checks database values,
Call new class that checks the config values,
Combine the results of both for the system,

In this way we would cover the database and config values and since both getConfigDefaultCurrencies and getConfigBaseCurrencies are due to return an array it should work with 1 or more currencies.

How does that sound?

nmalevanec · 2017-11-09T14:20:16Z

@dmanners Well, it's pretty doable. The only thing I'm concerning about(And this is actually one of the initial reasons to move logic into plugin) is Model/Currency already has coupling between objects more than allowed and suppressed warning about this. So adding to it another dependency like Magento\Config\App\Config\Type\System... Well, I'm not sure it's valid scenario. At least previously it wasn't.

dmanners · 2017-11-09T14:32:27Z

@nmalevanec a very valid point.

We could always consider changing the app/code/Magento/Directory/Model/Currency/Import/AbstractImport.php::_getDefaultCurrencyCodes to use a new class that is specific for our purpose. Then we could deprecate the other methods and use the @see notation to highlight the new class and methods.

nmalevanec · 2017-11-10T13:55:09Z

@dmanners Done.

dmanners · 2017-11-10T16:04:51Z

app/code/Magento/Directory/Model/CurrencySystemConfig.php

+    public function __construct(
+        System $systemConfig,
+        StoreManagerInterface $storeManager,
+        ResourceConnection $resources


I guess we do not need this if it is never assigned to anything.

dmanners · 2017-11-10T16:07:28Z

app/code/Magento/Directory/Model/Currency.php

@@ -88,6 +95,7 @@ public function __construct(
        \Magento\Framework\Locale\CurrencyInterface $localeCurrency,
        \Magento\Framework\Model\ResourceModel\AbstractResource $resource = null,
        \Magento\Framework\Data\Collection\AbstractDb $resourceCollection = null,
+        CurrencySystemConfig $currencyConfig = null,


Should this not be after the $data otherwise it will break backwards compatibility?

dmanners · 2017-11-10T16:10:19Z

app/code/Magento/Directory/Model/Currency.php

@@ -378,7 +390,8 @@ public function getConfigDefaultCurrencies()
     */
    public function getConfigBaseCurrencies()
    {
-        $defaultCurrencies = $this->_getResource()->getConfigCurrencies($this, self::XML_PATH_CURRENCY_BASE);
+        $defaultCurrencies = $this->_getResource()->getConfigCurrencies($this, self::XML_PATH_CURRENCY_BASE) ?:


What happens in the case that you have multiple stores, 1 with a database value and one without. In this case I guess we need to merge the values here.

…Config Error. #11809

nmalevanec changed the title ~~Github-8003: Using System Value for Base Currency Results in Config Error.~~ 8003: Using System Value for Base Currency Results in Config Error. Oct 27, 2017

dmanners self-assigned this Nov 6, 2017

dmanners added this to the November 2017 milestone Nov 6, 2017

dmanners suggested changes Nov 6, 2017

View reviewed changes

magento-team added the Progress: needs update label Nov 6, 2017

magento-engcom-team added the bugfix label Nov 7, 2017

magento-team added Progress: needs update and removed Progress: needs update labels Nov 9, 2017

magento-team removed the Progress: needs update label Nov 10, 2017

dmanners suggested changes Nov 10, 2017

View reviewed changes

magento-team added Progress: needs update and removed Progress: needs update labels Nov 10, 2017

nmalevanec added 3 commits November 17, 2017 11:35

8003: Using System Value for Base Currency Results in Config Error.

38735ac

8003: Using System Value for Base Currency Results in Config Error.

b63ceb9

8003: Using System Value for Base Currency Results in Config Error.

3e0c785

magento-team added the Progress: accept label Nov 21, 2017

okorshenko merged commit 3e0c785 into magento:2.2-develop Nov 21, 2017

okorshenko pushed a commit that referenced this pull request Nov 21, 2017

MAGETWO-83329: 8003: Using System Value for Base Currency Results in …

c65c016

…Config Error. #11809

magento-engcom-team added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Nov 21, 2017

nmalevanec deleted the Github-8003 branch August 21, 2018 07:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8003: Using System Value for Base Currency Results in Config Error. #11809

8003: Using System Value for Base Currency Results in Config Error. #11809

nmalevanec commented Oct 27, 2017

magento-cicd2 commented Oct 30, 2017 •

edited

Loading

dmanners left a comment

nmalevanec commented Nov 6, 2017 •

edited by magento-engcom-team

Loading

dmanners commented Nov 9, 2017

nmalevanec commented Nov 9, 2017

dmanners commented Nov 9, 2017

nmalevanec commented Nov 10, 2017

dmanners Nov 10, 2017

nmalevanec Nov 14, 2017

dmanners Nov 10, 2017

nmalevanec Nov 14, 2017

dmanners Nov 10, 2017

nmalevanec Nov 14, 2017

8003: Using System Value for Base Currency Results in Config Error. #11809

8003: Using System Value for Base Currency Results in Config Error. #11809

Conversation

nmalevanec commented Oct 27, 2017

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

magento-cicd2 commented Oct 30, 2017 • edited Loading

dmanners left a comment

Choose a reason for hiding this comment

nmalevanec commented Nov 6, 2017 • edited by magento-engcom-team Loading

dmanners commented Nov 9, 2017

nmalevanec commented Nov 9, 2017

dmanners commented Nov 9, 2017

nmalevanec commented Nov 10, 2017

dmanners Nov 10, 2017

Choose a reason for hiding this comment

nmalevanec Nov 14, 2017

Choose a reason for hiding this comment

dmanners Nov 10, 2017

Choose a reason for hiding this comment

nmalevanec Nov 14, 2017

Choose a reason for hiding this comment

dmanners Nov 10, 2017

Choose a reason for hiding this comment

nmalevanec Nov 14, 2017

Choose a reason for hiding this comment

magento-cicd2 commented Oct 30, 2017 •

edited

Loading

nmalevanec commented Nov 6, 2017 •

edited by magento-engcom-team

Loading