Select form element has undocumented properties

Please review.

+++ b/core/lib/Drupal/Core/Render/Element/Select.php
@@ -16,6 +16,9 @@
+ * - #multiple: A Boolean indicating whether one or more options can be

Does "Boolean" require capitalization? I'd recommend "boolean."

I agree. Patch and interdiff attached.

Hi @zerbash, I reviewed the patch and it is OK. Thank you for changing the capital letter.
But I was wondering why you changed also the other line in your commit. Could you explain it?

from:
* - #size: The size of the select box in characters.

to:
* - #size: The width of the textfield (in characters).

I copied the first half of the definition for the #size attribute for the select element from the D7 Form API Reference page. However, in confirming to see if that was accurate, I rendered this element out of the context of a form: Setting #size to an integer greater than one changes the select to a multiselect and ADDS that number of SPACES to the end of the options list. So maybe there's another issue?

In the context of a form:

• Size refers to the number of LINES displaced in a select box.
• If #multiple is set to TRUE, #size automatically is set to a minimum of 4 (unless a greater integer is specified).
• If #size is set to an integer greater than one, #multiple becomes TRUE.

Out of the context of a form:

• If #size is set to an integer greater than one, #multiple becomes TRUE.
• Setting #multiple on its own has no discernible effect.

The documentation of #multiple and #size is appropriate in latest patch. Hence changing the status of this issue to Needs Review.

Thanks!

This still needs some work though...

1. #4 is wrong. Boolean, when used in text, is capitalized. bool, when used as a data type, is not capitalized. Check your favorite English dictionary...

2. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -16,6 +16,9 @@
   + * - #multiple: A boolean indicating whether one or more options can be
   + *   selected.
   

   Um, you can always select 1 option, right? So this description seems confusing to me.

3. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -16,6 +16,9 @@
   + * - #size: The width of the textfield (in characters).
   

   It is not a text field, so can this also be updated?

Given it a try.

Thanks! Looking better, but still needs some work:

1. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -16,6 +16,9 @@
   + * - #multiple: A Boolean indicating either single or multiple values can be
   + *   selected.
   

   So it's fairly clear from the name #multiple that TRUE means multiple items can be selected and FALSE means only 1 item can be selected... but why not just say that, instead of this kind of hard-to-parse description?

2. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -16,6 +16,9 @@
   + * - #size: The width of the field (in characters).
   

   Is that really what #size is for a select element? If this gets put into the size attribute for the select HTML element (and I think it does), I think it is actually the number of elements displayed? Someone needs to check on this.

Yes jhodgdon, size specifies the number of elements displayed. I have tried to update it.

+++ b/core/lib/Drupal/Core/Render/Element/Select.php
@@ -16,6 +16,9 @@
+ * - #multiple: Indicates whether one or more options can be selected. Defaults
+ *   to FALSE.

Can you please just say "TRUE if ...; FALSE if ..." because again, saying "Indicates whether one or more options can be selected" is not clear on what TRUE means and what FALSE means. Thanks!

Thank you very much Jennifer. I have updated the patch.

Thanks, better! But:

1. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -16,6 +16,9 @@
   + * - #multiple: FALSE if one option is selected and TRUE if more than one
   + *   options are selected.
   

   This is still not correct. FALSE means only one option *can be selected*, not that only one *is selected*. Similarly for TRUE.

2. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -16,6 +16,9 @@
   + * - #size: Specifies the number of visible options in a select list.
   

   And here... we can omit "in a select list". This is all related to a select list.

Thanks @jhodgdon.

Worked on your suggestions. Please review.

Done.

Two patches -- @snehi, this issue was assigned, best to leave it to the person it was assigned to, thanks!

That aside, I like the wording in the patch in #22 slightly better -- I think the word "only" is helpful.

But two small things to fix:

1. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -16,6 +16,9 @@
   + * - #multiple: FALSE if only one option can be selected and TRUE if more than one
   

   This line goes over 80 characters.

2. +++ b/core/lib/Drupal/Core/Render/Element/Select.php
   @@ -16,6 +16,9 @@
   + *   options can be selected.
   

   options -> option

@Jhodgdon Sorry for that. But when i was uploading the patch the uploading time was same for the both patches may be a lag of few seconds.

Hi Jennifer, I have updated the patch.

Looks fine, thanks!

Thanks everyone. I checked and confirmed that Select directly extends FormElement, which does not define either of these properties: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

I also checked the rest of the Select code for how these are used. #multiple is currently documented on Select::processSelect(). So, we should decide how to consolidate that documentation.

At first I was confused by #size because there is only a reference to size (no #), but turns out that was within an Element::setAttributes(), which appends the #).

However, it actually looks like the Textfield documentation for #size is an outlier:

