[Config] Giving the possibility to have a config dependency based on empty config value

[eduard13]

Description (*)

This PR provides a way to set a dependency to a field based on the empty value.

For example:
You have 2 input config fields, and you want to hide the 2nd one if the 1st one won't be empty.

Our system.xml will look like:

<field id="first_input" translate="label" type="text" sortOrder="20" showInDefault="1" showInWebsite="1" showInStore="1">
    <label>First Input</label>
</field>
<field id="second_input" translate="label" type="text" sortOrder="30" showInDefault="1" showInWebsite="1" showInStore="1">
    <label>Second input</label>
    <comment><![CDATA[Dependent by 1st field]]></comment>
    <depends>
        <field id="*/*/first_input"/>
    </depends>
</field>

Fixed Issues (if relevant)

N/A

Manual testing scenarios (*)

1. Add to any system config group the provided sample code
2. Open it in the admin panel
3. Type something in the 1st input
4. The second should be hidden

Questions or comments

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)

[m2-assistant]

Hi @eduard13. 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.3-develop instance - deploy vanilla Magento instance

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

[eduard13]

Based on the Backward Compatibility Policy, we shouldn't change classes marked as @api, but what do you @paliarush think about having a such feature?
Thank you.

[dmytro-ch]

Hi @eduard13,
great job, thank you!

Based on the Backward Compatibility Policy, we shouldn't change classes marked as @api

No, it's not relevant unless we change the signature of public/protected methods or introduce new properties or methods, etc.

The only thing, I guess, the behavior you mentioned in description should apear in case the negative="1" attribute is set.
e.g. with the following configuration:

<depends>
    <field id="*/*/first_input" negative="1"/>
</depends>

the second field should be shown if the first one is empty, and vice versa.
Feel free to contact me for further discussion if necessary, or correct me if I'm wrong.

Could you please also implement the recommendations from the static test report?

Thank you!

[eduard13]

Hi @dmytro-ch, thank you for your review. Actually not sure what negative does, but we can't have an empty value for our dependency. We'll get Notice: Undefined index: value in /home/dev/sites/mage23/app/code/Magento/Config/Model/Config/Structure/Element/Dependency/Field.php on line 44.
Basically the provided change fixes this warning and gives us the possibility to set a dependency on empty value.
Please let me know your thoughts on this matter.

Thank you.

[dmytro-ch]

After taking a closer look at the issue and having an additional discussion, we decided to proceed with the current behaviour to keep the logic of \Magento\Config\Model\Config\Structure\Element\Dependency\Field as straightforward as possible.

[magento-engcom-team]

Hi @dmytro-ch, thank you for the review.
ENGCOM-6390 has been created to process this Pull Request

[engcom-Alfa]

✔️ QA Passed

Before:
[2019-12-04 10:05:40] main.CRITICAL: Notice: Undefined index: value in /home/arthur/sites/magento/magento23/app/code/Magento/Config/Model/Config/Structure/Element/Dependency/Field.php on line 44 [] []

After:

[m2-assistant]

Hi @eduard13, 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.