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.
Comments
Comment #1
jhodgdonGood 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.
Comment #2
jhodgdonThis is still an issue...
Comment #3
daniels220 CreditAttribution: daniels220 commentedLooking 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.
Comment #4
jhodgdonHmmm.
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". :)
Comment #5
daniels220 CreditAttribution: daniels220 commentedI 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.
Comment #6
daniels220 CreditAttribution: daniels220 commentedWhoops, forgot to set "needs review".
Comment #7
jhodgdonBetter....
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:
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:
Open to other ideas on that, but obviously as it is, it didn't catch your eye. :)
Comment #8
jhodgdonComment #9
daniels220 CreditAttribution: daniels220 commentedI realized I misunderstood the function, and I get it now. Easiest to explain my proposed change with another patch. D6 and D7 patches attached.
Comment #10
jhodgdonA 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)
Should we clarify what "escaped" means? maybe "If set, the title will not be passed through check_plain()."
c)
Should we clarify that these attributes are for the list of links?
d) This section:
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)
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.
Comment #11
daniels220 CreditAttribution: daniels220 commentedHere, 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.
Comment #12
jhodgdon(c) in comment #10 wasn't addressed. I think it's needed for clarity?
Other than that, I think this patch is OK. Thanks...
Comment #13
daniels220 CreditAttribution: daniels220 commentedWhoops, my bad, new patch attached.
Comment #14
jhodgdonThanks! This looks good now.
When committed, we need to move it down to D6 for porting.
Comment #15
Dries CreditAttribution: Dries commentedCommitted. Thanks!
Comment #16
jhodgdonComment #17
Albert Volkman CreditAttribution: Albert Volkman commentedHere's a D6 port.
Comment #18
jhodgdonThis 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.
Comment #19
jhodgdonbackport tagging
Comment #20
jeckman CreditAttribution: jeckman commentedRerolled with the css -> CSS change noted in 18
Comment #22
jeckman CreditAttribution: jeckman commentedOops. Rerolled the patch from 13 this time - for D7, not the one for D6. Same fix, css to CSS
Comment #24
Albert Volkman CreditAttribution: Albert Volkman commentedD8 patch
Comment #25
Albert Volkman CreditAttribution: Albert Volkman commented@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
Comment #26
jhodgdon#24 is good to go... thanks!
Comment #27
Dries CreditAttribution: Dries commentedCommitted #24 to 7.x and 8.x. I don't think that is all so marking as 'needs review'.
Comment #28
jhodgdonIt 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.
Comment #29
Albert Volkman CreditAttribution: Albert Volkman commentedD6 backport.
Also, I noticed in D6 there's a return comment that isn't present in D7/D8.
Comment #30
jhodgdonRE: @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.
Comment #31
Albert Volkman CreditAttribution: Albert Volkman commentedAs you wish! :)
Comment #32
jhodgdonGreat, thanks!
Standards for docs are at http://drupal.org/node/1354 by the way, in case you didn't realize. :)
Comment #33
Gábor HojtsyWhy 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?
Comment #34
jhodgdonThe 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.
Comment #35
Kristen PolSince "A keyed array..." was changed to "An associative array..." for
$links
, shouldn't it be changed here as well?Comment #36
jhodgdonyes, good catch!
Comment #37
Albert Volkman CreditAttribution: Albert Volkman commentedUpdated patch with fix from #35.
Comment #38
Kristen PolLooks good to me...
Comment #39
jhodgdonI do not think that this is correct for Drupal 6, on the attributes in each link array:
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?
Comment #40
Albert Volkman CreditAttribution: Albert Volkman commented@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?
Comment #41
jhodgdonI 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.
Comment #42
Albert Volkman CreditAttribution: Albert Volkman commentedRemoved the D7/8 specific portion.
Comment #43
jhodgdonThat looks good for D6. Thanks!
Comment #44
Gábor HojtsyCommitted, pushed to Drupal 6, thanks.