[mandelbrot:Element | Tue 20:27:37] $ grep -r "size" * | grep "*"
Date.php:   *   #attributes, #id, #name, #type, #min, #max, #step, #value, #size.
Email.php:   *   Properties used: #title, #value, #description, #size, #maxlength,
File.php: * - #size: The size of the file input element in characters.
File.php:   *   Properties used: #title, #name, #size, #description, #required,
Number.php:   *   #required, #attributes, #step, #size.
Password.php: *   '#size' => 25,
Password.php:   *   Properties used: #title, #value, #description, #size, #maxlength,
PasswordConfirm.php: *   '#size' => 25,
Search.php:   *   Properties used: #title, #value, #description, #size, #maxlength,
Table.php:   *     '#size' => 4,
Tel.php:   *   Properties used: #title, #value, #description, #size, #maxlength,
Textfield.php: * - #size: The size of the input element in characters.
Textfield.php: *   '#size' => 60,
Textfield.php:   *   Properties used: #title, #value, #description, #size, #maxlength,
Url.php: *   '#size' => 30,
Url.php:   *   Properties used: #title, #value, #description, #size, #maxlength,

For comparison, here's a similar search for the #multiple property (irrelevant results removed):

File.php: * - #multiple: A Boolean indicating whether multiple files may be uploaded.
Select.php:   *   - #multiple: (optional) Indicates whether one or more options can be
Tableselect.php: * - #multiple: Set to FALSE to render the table with radios instead checkboxes.

So I think we need to look a bit more closely on how we document these two element properties. To avoid too much scope creep, perhaps we rescope this and split it into two issues: One for documenting #size in a consistent way across the elements that support it (including checking for others that are missing it), and a second similar issue for #multiple.

Um, I'm confused. It looks like the #size element is documented all over the place. For textfields, #size is the width. For selects, it's the number of elements in the list. That is just how HTML is -- the size attribute has multiple meanings depending on what type of HTML element it's on, and our Element classes are correspondingly different. I don't see what needs to be done here to change the patch?

And for multiple, what other elements support it? It looks like three have the docs. In the old 7.x FAPI reference, only select and tableselect had #multiple attributes; those are documented here.

So... I don't think there is anything more to do here. Select was missing docs; this patch adds them. #size is not the same for Select as others, so the docs are different. It doesn't look like anything else is missing docs... please correct me if I'm wrong?

And I don't see it as a problem to have the docs for #multiple in two places. We want it in the main class so that users of Select have one place to look for docs. And the method that actually does the processing, why shouldn't it also document what it is doing?

There's the Migrate random test failure again...

Thanks @jhodgdon. The following elements specifically document what #size means for them:

• File
• Textfield

But all these don't:

• Date
• Email
• Number
• Password
• PasswordConfirm
• Select (current scope of this issue)
• Table
• Tel
• Url

I'm suggesting we change the scope of this issue to address that, rather than making a similar change in all those places and needing to check back to see if we were consistent. After all, this issue is already drawing on the documentation for Textfield.

And the method that actually does the processing, why shouldn't it also document what it is doing?

I agree it should, but what we shouldn't do is have different documentation for it in two places on the class, which is currently what this patch introduces. (Could just be a small change; I think we could address that by simply adding a "See processSelect()" or something, or vice versa.)

Related: It looks like there are a lot of these issues to add missing render element property documentation (thanks especially to @joachim and @jhodgdon for working on all these!). So far though none that I've had a chance to look at refer to any overall meta or plan. Is there such a resource, and if not, can we create a meta issue? Several questions came up in the process of reviewing a few of these and it would help to have a big picture issue where we can discuss them so we don't have the same questions in multiple places. Thanks!

BTW, the Migrate random fail issue is an existing critical: #2724871: Random failure in \Drupal\migrate_drupal_ui\Tests\d7\MigrateUpgrade7Test

OK...

Regarding a meta, we had a meta to get basic documentation on all the form elements. It is still in progress, with a child issue or two still open:
#2486967: [meta] Move/Create Form Element Documentation
Then we have a postponed follow-up issue to check what still needs to be done against the old FAPI reference:
#2603810: [meta] See what else needs updating in Form/Render element docs
but that hasn't been started, since the parent meta hasn't been finished yet. So, the issues exist. Lack of progress due to no one working on them. (I would have time to do them, and many more docs patches, but if I do docs patches, or any other patches for that matter, no one reviews them, so I mostly stick to reviews, so that at least others who do patches get a review.)

Regarding #size, I agree we could expand the scope of this issue to include the other elements, even though #size is quite different for the rest of the elements than what it means on Select, as discussed above. If you insist. I'd prefer to just leave this as it was, but I'm not the person who decides if issue scope is OK.... so whatever. I seem to always be making the wrong scope decisions.

Regarding the processSelect() method, yes let's get rid of the docs there and move them all up to the main Select class.

Updating the issue summary.

Updated patch as per updated Issue Summary.

Thanks @Vinay15: I can confirm that the patch on #38 (a) addresses the three requirements in the description on this ticket and (b) applies cleanly to 8.3.x HEAD.

Committed/pushed to 8.3.x and 8.2.x, thanks!

I cross-committed with the bot marking this needs work, but it's docs-only so shouldn't be the cause of real failures anyway.