Add property to identify if checkbox/radio is in group of checkboxes/radios

Doesn't apply to 9.1.x so needs reroll.

Re-rolled against 9.1.x. Kindly review a patch.

Thanks for the update.

1) Not sure this would be considered a bug but leaving for now.

2) This would need tests so tagging.

3) Not sure how this could be manually tested without custom code.

4) Looking at other '#is_' usage in core and see:

• #is_button
• #is_weight
• #is_multiple

Given that, assuming this approach might be accepted, it might be better to shorten the text though maybe "#is_group" is too ambiguous and/or hard to differentiate from #is_multiple.

Moving back to "Needs work" for tests.

Adding a few Kernel test cases to check if the field attribute is present or not.
I'm not quite sure about this approach. Feel free to suggest better approaches to implement test cases related to this.

Another one that I am thinking of:
- Create one form
- invoking one template_preprocess_hook
- Set the state variable inside template preprocess hook based on "#is_group_of_elements"
- Again assert the state variable and ensure that it is true.

Thanks for the tests! Noticed something trivial but otherwise it seems clear to me. Tests pass. Let me see if anyone in bugsmash has thoughts on the naming convention.

+++ b/core/tests/Drupal/KernelTests/Core/Element/FormElementPropertiesTest.php
@@ -0,0 +1,44 @@
+      '#title' => 'Test',

Extreme Nitpick: Could be 'Test Checkboxes' since other one is 'Test Radios'.

Small update in patch 20 just addressing comment #22 suggestion.

Thank you for the wording change. That looks good. I'll double check again to see if anyone in bugsmash has thoughts on the is_group_of_elements naming.

After discussing with the bugsmash team, moving this to a task. But, leaving the Bug Smash Initiative tag since we worked on this issue as part of the initiative.

I was asked by mohit_aghera for a review. This is likely the first time I have reviewed an issue about a form element.

Starting with the issue summary. That gave me an understanding of the problem so far so good.

Now reading the issue. In #7 the solution used here is questioned and help for a better solution was asked for. The following comments do not address that question. I don't know enough about forms to know if there is a better way to solve this. Can someone confirm?

Like Kristen Pol I think 'is_group_of_elements' a big long and prefer 'is_group'. Personally, I would not confuse that with 'is_multiple' another option could be 'is_group_member'. That makes more sense to me because an element is not a group of elements.

