Two #config_targets error when used on a text_format form element

This is the working branch.

Hi, to resolve this issue, the following modifications were made to the ConfigFormBase class:

Enhanced Handling of text_format Elements:
Added a special case handling for text_format elements within the doStoreConfigMap() method.
This ensures that text_format elements are properly mapped to their value and format properties by updating the $map array with these keys.

Code Modifications:

1. doStoreConfigMap() Method:

Checked if the element type is text_format.
If true, the #config_target for text_format fields is now mapped separately for both .value and .format properties. This prevents the conflict of having two #config_targets pointing to the same configuration key.
Added logic to handle #array_parents as arrays and applied proper mapping to avoid duplication issues.

2. copyFormValuesToConfig() Method:

Updated to ensure that $array_parents is always treated as an array when retrieving form elements.
Added logic to skip the iteration if the parent path is not found and to ensure that elements have the #config_target property set before processing.

The modifications ensure that text_format elements are handled correctly by addressing the issue of duplicate configuration targets. By implementing specific logic for text_format fields and updating the configuration mapping process, the fix prevents validation exceptions and maintains consistency across the configuration form.

Please review the changes to confirm that they address the issue effectively. Your feedback and suggestions for further improvements are greatly appreciated.

Thanks!

Hi, I have added a PHPUnit test case to verify the changes made to the doStoreConfigMap method in the ConfigFormBase class. This test is designed to check how the method handles text_format elements with #config_target properties and ensures that the configuration mappings are updated correctly.

In this test, I created a mock of FormStateInterface to simulate the form state and validate that the doStoreConfigMap method interacts with it as expected. Since doStoreConfigMap is a protected method, I used reflection to access and invoke it.

The test involves setting up a mock for FormStateInterface and specifying an expectation for the set method to be called with the key 'config_targets' and a particular map. I then used reflection to invoke the protected doStoreConfigMap method on a mocked instance of ConfigFormBase. The sample form element defined includes text_format and #config_target attributes, as well as #array_parents for validation.

The focus of the test is to ensure that the config_targets map contains the correct entries and values for text_format elements, verifying that the #config_target properties are processed appropriately.

Could anyone please advise if a functional test is also required in addition to this unit test? While this unit test verifies the method in isolation, a functional test might be needed to assess its behaviour within the broader application context.

As this is my first attempt at writing test cases, I would really appreciate any feedback on whether this test is thorough and meets all necessary criteria. If there are additional scenarios to consider or best practices to follow, your guidance would be very helpful.

Please review the test case and let me know if any further steps are needed or if improvements should be made.

Thanks!

Just noticed issue summary is incomplete.

Hi, I have updated the issue summary. Please review the changes and let me know if any further changes are required. Thank you!

https://git.drupalcode.org/issue/drupal-3463868/-/jobs/2381869 shows the test coverage so removing that tag.

Believe feedback has been addressed.

I think we should look at whether this can be done without special-casing text_format in the config system - the exception is designed to prevent broken usage, and if the usage isn't broken then we should be able to account for that. Other config could have a similar structure.

MR 10182 implements an alternative solution where the code ends up as follows:

        $form['parent']['akey']['asubkey'] = [
          '#type' => 'text_format',
          '#title' => $this->t('Subkey title'),
          '#format' => $this->config('mymodule.settings')->get('akey.asubkey.format'),
          '#config_target' => "mymodule.settings:akey.asubkey",
        ];
  

I'm not sure ConfigTarget was intended to control any element property other than #default_value when reading the change record at https://www.drupal.org/node/3373502 →

MR 10182, resolves the issue, and seems like the least noisy solution and most future safe.

That looks great but could we add some test coverage?

Yea the new MR dropped the test coverage from the original MR. One should also be close.d