Document Select/list elements

I took a quick look at the initial patch. Some thoughts:

I think maybe this needs some additional text. Like before that #return_value line, maybe something saying "Here are some notes on the properties:" or something like that... if that line is even needed. Probably it's useful.

And before the @code section, something like "Usage example:"

Also that usage example doesn't use #type = 'checkbox' so ... somewhat suspicious ;)

Yeah. I think I'd avoid listing all the properties for now though, since we still need to figure out what to do to document them. Mostly they're all the same, like '#title" for instance, and we really really really don't want to have to list them all on each element. Very difficult and tedious to maintain, not to mention tedious to do in the first place. So that is why I suggest a heading here conveying "some special notes on properties" at least for the time being.

Yeah. Well, for now let's just make sure we have notes about the special properties, and you can just make a bullet list with a line before it something like:

Property notes:

Maybe we'll end up putting the documentation about the generic properties on the defgroup topic, or on the base class, but we definitely don't want it repeated on every element class.

Looks great, thanks! I think the things you've repeated make good sense.

A few small suggestions and formatting errors:

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkbox.php
   @@ -13,8 +13,18 @@
   + * Properties:
   + * #return_value is the value to return when the checkbox is checked.
   + *
   

   I think the Properties sections would be better formatted as bullet lists. See https://www.drupal.org/node/1354#lists

   Applies to all the classes in the patch.

2. +++ b/core/lib/Drupal/Core/Render/Element/Checkbox.php
   @@ -13,8 +13,18 @@
   + * @see \Drupal\Core\Render\Element\Checkboxes
     * @FormElement("checkbox")
     */
   

   Should be a blank line between these two lines.

3. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -12,10 +12,21 @@
   + * Property notes:
   

   Let's be consistent and either always call it Properties or always Property notes. Either is OK with me.

