Allow configuration paths based on system.xml #31621

navarr · 2021-01-12T14:34:52Z

Description (*)

When merged, this code will update the path validator to consider system.xml paths valid (in addition to config.xml paths)

Fixed Issues (if relevant)

Fixes bin/magento config:set expects config path to match display path #27678

Manual testing scenarios (*)

Create sample module with system.xml like this

    <system>
        <section id="sales">
            <group id="general">
                <field id="keyid" type="text" sortOrder="100" showInDefault="1" showInWebsite="1" showInStore="0">
                    <label>Key Id</label>
                </field>
            </group>
        </section>
    </system>

Set some value via CLI
bin/magento config:set sales/general/keyid 1
We can see Value was saved. as expected.
Add some config_path to system.xml like this

    <system>
        <section id="sales">
            <group id="general">
                <field id="keyid" type="text" sortOrder="100" showInDefault="1" showInWebsite="1" showInStore="0">
                    <label>Key Id</label>
                    <config_path>custom_path/keyid</config_path>
                </field>
            </group>
        </section>
    </system>

Clear cache config
Try to set some value via CLI again
bin/magento config:set sales/general/keyid 777

AR:

The "sales/general/keyid" path doesn't exist. Verify and try again.

ER:

Value was saved.

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 are green)

Resolves magento#27678

m2-assistant · 2021-01-12T14:34:54Z

Hi @navarr. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests
Semantic Version Checker

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

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

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

ihor-sviziev

Hi @navarr,
Thank you for your contribution! Your change looks good to me.
Could you cover your change with some automated tests?

ihor-sviziev · 2021-01-13T09:46:26Z

@magento run all tests

engcom-Hotel · 2021-01-13T12:06:37Z

@magento run Functional Tests CE, Functional Tests EE

engcom-Hotel · 2021-01-13T16:36:10Z

@magento run all tests

engcom-Hotel · 2021-01-14T06:47:08Z

@magento run Functional Tests B2B, Functional Tests EE

ihor-sviziev

Please check my comments and also please add test coverage for new cases.

ihor-sviziev · 2021-01-14T07:03:13Z

app/code/Magento/Config/Model/Config/PathValidator.php

