Document how optgroups are generated in form_select_options()

I start with this

Uploading patch.

Thanks for the patches! A few things to fix:

a)

+ * This function calls itself recursively to obtain the values for a optgroup.

Should be "an optgroup"... but... maybe it should be "each optgroup within the list of options"?

b)

+ *   - '#multiple': Optional boolean indicating if the user may select more than

Boolean should be capitalized. Also, there is a specific list syntax for optional items: https://www.drupal.org/node/1354#lists -- and in lists like this, we normally don't put the element keys inside ''.

c)

+ *   - '#options': An array of options to render as HTML.
+ *     A string or integer key mapped to a translated string is interpreted as a

This second line should be wrapped in the same paragraph as the first. Later down in this section, there's another wrapping problem. Wrap it all as one paragraph; do not have a line break after each sentence in the paragraph.

d)

+ *     A translated string mapped to an array indicates a group of options. The
+ *     array should contain the options you wish to group. Groups are output as
+ *     HTML optgroup elements.

This doesn't explicitly say that the string is used as the label attribute of the group... not sure if that is too obvious to mention? Seems like a good idea to mention?

e)

+ *   - '#value': Optional integer, string or array representing which option(s)

Needs serial comma before "or".

f)

+ * @param array|object|null $choices
+ *   Either an associative array of options, an object with an 'option' member
+ *   that is an associative array, or NULL. This parameter is only used
+ *   internally and should not be passed.

Should say (optional) at the beginning of the description here. And at the end "passed" ==> "passed in". I think it should also say that $choices should be the same format as $element['#options'], right?

Hmmm....

So looking at the code for this function, it looks to me as though $choices is initially set to $element['#options'], the first time through (assuming $choices isn't passed in explicitly, which it shouldn't be). Then for each recursive call, it goes through the $choices array