4. +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -10,9 +10,12 @@
    /**
   - * Provides a form element for a single radio button.
   + * Provides a form element for a single radio button.  This is an internal element
   + * that is primarily used by drupal in the radios control. Please refer to Radios element
   + * documentation on its use.
     *
   

   The first line of any doc block should be a one-line description ending in .

   Added docs should go in a new paragraph.

5. +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -10,9 +10,12 @@
   + * Provides a form element for a single radio button.  This is an internal element
   + * that is primarily used by drupal in the radios control. Please refer to Radios element
   + * documentation on its use.
     *
   

   Also these lines go over 80 characters. There are numerous places in the patch that do this, please check it over.

   And Drupal should be capitalized... although probably best to reword this so it doesn't actually say "Drupal" at all, like saying it's used in the Form API?

   What is a "radios control" anyway? Probably should stick to standard terminology and call it an "element"?

6. +++ b/core/lib/Drupal/Core/Render/Element/Radios.php
   @@ -13,8 +13,23 @@
   + * #options is an associative array, where the key is the returned value of the radios displayed and
   

   Over 80 characters.

7. +++ b/core/lib/Drupal/Core/Render/Element/Radios.php
   @@ -13,8 +13,23 @@
   + * the value is label next to each radio.
   

   Probably should say "radio button" at the end not just "radio"?

8. +++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
   @@ -14,9 +14,12 @@
   + * See https://www.drupal.org/node/94510 for a example and full explanation
   

   This should end in . Is it possible to put some explanation in here too, rather than referring to a drupal.org page that may not be available to someone who has the Drupal source code?

9. +++ b/core/lib/Drupal/Core/Render/Element/Weight.php
   @@ -15,6 +15,20 @@
   + * #delta is used to specify the range of possible weight values used. A delta
   + * of 10 would indicate possible weight values between
   + *
   

   This looks unfinished?

This is looking much better!

Probably one more iteration and we'll have it... A few suggestions:

1. +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -12,7 +12,12 @@
   + * This is an internal element that is primarily used
   + * to render radios form element. Please refer to Radios
   + * element documentation on its use.
   + *
   

   Much better here!

   In the second line:
   radios form element => elements

   Then... Don't say "Please". See https://www.drupal.org/node/604342 (search for the word "please" there) for some reasoning on this.

   And any class name needs a namespace.

   So we would like it to say something like:

   See \Drupal\Core\Render\Element\Radios documentation for more information.

2. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -13,8 +13,15 @@
   + * - #options is an associative array, where the key is the #return_value of the
   + *   select element and the value is label displayed.
   + *
   + * - #empty_option defines the label that will be displayed to denote no selection
   + *   in the selection element.
   + *
   

   No blank lines between list items.

3. In the lists, use a syntax like this for each element:

   + * - #options: An associative array, where the key is the #return_value of the ...
   

   Rather than making them into nice prose sentences like "#options is an associative...".

4. Still need to take out the added blank line here:

   + *
     */
    class LanguageSelect extends FormElement {
   

This all looks excellent to me now. Thanks very much for the patches!

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkbox.php
   @@ -13,6 +13,18 @@
   + * - #return_value: The value to return when the checkbox is
   + *   checked.
   
   +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -12,9 +12,21 @@
   + * - #options: An associative array, where the key is the #return_value of
   + *   the checkbox and the value is displayed. The #options array cannot have
   + *   a 0 key, as it would not be possible to discern checked and unchecked
   + *   states.
   
   +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -12,7 +12,13 @@
   + * This is an internal element that is primarily used
   + * to render the radios form element. Refer to
   + * \Drupal\Core\Render\Element\Radios
   + * for more documentation.
   
   +++ b/core/lib/Drupal/Core/Render/Element/Radios.php
   @@ -13,8 +13,23 @@
   + * - #options: An associative array, where the key is the returned value of
   + *   the radios displayed and the value is label next to each radio.
   
   +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -13,8 +13,13 @@
   + * - #empty_option: The label that will be displayed to denote no selection
   + *   in the selection element.
   
   +++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
   @@ -14,9 +14,14 @@
   + * - #options: An array of key value pairs where the key is the value
   + *   returned when the user select the radio button or checkbox, and
   + *   the value is the row of table data.
   

   Not using the full 80 characters width.

2. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -12,9 +12,21 @@
   + *   '#options' => drupal_map_assoc(array(t('SAT'), t('ACT'))),
   

   I think this example is problematic because using a t()'d string as the key means the value would change if the translated value is changed.

Hm, I gave this latest patch a review and found a few more things to fix... which either I missed in my last few reviews or things changed in this last patch?

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkbox.php
   @@ -13,6 +13,18 @@
   + * Properties:
   + * - #return_value: The value to return when the checkbox is
   + *   checked.
   + *
   

   This still doesn't go out to 80 characters.

2. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -13,8 +13,13 @@
   + * - #options: An associative array, where the key is the #return_value of the
   + *   select element and the value is label displayed.
   + * - #empty_option: The label that will be displayed to denote no selection in
   

   "the value is label displayed" ... hm, a bit awkward? Sorry didn't notice this in the last review.

3. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -13,8 +13,13 @@
   + * - #empty_option: The label that will be displayed to denote no selection in
   + *   the selection element.
   + * - #empty_value: The #return_value or #default_value of the select element
   

   What's a "selection element"?

4. +++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
   @@ -14,9 +14,14 @@
   + * - #headers: Table headers used in the selection table.
   

   What's a "selection table"?

5. +++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
   @@ -14,9 +14,14 @@
   + *   when the user select the radio button or checkbox, and the value is the row
   

   select -> selects

6. +++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
   @@ -14,9 +14,14 @@
   + * See https://www.drupal.org/node/94510 for a example and full explanation
   

   Needs to end in .

7. +++ b/core/lib/Drupal/Core/Render/Element/Weight.php
   @@ -15,6 +15,20 @@
   + * - #delta: The range of possible weight values used. A delta
   + *   of 10 would indicate possible weight values between 10 and -10.
   + *
   

   Still not up to 80 characters.

This is much better! I think we could still improve the wording and consistency of wording between the various #options descriptions:

1. +++ b/core/lib/Drupal/Core/Render/Element/Radios.php
   @@ -13,8 +13,23 @@
   + * - #options: An associative array, where the key is the returned value of the
   + *   radios displayed and the value is label next to each radio.
   + *
   

   This is still a bit difficult to read, I think...

   Suggestion:

   ... where the keys are the returned values for each radio button, and the values are the labels next to each radio button.

2. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -13,8 +13,12 @@
   + * - #options: An associative array, where the key is the #return_value of the
   

   Looking at this vs. Radios, I think maybe we shouldn't say #return_value here. We should make it like my suggestion for Radios above.

3. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -13,8 +13,12 @@
   + * - #empty_value: The #return_value or #default_value of the select element
   + *   that is used to denote no selection.
     *
   

   I guess I'm not sure what this means, and we shouldn't use #return_value or #default_value in this text. It isn't a #default_value for sure. It's just a value.

4. +++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
   @@ -14,9 +14,14 @@
   + * - #options: An array of key value pairs where the key is the value returned
   + *   when the user selects the radio button or checkbox, and the value is the
   + *   row of table data.
   + *
   

   This is better written than the others. ;) But it should probably say "associative array".