@@ -40,6 +40,11 @@ public function __construct(Structure $structure)
     */
    public function validate($path)
    {
+        $element = $this->structure->getElementByConfigPath($path);
+        if ($element && method_exists($element, 'getConfigPath') && $element->getConfigPath()!==null) {


Instead of checking that method exists, could we check here that the element is an instance of some class, or maybe not an instance of an element group?

magento2/app/code/Magento/Config/Block/System/Config/Form.php

Lines 301 to 304 in fbfac3e

if ($element instanceof \Magento\Config\Model\Config\Structure\Element\Group) {

$this->_initGroup($element, $section, $fieldset);

} else {

$path = $element->getConfigPath() ?: $element->getPath($fieldPrefix);

I believe if the $element->getConfigPath() will return empty string that's not also good option. I think we should just check if(... && $element->getConfigPath()) {. What do you think about it?

Has been refactored.

ihor-sviziev · 2021-01-14T07:05:35Z

app/code/Magento/Config/Model/Config/Structure.php

@@ -191,7 +191,7 @@ public function getElementByConfigPath($path)
    {
        $allPaths = $this->getFieldPaths();

-        if (isset($allPaths[$path])) {
+        if (isset($allPaths[$path]) && is_array($allPaths[$path])) {


Could you explain why it wasn't needed earlier?

We didn't call it with empty $allPaths before and with the changes at the 'validate' method we can have an error at $path = array_shift($allPaths[$path]) because '$allPaths[$path]' could be 0 (int) instead of array.

engcom-Hotel · 2021-01-14T15:02:10Z

@magento run all tests

engcom-Hotel · 2021-01-15T09:54:06Z

@magento run Functional Tests B2B, Functional Tests CE

magento-engcom-team · 2021-01-25T14:17:42Z

Hi @ihor-sviziev, thank you for the review.
ENGCOM-8701 has been created to process this Pull Request

engcom-Alfa · 2021-01-26T09:53:48Z

✔️ QA Passed

Manual testing scenario:

Create sample module with system.xml like this

    <system>
        <section id="sales">
            <group id="general">
                <field id="keyid" type="text" sortOrder="100" showInDefault="1" showInWebsite="1" showInStore="0">
                    <label>Key Id</label>
                </field>
            </group>
        </section>
    </system>

Set some value via CLI
bin/magento config:set sales/general/keyid 1
We can see Value was saved. as expected.
Add some config_path to system.xml like this

    <system>
        <section id="sales">
            <group id="general">
                <field id="keyid" type="text" sortOrder="100" showInDefault="1" showInWebsite="1" showInStore="0">
                    <label>Key Id</label>
                    <config_path>custom_path/keyid</config_path>
                </field>
            </group>
        </section>
    </system>

Clear cache config
Try to set some value via CLI again
_bin/magento config:set sales/general/keyid 777;

Before: ✖️ The "sales/general/keyid" path doesn't exist. Verify and try again.

After: ✔️ Value was saved.

DB -> core_config_data

and setting configuration values work as expected.

m2-assistant · 2021-01-30T17:33:30Z

Hi @navarr, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Allow configuration paths based on system.xml

c7b5a73

Resolves magento#27678

magento-engcom-team added Component: Config Release Line: 2.4 Partner: Blue Acorn iCi partners-contribution Pull Request is created by Magento Partner tech-partners-contribution Technology Partner: Vertex labels Jan 12, 2021

m2-community-project bot added Progress: pending review Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. labels Jan 12, 2021

aleron75 removed the Release Line: 2.4 label Jan 12, 2021

m2-community-project bot assigned ihor-sviziev Jan 13, 2021

ihor-sviziev requested changes Jan 13, 2021

View reviewed changes

m2-community-project bot added Progress: needs update and removed Progress: pending review labels Jan 13, 2021

ihor-sviziev added Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests Award: bug fix labels Jan 13, 2021

engcom-Hotel self-assigned this Jan 13, 2021

ihor-sviziev requested changes Jan 14, 2021

View reviewed changes

m2-community-project bot added Progress: review and removed Progress: needs update labels Jan 14, 2021

ihor-sviziev added the Progress: needs update label Jan 15, 2021

m2-community-project bot removed the Progress: review label Jan 15, 2021

m2-community-project bot added the Progress: pending review label Jan 25, 2021

ihor-sviziev approved these changes Jan 25, 2021

View reviewed changes

m2-community-project bot added Progress: ready for testing and removed Progress: pending review labels Jan 25, 2021

magento-engcom-team added the Release Line: 2.4 label Jan 25, 2021

engcom-Alfa self-assigned this Jan 25, 2021

m2-community-project bot added Progress: testing in progress Progress: ready for testing and removed Progress: ready for testing Progress: testing in progress labels Jan 26, 2021

engcom-Alfa added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label Jan 26, 2021

m2-community-project bot added Progress: ready for testing Progress: testing in progress Progress: extended testing Progress: accept and removed Progress: testing in progress Progress: ready for testing Progress: extended testing labels Jan 26, 2021

magento-engcom-team pushed a commit that referenced this pull request Jan 30, 2021

ENGCOM-8701: Allow configuration paths based on system.xml #31621

d8e9787

magento-engcom-team merged commit 41d96c0 into magento:2.4-develop Jan 30, 2021

gabrieldagama added Type: Feature/Improvement Type: Bug Fix and removed Type: Feature/Improvement labels Mar 2, 2021

navarr deleted the patch-2 branch September 5, 2021 18:23

	if ($element instanceof \Magento\Config\Model\Config\Structure\Element\Group) {
	$this->_initGroup($element, $section, $fieldset);
	} else {
	$path = $element->getConfigPath() ?: $element->getPath($fieldPrefix);

Allow configuration paths based on system.xml #31621

Allow configuration paths based on system.xml #31621

Uh oh!

Conversation

navarr commented Jan 12, 2021 • edited by engcom-Charlie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

AR:

ER:

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Jan 12, 2021

Uh oh!

ihor-sviziev left a comment

Choose a reason for hiding this comment

Uh oh!

ihor-sviziev commented Jan 13, 2021

Uh oh!

engcom-Hotel commented Jan 13, 2021

Uh oh!

engcom-Hotel commented Jan 13, 2021

Uh oh!

engcom-Hotel commented Jan 14, 2021

Uh oh!

ihor-sviziev left a comment

Choose a reason for hiding this comment

Uh oh!

ihor-sviziev Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

engcom-Hotel Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

ihor-sviziev Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

engcom-Hotel Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

engcom-Hotel commented Jan 14, 2021

Uh oh!

engcom-Hotel commented Jan 15, 2021

Uh oh!

magento-engcom-team commented Jan 25, 2021

Uh oh!

engcom-Alfa commented Jan 26, 2021

Uh oh!

m2-assistant bot commented Jan 30, 2021

Uh oh!

Uh oh!

navarr commented Jan 12, 2021 •

edited by engcom-Charlie

Loading