Now looking at the patch.

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -84,6 +84,10 @@ public static function processCheckboxes(&$element, FormStateInterface $form_sta
   +          '#is_group_of_elements' => TRUE,
   
   +++ b/core/lib/Drupal/Core/Render/Element/Radios.php
   @@ -81,6 +81,10 @@ public static function processRadios(&$element, FormStateInterface $form_state,
   +          '#is_group_of_elements' => TRUE,
   

   This is setting the default to be TRUE? How does one make it FALSE?

2. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -84,6 +84,10 @@ public static function processCheckboxes(&$element, FormStateInterface $form_sta
   +          // Indicate that this checkbox is part of a group of checkboxes.
   +          // Themers should be able to use this value in hook_preprocess_HOOK()
   +          // to apply conditional theming.
   
   +++ b/core/lib/Drupal/Core/Render/Element/Radios.php
   @@ -81,6 +81,10 @@ public static function processRadios(&$element, FormStateInterface $form_state,
   +          // Indicate that this radio is part of a group of radios.
   +          // Themers should be able to use this value in hook_preprocess_HOOK()
   +          // to apply conditional theming.
   

Explaining the purpose of the property should be in FormElement. Here, tell the user why it is set to TRUE.

The new property should be documented in \Drupal\Core\Render\Element\FormElement. Looking at the file it would be something like
"(bool) TRUE if this element is part of a group of checkboxes or radios. FALSE otherwise. This allows themers to to apply conditional theming." That is not meant to be copy/pasted. But it raises another question. Does it matter that this property is only applicable to Checkboxes and Radios?

Next I looked at the test. The test should also test the case when is_group_of_elements is FALSE as well.

And back to the IS. The proposed resolution is not the same as what is implemented, so it needs updating.

I hope that is useful.

I think I found a duplicate of this #1612596: Support css class per radio item. It is too late for me to read it thoroughly, so noting it here for now.

For Suggestion #1
I am not sure if we need to set it FALSE or not.
Reason is, we are setting is_group_of_elements to TRUE for checkboxes and radios element only.
Let me know your thoughts on this.

Suggestion #2
I've updated the comments, so current patches addresses that issue.
Feel free to review the wordings.

Added property in the FormElement class' comments. I've added another test case for single checkbox as well to ensure that property doesn't exist for single checkboxes.

Updating issue summary.

Having another look. Nice to see the additional test case.

1. +++ b/core/lib/Drupal/Core/Render/Element/FormElement.php
   @@ -80,6 +80,9 @@
   + * - #is_group_of_elements: (bool) It is true, if element is part of a group of
   

   A few lines above this is the descriptions for another boolean, #tree, let's use the same format (bool) TRUE if .... ; FALSE if the values should be flat..

2. +++ b/core/tests/Drupal/KernelTests/Core/Element/FormElementPropertiesTest.php
   @@ -0,0 +1,56 @@
   +      '#options' => ['SAT' => t('SAT'), 'ACT' => t('ACT')],
   

   The use of t() can be remove throughout the test. Tests should only use the t() function when specifically testing translations.

   I also think the test can be simplified by creating all the fields on one form and then testing that form.

Updated the IS to fix typos and simplify the proposed resolution.

Tried to address point 1 and 2 of comment #31.

Add related issue.

@mohit_aghera, thanks for the answers in #28. Thanks resolves my questions.
@ravi.shankar, thanks for the fixes for #31 1 and 2. I think you missed the paragraph about changing the test?

#26
paragraph 3. The question about the approach being used here needs to be resolved. Or maybe I just do not understand because I don't know much about forms.
paragraph 4. Both Kristen Pol and I think that the property name is too long. See #24 and #26
#31 The test can be simplified.

A few code comments.

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -84,6 +84,9 @@ public static function processCheckboxes(&$element, FormStateInterface $form_sta
   +          // Set to true, to indicate that this checkbox is part of a group of
   +          // checkboxes.
   

   See my comment about this comment in Radios.php

2. +++ b/core/lib/Drupal/Core/Render/Element/FormElement.php
   @@ -80,6 +80,9 @@
   + * - #is_group_of_elements: (bool) TRUE if element is part of a group of
   

   s/if element/if the element/

3. +++ b/core/lib/Drupal/Core/Render/Element/Radios.php
   @@ -81,6 +81,9 @@ public static function processRadios(&$element, FormStateInterface $form_state,
   +          // Set to true, to indicate that this checkbox is part of a group of
   +          // checkboxes.
   

   This comment is the Radios file so should not be referring to checkboxes. And now that I see this shorted version I think we can get it on one line. As a suggestion,
   "// Indicates this is part of a group." Change that if you like.

4. +++ b/core/tests/Drupal/KernelTests/Core/Element/FormElementPropertiesTest.php
   @@ -0,0 +1,56 @@
   +class FormElementPropertiesTest extends KernelTestBase {
   

   The title here suggests that this is testing all Form Element properties. It needs a new name. Maybe, RadioCheckboxGroupPropertyTest ? And the test can be simplified, there is no need to create two forms.

mohit_aghera asked for some clarification of the second paragraph in #35 in #bugsmash. Ah, I see that is poorly formatted as well. There are two parts. The first, in #7 hass said they were not sure that the approach taken in the patch was the 'right' one and talked about looking at #parent. And, as far as I can tell, that approach has been used in the following patches without question. So, is there a better way to do this? I don't know - I just want to make sure that someone has thought about it. And the second, is questing the name of the new property.

And that ping in #bugsmash was timely as well. I was wondering the other day about #26.2 and didn't manage to get back here to check. In that comment I said I thought the documentation for the new property should be in FormElement and now I think that is wrong. Other elements, such as Url and Weight define properties that are unique to those elements. I now think the documentation for the new element should be in Radios and Checkboxes. If you agree, sorry about the extra work.

Hope that helps.

Hit this issue this week #3276250: Gin theme checkbox styles breaks Webform custom composite fields required. Gin theme has some fancy CSS to avoid theming checkboxes incorrectly that would be greatly simplified by this fix getting into core. Thanks for the work that was done on this last year. I guess its not super clear what is being asked to change to take this forward.

I'm working on #35 and #36 I'll send a patch in a few moments.

Regarding #35
i - Done. Shortened property name to #is_group_member
1 & 3 - Done! Shortened comments to a single line.
2 - Done.
4 - Done. Renamed test to CheckboxRadioGroupPropertyTest and simplified test to use a single form. I also added one more test case for the single radio case.

Regarding #36
I moved the new property description to Checkbox.php and Radios.php

The fail seems unrelated, requeueing test.

Tested the patch from #40
All suggestions are rectified in the latest patch
Steps followed.
Added a custom form

$form['options'] = array(
    '#type' => 'checkboxes',
    '#title' => t('Various Options by Checkbox'),
    '#options' => array(
      'key1' => t('Option One'),
      'key2' => t('Option Two'),
      'key3' => t('Option Three'),
    ),
   );

$form['options'] = array(
    '#type' => 'checkboxes',
    '#title' => t('Various Options by Checkbox'),
    '#is_group_member' => FALSE,
    '#options' => array(
      'key1' => t('Option One'),
      'key2' => t('Option Two'),
      'key3' => t('Option Three'),
    ),
   );

Added custom class in preprocess hook using the variable is_group_member
Patch works correctly.

Unrelated test fail, re-queueing tests and restoring status

Unrelated test fail. Restoring status.

Unrelated test fail. Restoring status.

Tested the patch from #40
Against latest 9.5.x-dev
Patch cleanly apply and works correctly.

Unrelated test fail. Restoring status.