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.31k forks source link

Dynamically created config fields filtered out in the save controller (M2.3.2) #23423

Closed mattijv closed 5 years ago

mattijv commented 5 years ago

Summary

We have a module that adds config fields dynamically to the admin shipping method page. When testing the module in M2.3.2 we ran into an issue where saving any values via the admin page was impossible. The reason is the filter in the save controller added in this commit https://github.com/magento/magento2/commit/8089bd741cd3521943c1b33b6d80c74df8a49667 that strips out all fields not defined in a system.xml file.

In our use case (dynamically created shipping methods) it is impossible to create corresponding system.xml files. Fortunately we can bypass the new filter by making a plugin to the Magento/Config/Model/Config/Structure::getFieldPaths-method. This, however, seems a bit kludgy and non-semantic.

Examples

  1. Create a field in an adminhtml section dynamically.
  2. Try to save a value for the dynamic field.

Works in <= M2.3.1, but not in M2.3.2.

Proposed solution

I'm not 100% sure this even is an issue, since the workaround is fairly obvious. However, I feel there could be a more semantic way of being able to use dynamic fields in the adminhtml. Tricking the filter into believing there are system.xml paths for the config values isn't a great for code smell and readability perspective.

I am not sure what the addition of the filter was meant to fix as there is little info in the commit, but I assume a filter like that is necessary. As I don't think our use case is that outlandish for an extension developer, I think there could be a more obvious method added that can be pluginized into, or some other way of whitelisting fields for the filter. Thoughs?

m2-assistant[bot] commented 5 years ago

Hi @mattijv. Thank you for your report. To help us process this issue please make sure that you provided the following information:

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento give me 2.3-develop instance - upcoming 2.3.x release

For more details, please, review the Magento Contributor Assistant documentation.

@mattijv do you confirm that you were able to reproduce the issue on vanilla Magento instance following steps to reproduce?


mattijv commented 5 years ago

Since there seems to be no label for it, I want to stress that this is strictly a developer experience issue.

m2-assistant[bot] commented 5 years ago

Hi @engcom-Delta. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

engcom-Delta commented 5 years ago

Hi @mattijv thank you for your report. I'm not able to reproduce issue by steps you described on clean 2.3-develop. #23423Issue

#23423IssueDb

If you'd like to update issue, please reopen it.

mattijv commented 5 years ago

@engcom-Delta To clarify: by dynamic I don't mean the FieldArray input type. I guess "dynamic" was not a good term.

What I mean by "dynamically" is to use a plugin to add fields (of any input type) that are not defined in a system.xml file. For example by creating a plugin for the setData method of `app/code/Magento/Config/Model/Config/Structure/Element/AbstractComposite.php``that adds an additional field besides those defined in a system.xml file.

EDIT: I'm leaving this closed since it's a developer experience issue rather than a functional issue and it's more your decision if you wish this to be discussed more.