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' => new ConfigTarget('mymodule.settings', 'akey.asubkey', fromConfig: fn (?array $value = NULL): string => $value['value'] ?? ''),
];

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

I pushed a test to https://git.drupalcode.org/project/drupal/-/merge_requests/10182/commits but it is not showing up in Gitlab? Attached 3463868-21-test-only.patch contains the code

Oh, the change showed up on gitlab

Cleaned up the old MRs so we should only have 1 with the test coverage https://git.drupalcode.org/issue/drupal-3463868/-/jobs/7129797

Restoring previous RTBC

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixed the merge conflict in MR 10182 after #3555535: Since symfony/validator 7.4: Support for passing the choices as the first argument to Symfony\Component\Validator\Constraints\Choice is deprecated. was committed

Updated the Proposed resolution to match the current implementation

This still needs blessing from the subsystem maintainer, but I think this is looking real good. Currently TextFormat is the only element that has a `$keys_not_to_copy`, but if other would be doing the same, I think this solution would work for them as well.

This doesn't actually set the default value of the text field correctly...

@alexpott can you elaborate what "this" refers to in #32 and what is incorrect about it?

I haven't manually tested this recently, but if this doesn't work we should also fix the test that is trying to test the behavior as well.

The default value for the field is not set... so although the value is saved it is not displayed back to the user. This is because of how \Drupal\filter\Element\TextFormat::processFormat() works. I can't quite work out how to fix this yet.

I just pushed a test that shows the problem.

The URL under test returns a cache hit, as the form has no cache dependency on the config being edited.

I have added a cache tag specifically to the form being tested, but I can see the cache tag being added in ConfigFormBase instead. Perhaps in a follow-up?

@idebr I think that the cache tag should be added in ConfigFormBase instead. Great find. Definitely something we should do.

We still to work out how the format can be saved and the default value should work.

Addressed #38 and #39

Some new test failures on related functional and unit tests, see https://git.drupalcode.org/project/drupal/-/pipelines/778184

Fixed the tests.

Thanks! The additional changes look good to me

The current diff from MR doesn't work for Drupal 11.3.8. Attaching a patch that works (without the tests)

This looks great now. Issue went through some twists and turns but I first read the MR, then went back through the issue history to figure out how we ended up where we did and can't think of anything else.

Committed/pushed to main and cherry-picked to 11.x and 11.3.x thanks!

This is causing test failures on 11.3.x: https://git.drupalcode.org/project/drupal/-/jobs/9744692
Since #3566768: Change \Drupal\system\Form\CronForm to use ConfigFormBase and use #config_target wasn't backported to 11.3.x, the CronForm doesn't extend ConfigFormBase, so the config:system.cron cache tag doesn't get added.

MR against 11.3.x to remove the tags from the tests: https://git.drupalcode.org/project/drupal/-/merge_requests/15716

Committed a50bc84 and pushed to 11.3.x. Thanks!

Thanks @godotislate - follow-up committed.