Not a bug, not an inaccurate documentation, but still, shouldn't we have a description of the parameter links under the Drupal 6.x tab, a description such as

links: A keyed array of links to be themed. The key for each link is used as its css class. Each link should be itself an array, with the following keys:
title: the link text
href: the link URL. If omitted, the 'title' is shown as a plain text item in the links list.
html: (optional) set this to TRUE if 'title' is HTML so it will be escaped.

This description for the parameter links is only available under the Drupal 7 tab. It seems to be valid for Drupal 6.x as well, but maybe not because it only appears under D7.

Files: 
CommentFileSizeAuthor
#42 theme_links_doc_needs_expansion-736556-42-d6.patch1.22 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#37 theme_links_doc_needs_expansion-736556-37-d6.patch1.31 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#31 theme_links_doc_needs_expansion-736556-31-d6.patch1.3 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#29 theme_links_doc_needs_expansion-d6-736556-29.patch1.3 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#24 theme_links_doc_needs_expansion-d8-736556-20.patch650 bytesAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 34,626 pass(es).
[ View ]
#22 theme_links_D7_new_736556-21.patch3.47 KBjeckman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_links_D7_new_736556-21.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#20 theme_links_doc_needs_expansion-d6-736556-20.patch1.21 KBjeckman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_links_doc_needs_expansion-d6-736556-20.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#17 theme_links_doc_needs_expansion-d6-736556-17.patch1.21 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#13 theme_links_D7_new_736556.patch3.47 KBdaniels220
PASSED: [[SimpleTest]]: [MySQL] 26,753 pass(es).
[ View ]
#11 theme_links_D7_736556.patch3.38 KBdaniels220
PASSED: [[SimpleTest]]: [MySQL] 26,742 pass(es).
[ View ]
#9 theme_links-new-736556-D6.patch1.01 KBdaniels220
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_links-new-736556-D6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 theme_links-D7-new-736556.patch1.5 KBdaniels220
PASSED: [[SimpleTest]]: [MySQL] 26,744 pass(es).
[ View ]
#5 theme_links_doc-new-736556-D6.patch1.01 KBdaniels220
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_links_doc-new-736556-D6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#5 theme_links_doc-D7-736556.patch1.15 KBdaniels220
PASSED: [[SimpleTest]]: [MySQL] 26,756 pass(es).
[ View ]
#3 theme_links_doc-736556-D6.patch952 bytesdaniels220
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_links_doc-736556-D6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Title:Documentation problem with theme_linkstheme_links doc needs expansion
Version:7.x-dev» 6.x-dev
Issue tags:+Novice

Good point: and it is a documentation bug if the documentation is lacking or incorrect. :)

So the 6.x documentation of theme_links() needs expansion. 7.x seems to be fine, so 6.x can probably mostly copy from that documentation (after verifying whether the pieces work in Drupal 6 too).

Though note that theme_* functions were changed in Drupal 7 to take all of their arguments in one associative array, so the structure is slightly different.

This is still an issue...

Assigned:Unassigned» daniels220
Status:Active» Needs review
StatusFileSize
new952 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_links_doc-736556-D6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Looking through the code it seems like the $links param is handled the same way as the $variable['links'] key in D7, so I copied that section of the doc over.

Version:6.x-dev» 7.x-dev
Status:Needs review» Active

Hmmm.

It looks like $link['attributes'] is also supported. This also needs to be added to the doc in Drupal 7. So we need to fix it there, then go back to Drupal 6 fixing.

Hopefully whoever patches D7 can also change "keyed array" to the more standard "associative array". :)

StatusFileSize
new1.15 KB
PASSED: [[SimpleTest]]: [MySQL] 26,756 pass(es).
[ View ]
new1.01 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_links_doc-new-736556-D6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I took a shot at patching D7. Let's see if my patch passes the testbot. I also modified the description of the 'html' key to match the documentation guidelines—I think. If that's wrong, I'll make a new patch without that change.

D6 patch also attached.

Status:Active» Needs review

