Some of the attributes used by theme_fieldset are optional, and some are required. The documentation should specify which is which.


--- includes/form.inc
+++ includes/form.inc
@@ -2624,6 +2624,7 @@ function form_get_options($element, $key) {
  *   - element: An associative array containing the properties of the element.
  *     Properties used: #attributes, #children, #collapsed, #collapsible,
  *     #description, #id, #title, #value.
+ *     Required properties:  #attributes (must be an Array), #children (must be a String).
  *
  * @ingroup themeable
  */
CommentFileSizeAuthor
#17 form.inc_.patch1.4 KBpillarsdotnet
#14 form.inc_.patch1.4 KBpillarsdotnet
#11 form.inc_.patch1.46 KBpillarsdotnet
#8 form.inc_.patch1.42 KBpillarsdotnet
#5 form.inc_.patch1.12 KBpillarsdotnet
#6 form.inc_.patch1.12 KBpillarsdotnet
#3 file.inc_.patch889 bytespillarsdotnet
form.inc_.patch509 bytespillarsdotnet

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Title: Documentation problem with theme_fieldset » theme_fieldset should say which properties are required.
Status: Active » Needs work

Thanks for the issue report...

I am not completley happy with the solution, though. How about just saying:

Properties used (all are optional except as noted): #attributes (required, must be an array), #children (required, must be a string), #collapsed, ...

If you do want to make it a separate sentence, please (a) don't capitalize array/string, and (b) don't start this on a new line (continue the paragraph begun on the previous line).

Finally, when you attach a patch, please set the status to "needs review".

Thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Actually, maybe the elements should be made into a list?

pillarsdotnet’s picture

pillarsdotnet commented
Status: Needs work » Needs review
StatusFileSize
new file.inc_.patch889 bytes

As you wish. Actually, I'd rather change the function to gracefully deal with missing properties, but this is more likely to be accepted:

--- includes/form.inc
+++ includes/form.inc
@@ -2622,8 +2622,9 @@ function form_get_options($element, $key) {
  * @param $variables
  *   An associative array containing:
  *   - element: An associative array containing the properties of the element.
- *     Properties used: #attributes, #children, #collapsed, #collapsible,
- *     #description, #id, #title, #value.
+ *     Properties used: #attributes (required; must be an array), #children
+ *     (required; must be a string), #collapsed, #collapsible, #description, 
+ *     #id, #title, #value.
  *
  * @ingroup themeable
  */
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

At this point, I think you are correct that the documentation is more likely to be accepted.

The patch you attached here doesn't appear to be the right patch. The text you pasted into comment #3 looks good.

pillarsdotnet’s picture

pillarsdotnet commented
StatusFileSize
new form.inc_.patch1.12 KB

Looking around, other docs that specify required vs. optional do it this way:

--- includes/form.inc
+++ includes/form.inc
@@ -2622,8 +2622,15 @@ function form_get_options($element, $key) {
  * @param $variables
  *   An associative array containing:
  *   - element: An associative array containing the properties of the element.
- *     Properties used: #attributes, #children, #collapsed, #collapsible,
- *     #description, #id, #title, #value.
+ *     Properties used:
+ *     - "#attributes": An array of HTML attributes of the fieldset tag. (Required)
+ *     - "#children": Preprocessed HTML body of the fieldset. (Required)
+ *     - "#collapsed": Whether the fieldset is initially collapsed. Defaults to FALSE.
+ *     - "#collapsible": Whether the fieldset may be collapsed.  Defaults to FALSE.
+ *     - "#description": An optional description to insert at the top of the fieldset.
+ *     - "#id": An optional ID attribute for the fieldset tag.
+ *     - "#title": An optional title to be wrapped in a legend tag.
+ *     - "#value": An optional text string to insert at the bottom of the fieldset.
  *
  * @ingroup themeable
  */
pillarsdotnet’s picture

pillarsdotnet commented
Status: Needs work » Needs review
StatusFileSize
new form.inc_.patch1.12 KB

Minor correction, with corresponding edit to the inline code above:

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Please just post patches. It isn't necessary to repeat the patch in the text of your comment. Thanks!

The version you have in the comment on #5 and the patch in #6 is not following our current standards, which are at:
http://drupal.org/node/1354
Use your browser to search for "optional" and you'll see what the standards are.

pillarsdotnet’s picture

pillarsdotnet commented
StatusFileSize
new form.inc_.patch1.42 KB

Thanks for the reference. Cleaned up according to the link you posted.

pillarsdotnet’s picture

pillarsdotnet commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Much better! A few minor edits needed:

a) + * - #description: (optional) A string to insert before the #children text
Needs to end in a .

b) + * - #title: (optional) A string to use as the fieldset title. If #collapsible
One space, not two, after the .

c) + * - #collapsed: (optional) Boolean whether the fieldset body is initially
(and the next one too) -- "Boolean whether the fieldset body..." is not a good sentence. You can probably change this to "Whether or not the fieldset body...", and leave out the word boolean, since "Defaults to FALSE" afterwards indicates that it is a boolean value.

d) Do you think we should add on #children something like "along with #description and #value"? And maybe indicate that this is normally built from the rendered children of the fieldset before the theme function is called?

pillarsdotnet’s picture

pillarsdotnet commented
StatusFileSize
new form.inc_.patch1.46 KB

a) -- Done.
b) -- Done.
c) -- I don't like the wording either, but it's copied almost verbatim from the standards documentation you said I should follow.
d) -- Dunno; I don't know the history nor reasoning behind why the function works the way it does. The only reason I'm getting involved is because another contrib module uses this function wrongly, and I can't gripe at the author for failing to grasp the unclear documentation.

Somewhat improved (I think) patch:

pillarsdotnet’s picture

pillarsdotnet commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

This version is fine, thanks!

I also fixed up the awkward wording around "Boolean whether" on the standards page. :)

pillarsdotnet’s picture

pillarsdotnet commented
StatusFileSize
new form.inc_.patch1.4 KB

After receiving an education from Dave Reid and marcingy (See http://drupal.org/node/1004578#comment-3855230 and http://drupal.org/node/1004556#comment-3855212), I realize now the "required" vs. "optional" discussion is moot.

Correcting...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, form.inc_.patch, failed testing.

marcingy’s picture

marcingy commented
Status: Needs work » Closed (works as designed)

Closing as this isn't a bug.

pillarsdotnet’s picture

pillarsdotnet commented
Title: theme_fieldset should say which properties are required. » Better docs for theme_fieldset
Category: bug » feature
Priority: Normal » Minor
Status: Closed (works as designed) » Needs review
StatusFileSize
new form.inc_.patch1.4 KB

@jhodgdon -- see http://drupal.org/node/1004852
@marcingy -- You're right. Adjusting accordingly.

marcingy’s picture

marcingy commented
Status: Needs review » Closed (won't fix)
jabeeroo’s picture

jabeeroo commented
Status: Closed (won't fix) » Needs review

#17: form.inc_.patch queued for re-testing.

pillarsdotnet’s picture

pillarsdotnet commented
Status: Needs review » Closed (won't fix)