foreach ($choices as $key => $choice) {
 

1. If $choice is an array (indicating an opt group), then $choice is passed in as $choices in the recursive call.
2. If $choice is an object with an ->option property, then it calls form_select_options($element, $option->option)
3. Otherwise, $choice should be a string, and it makes an option with this as the label.

So. Given that, it seems like we should document that:
- $element['#options'] has to be an key/value array
- The array values can either be strings, arrays, or objects with ->option properties
- If the array value is itself an array, each element in the array has to have the same format as $element[#options]
- if the array value is an object, then the ->option property has to have the same format as $element[#options]
- $choices has to have the same format as $element[#options], but should be omitted in the initial call

Note: I do not think $choices can be an object. It has to be an array or omitted/NULL.

g) It looks like the eventual optgroup labels, select keys, and select labels, are passed through String::checkPlain(). This should be noted, because it's important to avoid double-sanitization.

This recent SA https://www.drupal.org/SA-CORE-2014-003 suggests that we should specify that the group labels, if they are translated, should NOT use placeholders that sanitize data. Doing so will lead to double-escaping.

EDIT: the 7.29 release notes confirm this: group labels should not be sanitized, as core takes care of this.

Looking good, but a few things to clear up and clarify:

1. +++ b/core/includes/form.inc
   @@ -849,17 +849,43 @@ function template_preprocess_select(&$variables) {
   + *   - #options: A associative array of options to render as HTML. Each array
   

   A*n* associative

2. +++ b/core/includes/form.inc
   @@ -849,17 +849,43 @@ function template_preprocess_select(&$variables) {
   + *     value can be a string, array, or an object with an 'option' property:
   

   *an* array

3. +++ b/core/includes/form.inc
   @@ -849,17 +849,43 @@ function template_preprocess_select(&$variables) {
   + *     - A string or integer key mapped to a translated string is interpreted as
   

   by 'mapped to', do you mean that the value is a translated string? That's not terminology I've seen used for PHP associative arrays.

   How about: 'A [...] key whose value is [...]'

   Also, 'string or integer' is redundant surely. Array keys can only be strings or integers. And the option type that allows an optgroup... a translated string is still just a string. It could even be a number!

4. +++ b/core/includes/form.inc
   @@ -849,17 +849,43 @@ function template_preprocess_select(&$variables) {
   + *     a single HTML option element. Do not use placeholders to sanitize data:
   

   'which', not 'to'. The ! placeholder is acceptable and may be needed.

5. +++ b/core/includes/form.inc
   @@ -849,17 +849,43 @@ function template_preprocess_select(&$variables) {
   + *     sensitive information in it and sanitize it before use.
   

   I'd say 'sanitize it if based on user input'. Most of the time it's not -- it's things like node ids, entity type names, etc, which are defined in code.

6. +++ b/core/includes/form.inc
   @@ -849,17 +849,43 @@ function template_preprocess_select(&$variables) {
   + *     double-escaping. The array should contain the options you wish to group.
   

   Should say, just to be totally clear, that the array itself follows the syntax of #options. Or does it? I presume you can't nest groups, so it can only be the plain type?

7. +++ b/core/includes/form.inc
   @@ -849,17 +849,43 @@ function template_preprocess_select(&$variables) {
   + *     - If the function encounters a string or integer key mapped to an object
   

   We don't need to say 'the function encounters'. It's unnecessarily wordy. Also, it doesn't follow the pattern of the list items above it.

8. +++ b/core/includes/form.inc
   @@ -849,17 +849,43 @@ function template_preprocess_select(&$variables) {
   + *     with an 'option' property, the key is ignored, the contents of the
   

   If the key is ignored, what ends up being the key for the option? What happens to other items that are in #options? Is that just something you shouldn't do? ie, if you use the object version, that should be the only item in #options?

9. +++ b/core/includes/form.inc
   @@ -849,17 +849,43 @@ function template_preprocess_select(&$variables) {
   + *   - #value: Optional integer, string, or array representing which option(s)
   + *     to pre-select when the list is first displayed. The integer or string
   + *     must match the key of an option in the '#options' list. If '#multiple' is
   + *     TRUE, this can be an array of integers or strings.
   

   Is that right? Isn't it #default_value? IIRC you get #value only when the form is being validated or something like that.

   Also, #value and #dv are both standard across FormAPI elements, so do they need repeating here?

> and Drupal doesn't enforce that the value of a select box match the set of options it sent

Really? I thought that was one of the major points of FormAPI -- that you can be sure that form values are from the options in the form.

So about the object malarkey -- have I understood this correctly:

$object->option = array(
  'bar' => t('Bar'),
  'biz' => t(Biz'),
);

$element['#options'] = array(
  'foo' => t('Foo'),
  'ignored' => $object,
);

will give me 3 options, at the same level, which are foo, bar, biz?

(BTW, I've never seen this object syntax for options in practice; I wonder what its use case is!)

Weird!
Also, I can't find a single use of it in core -- at least, "ack '\->option\b'" only comes up with tests. I'm betting this is some weird archaeological remain from something like D5!

EDIT: Bingo! https://www.drupal.org/node/24023#comment-329942

Git blame is a wonderful thing :)

Also: https://www.drupal.org/node/24023#comment-329940 HAHAHAHA.

Sorry, this review got lost in the shuffle...

So, I took a look at the preceding comments, and the text in the patch. I think the text adequately explains this weirdness.

A few minor problems in the patch to clear up:

a) List formatting -- in the sub-lists:

+ *     - A string or integer key whose value is a translated string is
+ *     interpreted as a single HTML option element. Do not use placeholders
...

The subsequent lines here need to be indented 2 more spaces. This is true for all of the lists at this level.

b)

+ *     interpreted as a single HTML option element. Do not use placeholders
+ *     which sanitize data: doing so will lead to double-escaping. Note that the

which -> that

c)

+ *     which sanitize data: doing so will lead to double-escaping. Note that the
+ *     key will be visible in the HTML and could be modified by malicious users,
+ *     so don't put sensitive information in it and sanitize it before use if
+ *     based on user input.

Um... I'm not sure what this last sentence is trying to say. The previous sentence is saying something about not sanitizing, and then here you are saying it needs to be sanitized... probably that last bit about "sanitize it before use" could just be left out (isn't that just generic "how to program in PHP with user input" advice? Doesn't belong here).

d)

+ *     and should follow the syntax of $element['#options'].
+ *     Groups are output as HTML optgroup elements.

This second line should be wrapped with the previous line... or omitted, isn't it redundant?

e)

+ * @param array|null $choices
+ *   (optional) Either an associative array of options in the same format as
+ *   $element['options'] above, or NULL. This parameter is only used internally

Shouldn't that be $element['#options']?

Please review revised patch with fixes in #18.

Thanks!

One more list formatting update:

+ *   - #value: Optional integer, string, or array representing which option(s)
+ *       to pre-select when the list is first displayed. The integer or string
+ *       must match the key of an option in the '#options' list. If '#multiple'
+ *       is TRUE, this can be an array of integers or strings.

The text in the 2nd, 3rd, ... lines should line up under # -- it's a bit to the right of where it should be.

Otherwise, looks perfect!

Please review updated patch with #20 changes.

Thanks! Sorry for the delay in reviewing; I was away...

Thanks all! Committed to 8.0.x.

Oh, looks like we should probably backport this to 7. Does it work the same there?

Please review attached patch for D7 as it appears to be same because the same element exist.

Agreed, D7 appears to be exactly the same. Thanks for the patch!

Thanks again everyone! Committed to 7.x.