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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Title: Documentation problem with theme_links » theme_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.

jhodgdon’s picture

This is still an issue...

daniels220’s picture

Assigned: Unassigned » daniels220
Status: Active » Needs review
FileSize
952 bytes

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.

jhodgdon’s picture

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". :)

daniels220’s picture

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.

daniels220’s picture

Status: Active » Needs review

Whoops, forgot to set "needs review".

jhodgdon’s picture

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. :)

jhodgdon’s picture

Status: Needs review » Needs work
daniels220’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
1.01 KB

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.

jhodgdon’s picture

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.

daniels220’s picture

Status: Needs work » Needs review
FileSize
3.38 KB

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.

jhodgdon’s picture

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

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

daniels220’s picture

Whoops, my bad, new patch attached.

jhodgdon’s picture

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.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks!

jhodgdon’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)
Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.21 KB

Here's a D6 port.

jhodgdon’s picture

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.

jhodgdon’s picture

backport tagging

jeckman’s picture

Status: Needs work » Needs review
FileSize
1.21 KB

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.

jeckman’s picture

Status: Needs work » Needs review
FileSize
3.47 KB

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.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
650 bytes

D8 patch

Albert Volkman’s picture

@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

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

#24 is good to go... thanks!

Dries’s picture

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'.

jhodgdon’s picture

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.

Albert Volkman’s picture

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.
jhodgdon’s picture

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.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.3 KB

As you wish! :)

jhodgdon’s picture

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. :)

Gábor Hojtsy’s picture

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?

jhodgdon’s picture

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.

Kristen Pol’s picture

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?

jhodgdon’s picture

yes, good catch!

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.31 KB

Updated patch with fix from #35.

Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me...

jhodgdon’s picture

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?

Albert Volkman’s picture

@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?

jhodgdon’s picture

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.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.22 KB

Removed the D7/8 specific portion.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

That looks good for D6. Thanks!

Gábor Hojtsy’s picture

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.