Whoops, forgot to set "needs review".

Better....

If we're fixing this up though, let's get the list in compliance with standards:
http://drupal.org/node/1354#lists
The main thing I see is capitalization of the description for each list item:

- item: Description starts with a capital letter, ends with a period.

Also, I prefer the wording "elements" rather than "keys" as in: "Each link should be itself an array, with the following keys:"

And technically, 'attributes' is actually used in both cases -- it's not ignored if href is passed, because l() uses it. There's a note after the list of elements saying "Array items are passed on to the l() function's $options parameter when creating the link.", but maybe that should be moved into the list somehow to make it clearer? What we don't want to do is list all the possible elements that could be used by l() or url(), but I think maybe adding a "..." element to the list would be useful? Maybe something like:

- ...: If the href element is supplied, the entire link array is passed to l() as its $options parameter.

Open to other ideas on that, but obviously as it is, it didn't catch your eye. :)

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.5 KB
PASSED: [[SimpleTest]]: [MySQL] 26,744 pass(es).
[ View ]
new1.01 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_links-new-736556-D6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I realized I misunderstood the function, and I get it now. Easiest to explain my proposed change with another patch. D6 and D7 patches attached.

Status:Needs review» Needs work

A couple of the lines in the latest D7 patch wrap at over 80 characters. (I'd advise holding off on patching D6 until we've established D7).

As long as you are fixing that... In case you can't guess, my philosophy is that when we are fixing docs for a particular function, we fix it up so it conforms to our doc standards and is clear. :)

So here are some other issues, none of which you introduced:

a) Should we change "Each link should be itself an array, with..." to "Each link is an array with..."?

b)

+ *     - html: (optional) Whether or not 'title' is HTML. If set, the title
+ *       will not be escaped.

Should we clarify what "escaped" means? maybe "If set, the title will not be passed through check_plain()."

c)

*   - attributes: A keyed array of attributes.

Should we clarify that these attributes are for the list of links?

d) This section:

*     links. When using an array the following keys can be used:
*     - text: the heading text
*     - level: the heading level (e.g. 'h2', 'h3')
*     - class: (optional) an array of the CSS classes for the heading

is not up to standards. Maybe it should say, "If it's an array, it can have the following elements:", and like the previous list, it doesn't have Description starts capitol, ends period. formatting.

e)

*     Headings should be used on navigation menus and any list of links that
*     consistently appears on multiple pages. To make the heading invisible
*     use the 'element-invisible' CSS class. Do not use 'display:none', which
*     removes it from screen-readers and assistive technology. Headings allow
*     screen-reader and keyboard only users to navigate to or skip the links.
*     See http://juicystudio.com/article/screen-readers-display-none.php
*     and http://www.w3.org/TR/WCAG-TECHS/H42.html for more information.

There's a paragraph break before this paragraph, which is going to make this appear *outside* the @param $variables, item -heading section that it belongs with. The entire @param has to be one "paragraph" with no blank lines. So this information needs to be merged into the rest of the text somehow.

Status:Needs work» Needs review
StatusFileSize
new3.38 KB
PASSED: [[SimpleTest]]: [MySQL] 26,742 pass(es).
[ View ]

Here, try this. I didn't do anything in particular for the paragraph at the bottom—just deleted the blank line. I think it reads okay, though it could be better.

(c) in comment #10 wasn't addressed. I think it's needed for clarity?

Other than that, I think this patch is OK. Thanks...

StatusFileSize
new3.47 KB
PASSED: [[SimpleTest]]: [MySQL] 26,753 pass(es).
[ View ]

Whoops, my bad, new patch attached.

Status:Needs review» Reviewed & tested by the community

Thanks! This looks good now.

When committed, we need to move it down to D6 for porting.

Status:Reviewed & tested by the community» Fixed

Committed. Thanks!

Version:7.x-dev» 6.x-dev
Status:Fixed» Patch (to be ported)

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.21 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Here's a D6 port.

Version:6.x-dev» 8.x-dev
Status:Needs review» Needs work

