- Issue created by @matthieuscarset
- First commit to issue fork.
- Merge request !9014Issue:3463868 - Two #config_targets error when used on a text_format form element → (Open) created by VinmayiSwamy
- 🇮🇳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 fortext_format
elements within thedoStoreConfigMap()
method.
This ensures thattext_format
elements are properly mapped to their value and format properties by updating the$map
array with these keys.Code Modifications:
doStoreConfigMap()
Method:copyFormValuesToConfig()
Method:
Checked if the element type is
text_format
.
If true, the#config_target
fortext_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.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 fortext_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 7:25am 1 August 2024 - Status changed to Needs work
about 1 month ago 1:03pm 1 August 2024 - Status changed to Needs review
about 1 month ago 2:06pm 8 August 2024 - 🇮🇳India VinmayiSwamy
Hi, I have added a PHPUnit test case to verify the changes made to the
doStoreConfigMap
method in theConfigFormBase
class. This test is designed to check how the method handlestext_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 thedoStoreConfigMap
method interacts with it as expected. SincedoStoreConfigMap
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 protecteddoStoreConfigMap
method on a mocked instance ofConfigFormBase
. The sample form element defined includestext_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 fortext_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!
- Status changed to Needs work
25 days ago 3:23pm 13 August 2024 - 🇮🇳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 4:22am 14 August 2024 - Status changed to RTBC
24 days ago 6:07pm 14 August 2024 - 🇺🇸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 7:13am 30 August 2024 - 🇬🇧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.