Two #config_targets error when used on a text_format form element

Created on 25 July 2024, about 1 month ago
Updated 30 August 2024, 9 days ago

Problem/Motivation

Trying to use the #config_target on text_format form elements in a configuration form results in a validation exception:

LogicException: Two #config_targets both target "akey.asubkey" in the "mymodule.settings" config: `$form['parent']['akey']['asubkey']` and `$form['parent']['akey']['asubkey']['value']`. in Drupal\Core\Form\ConfigFormBase->doStoreConfigMap() (line 183 of core/lib/Drupal/Core/Form/ConfigFormBase.php).

Steps to reproduce

  • Create a new configuration form extending \Drupal\Core\Form\ConfigFormBase
  • Add a nested text_format field with #config_target such as:

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

Proposed resolution

The issue with the #config_target on text_format form elements is addressed by updating the ConfigFormBase class. The solution involves adding special handling for text_format elements within the doStoreConfigMap() method. This fix ensures that text_format fields are correctly mapped for both .value and .format properties, avoiding conflicts where two #config_targets might point to the same configuration key.

Remaining tasks

  1. Review and validate the changes to ensure they resolve the issue without introducing new problems.
  2. Evaluate the added PHPUnit test case to ensure it is thorough and effective, and determine if any further improvements are needed.
🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Configuration 

Last updated 1 day ago

Live updates comments and jobs are added and updated live.
  • Needs subsystem maintainer review

    It is used to alert the maintainer(s) of a particular core subsystem that an issue significantly impacts their subsystem, and their signoff is needed (see the governance policy draft for more information). Also, if you use this tag, make sure the issue component is set to the correct subsystem. If an issue significantly impacts more than one subsystem, use needs framework manager review instead.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @matthieuscarset
  • 🇺🇸United States cilefen

    This is the working branch.

  • First commit to issue fork.
  • Pipeline finished with Success
    about 1 month ago
    Total: 848s
    #240200
  • 🇮🇳India VinmayiSwamy

    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:
    2. 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.

    3. copyFormValuesToConfig() Method:
    4. 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!

  • Status changed to Needs review about 1 month ago
  • Status changed to Needs work about 1 month ago
  • Pipeline finished with Failed
    about 1 month ago
    Total: 237s
    #247904
  • Status changed to Needs review about 1 month ago
  • 🇮🇳India VinmayiSwamy

    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!

  • Pipeline finished with Failed
    about 1 month ago
    Total: 245s
    #247917
  • Pipeline finished with Success
    about 1 month ago
    Total: 1230s
    #247927
  • Status changed to Needs work 25 days ago
  • 🇺🇸United States smustgrave

    Just noticed issue summary is incomplete.

  • 🇮🇳India VinmayiSwamy

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

  • Status changed to Needs review 25 days ago
  • Status changed to RTBC 24 days ago
  • 🇺🇸United States smustgrave

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

    Believe feedback has been addressed.

  • Status changed to Needs work 9 days ago
  • 🇬🇧United Kingdom catch

    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.

Production build 0.71.5 2024