This looks pretty good... thanks for digging up this old issue!

I did notice this:

"is used as its css class." ==> CSS is an acronym and should be all-caps.

Uh oh, that's also a problem in the Drupal 7 patch. Looks like we should go back to Drupal 7/8 and fix it there first. Sorry! That should be a good Novice project.

backport tagging

Status:Needs work» Needs review
StatusFileSize
new1.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_links_doc_needs_expansion-d6-736556-20.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Rerolled with the css -> CSS change noted in 18

Status:Needs review» Needs work

The last submitted patch, theme_links_doc_needs_expansion-d6-736556-20.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_links_D7_new_736556-21.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Oops. Rerolled the patch from 13 this time - for D7, not the one for D6. Same fix, css to CSS

Status:Needs review» Needs work

The last submitted patch, theme_links_D7_new_736556-21.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new650 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,626 pass(es).
[ View ]

D8 patch

@jeckman Your patch probably failed because this issue is currently set for the 8.x branch. After the test completes for the D8 patch, we can set it to D7 and re-test

Status:Needs review» Reviewed & tested by the community

#24 is good to go... thanks!

Status:Reviewed & tested by the community» Needs review

Committed #24 to 7.x and 8.x. I don't think that is all so marking as 'needs review'.

Version:8.x-dev» 6.x-dev
Status:Needs review» Patch (to be ported)

It looks like everything in 7/8 is OK now. We just need to patch up Drupal 6 so it's as good as 7/8.

StatusFileSize
new1.3 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

D6 backport.

Also, I noticed in D6 there's a return comment that isn't present in D7/D8.

* @return
*   A string containing an unordered list of links.

Status:Patch (to be ported)» Needs work

RE: @return -- that is OK. In D7/8 we are using update docs standards for theme functions, which omit the return.

So this looks correct for D6. The only thing I would change is put a blank line between the @params and the @return.

Status:Needs work» Needs review
StatusFileSize
new1.3 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

As you wish! :)

Status:Needs review» Reviewed & tested by the community

Great, thanks!

Standards for docs are at http://drupal.org/node/1354 by the way, in case you didn't realize. :)

Status:Reviewed & tested by the community» Needs work

Why are we repeating the docs of l() here? Why not put in a @see and just tell people that theme_links() would reuse l() and its arguments?

Status:Needs work» Reviewed & tested by the community

The array keys in this case are not the same as the arguments to l() actually. For instance, it uses 'href' instead of 'path'. I think it needs separate documentation.

Status:Reviewed & tested by the community» Needs work

+++ b/includes/theme.inc
@@ -1191,12 +1191,25 @@ function theme_status_messages($display = NULL) {
+ *   A keyed array of attributes for the UL containing the list of links.

Since "A keyed array..." was changed to "An associative array..." for $links, shouldn't it be changed here as well?

yes, good catch!

Status:Needs work» Needs review
StatusFileSize
new1.31 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Updated patch with fix from #35.

Status:Needs review» Reviewed & tested by the community

Looks good to me...

Status:Reviewed & tested by the community» Needs work

I do not think that this is correct for Drupal 6, on the attributes in each link array:

If element 'class' is included, it must be an array of one or more class names.

It looks like this is getting passed to either l() or drupal_attributes(), and in D6, both would be expecting the class attribute to be a string.

Right?

@jhodgdon You are correct. We should remove that and I see another element in the array we didn't account for, $links['language']. Since its the default language object, do we just use a @see?

I think we would be better off just saying that additional parts of the array are passed to l(), like we did in D7/8, and leaving it at that. Only put in docs for parts that would be used if href is missing.

Status:Needs work» Needs review
StatusFileSize
new1.22 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Removed the D7/8 specific portion.

Status:Needs review» Reviewed & tested by the community

That looks good for D6. Thanks!

Status:Reviewed & tested by the community» Fixed

Committed, pushed to Drupal 6, thanks.

Status:Fixed» Closed (fixed)
Issue tags:-needs backport to D6, -Novice, -needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.