Addressed #25 changes.

When you make a new patch on an issue, it is normal/helpful to make an Interdiff file. Next time, please do so; don't bother now on this issue since I've reviewed the patch provided. Thanks for the new patch, anyway!

So this is getting better... still not quite there:

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -12,9 +12,20 @@
   + * - #options: An associative array, where the key is the #return_value of the
   + *   checkbox and the value is displayed. The #options array cannot have a 0
   + *   key, as it would not be possible to discern checked and unchecked states.
   + *
   

   Can we revise this similar to what was suggested in #25?

   Specifically I would suggest since this is a Checkboxes element:

   An associative array whose keys are the values returned for each checkbox, and whose values are the labels next to each checkbox.

2. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -13,8 +13,12 @@
   + * - #options: An associative array, where the keys are the retured values for
   + *   each select element, and the values are label next to each select element.
   + * - #empty_option: The label that will be displayed to denote no selection.
   

   Actually they aren't "select elements", they are options, and the labels are not "next to" the options, they are what's shown in the drop-down list.

   So this needs a rewrite.

   In the #empty_value property just below here, also don't call it "select element"

3. +++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
   @@ -14,9 +14,14 @@
   + * - #options: An associative array of key value pairs where the key is the
   

   take out "of key value pairs". That is what an associative array is, so it's redundant.

   Also it should be the "keys are" not "key is". Same for "values" not "value".

re-rolling the patch with the modification in #2486979: Document Select/list elements

Good, still a few things to fix:

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -12,9 +12,21 @@
   + *   checkbox, and whose values are the labels next to each checkbox.
   + *   The #options array cannot have a 0 key, as it would not be possible
   + *   to discern checked and unchecked states.
   + *
   

   This section still could be wrapped better -- should be closer to 80 character lines. I wouldn't have mentioned it at all but there is something else to fix in the patch anyway...

2. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -13,8 +13,13 @@
   + *   each options, and the values are the options to be shown in the drop-down
   

   each options -> each option

3. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -13,8 +13,13 @@
   + * - #empty_value: The value of the select element that is used to denote no
   

   select element -> option

4. +++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
   @@ -14,9 +14,14 @@
   + * - #options: An associative array where the key are the values returned when
   + *   user selects the radio button or checkbox, and the value is the row of
   + *   table data.
   + *
   

   key -> keys

   hm... Well given the rest of the sentence, you either need to make it keys are / values are or each key is / each value is. I don't care which, but make it consistent/parallel wording.

5. +++ b/core/lib/Drupal/Core/Render/Element/Weight.php
   @@ -12,8 +12,22 @@
   + *   indicate possible weight values between 10 and -10.
   

   As long as we're fixing things, maybe make this -10 and 10?

Almost!

1. +++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
   @@ -14,9 +14,14 @@
   + * - #options: An associative array where each key is the values returned when
   + *   a user selects the radio button or checkbox, and each value is the row of
   + *   table data.
   + *
   

   One typo: should be "each key is the value [no s] returned"

2. +++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
   @@ -14,9 +14,14 @@
   + * See https://www.drupal.org/node/94510 for a example and full explanation.
   

   Another typo: should be "an example".

   Whoops. Also I just checked this link and I think it should be https://www.drupal.org/node/945102 [2 missing at end of URL]

Thanks! But see comment #31 item 2 - patch still says "a example".

Great! I think it's all good now.

This looks like a really nice improvement. Committed 1403239 and pushed to 8.0.x. Thanks!