magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.5k stars 9.3k forks source link

System config backend_model not used when reading from ScopeConfigInterface::getValue() #10457

Closed careys7 closed 6 years ago

careys7 commented 7 years ago

Preconditions

  1. Magento 2.1.7

Steps to reproduce

  1. Create a system.xml configuration that includes a backend_model node, eg:
<field id="country_mapping" translate="label comment" type="text" sortOrder="20" showInDefault="1" showInWebsite="1" showInStore="1">
  <label>Country Mappings</label>
  <frontend_model>Namespace\Extension\Block\Adminhtml\System\Config\Form\Field\CountryMappings</frontend_model>
  <backend_model>Magento\Config\Model\Config\Backend\Serialized\ArraySerialized</backend_model>
</field>
  1. Attempt to retrieve configuration value on frontend, eg:
$this->scopeConfig->getValue('shipping/extension/country_mapping')

Expected result

  1. ArraySerialized::_afterLoad() is called

Actual result

  1. Method is not called, data retrieved is still serialised after being retrieved from the cache.
  2. Magento\Framework\App\Config\Value::_afterLoad() is only called when rendering admin configuration forms

Related: https://github.com/magento/magento2/issues/7741

Is this a feature or a bug? In the case of ArraySerialized, it means that any use of the configuration value in other parts of the application need to handle unserialization in two places:

korostii commented 7 years ago

Definitely a bug, and a PR fixng it was merged into develop less than a month ago: #7742 But if you'd like to see it on 2.1.x branch, you might consider creating an extra pull request, targeting 2.1-develop and referencing the previous one.

careys7 commented 7 years ago

In my case I had no default value configured. Is it possible that the following use case is not resolved by #7742?

I found the results of $this->scopeConfig->getValue('x/y/z') were not in a type that would suggest _afterLoad() had been called when used on the frontend. I was expecting an array, but consistently returned a serialized string.

It was as if core_config_data's value field had been persisted to cache without having been processed by the backend model's _afterLoad() at any stage.

magento-engcom-team commented 7 years ago

@careys7, thank you for your report. We've created internal ticket(s) MAGETWO-80296 to track progress on the issue.

adrian-martinez-interactiv4 commented 6 years ago

I'm working on it #SQUASHTOBERFEST, can I be assigned to this issue?

sidolov commented 6 years ago

Hi @careys7, looks like described behavior caused by configuration mismatch. Try to declare backend_model for field "country_mapping" in config.xml file: <country_mapping backend_model="Magento\Config\Model\Config\Backend\Serialized\ArraySerialized"/>

after this fix expression $this->scopeConfig->getValue('shipping/extension/country_mapping') will return unserialized array.

Could you please take a look at provided solution and leave feedback? Thanks!

careys7 commented 6 years ago

Hey @sidolov

Thanks for the feedback! It doesn't look like this is applied consistently in the core, can you think of why?

(eg NewRelicReporting, ProductAlert, Tax, etc)

sidolov commented 6 years ago

@careys7 looks like it was overlooked during the implementation. Maybe it makes sense to show raw value for some cases. Is this issue still actual for you or we can close it?

careys7 commented 6 years ago

This looks resolved now, thanks for investigating @sidolov

If I understand correctly configuration options that require a backend model must also have a default configuration option node entered under config.xml. I will take a look at the devdocs and open a PR to document this if it's missing. Thanks again

sidolov commented 6 years ago

Thanks @careys7, it will be great if you update devdocs!