theme_link() parameters missing detail

Agreed, should document that $variables['options'] needs to contain at least 'attributes' and 'html' keys.

I cannot figure out where this is in D8 to see if we need to document it there, but I guess not.

I added more description and changed 'options' key to 'options' array and what it contains.

also should add 'language' in $variables

I was going off of the results from a dpm($variables). Is 'language' a key or an array of its own?

Hm.

So... I think we should only document here the parts of $variables that this function actually uses and needs. I also think that we shouldn't reproduce documentation elsewhere.

Looking at the code for this function, the parts of $variables that it uses directly are:
- path
- text
- options
-- options['attributes']
-- options['html']

And then it also passes options into the url() function, so let's say that in the docs rather than trying to list everything that url() might use in options. And let's not document other things that might be present in $variables that this function doesn't actually use -- there is a lot of stuff that gets passed to theme functions from the standard preprocessing, but we don't want to document it unless the function actually needs it (plus, it's not stuff that the user of this theme hook would normally need to provide).

Thanks!

Would you recommend mentioning @see url() along with the @see l() instead of duplicating the documentation for those functions?

yes, sounds like a good idea.

I added a little bit about the url() function

Thanks! Looking better. A few thoughts:

1. + *   An associative array containing the keys 'text', 'path', and the
   + *   'options' array which contains the 'attributes', and 'html' keys.
   + *   The 'path' and 'options' values are passed into the url() function to
   + *   build the link. See the l() function for information about these
   + *   variables.
   

   I found this a bit awkward to read... How about something like this:

   An associative array with elements 'text', 'path', and 'options', corresponding to the parameters to the l() function. The 'path' and 'options' elements are passed into the url() function to build the link, and in the 'options' array, elements 'attributes' and 'html' are used in this function like they are in l().

2. +++ b/includes/theme.inc
   @@ -1710,11 +1710,16 @@ function theme_status_messages($variables) {
     * @see l()
   + *
   + * @see url()
     */
   

   Remove blank line here. We normally bump all the @see lines together into one block.

Now you're almost doing my job for me!

Looks good, thanks!

Fair enough. Yes, a sub-list may be clearer, and saying explicitly that things are required is probably a good idea.

Let's try it again.

Thanks for the new patch -- it's pretty close! A few things to fix:

1. +++ b/includes/theme.inc
   @@ -1710,11 +1710,25 @@ function theme_status_messages($variables) {
   + *   An associative array containing the keys:
   + *     - text: The text of the link
   + *     - path: The URL of the link
   

   The list is indented two extra spaces. The - should line up under the text above it.

   Also each entry should end in . according to https://www.drupal.org/node/1354#lists

2. +++ b/includes/theme.inc
   @@ -1710,11 +1710,25 @@ function theme_status_messages($variables) {
   + *     - options: An associative array containing optional values.  It
   

   um, "options" is not for "optional values", it's for the options for the l() and url() functions, right?

   And somehow this patch lost the information saying to look at l() for documentation of what these things are?

3. +++ b/includes/theme.inc
   @@ -1710,11 +1710,25 @@ function theme_status_messages($variables) {
   + *         - Classes must be declared in an array: 'class' => array('class_name').
   

   This line is too long, but if you fix the indentation I think it will be OK.

4. +++ b/includes/theme.inc
   @@ -1710,11 +1710,25 @@ function theme_status_messages($variables) {
   + *   The link itself is built by the url() function which takes
   

   Needs comma before "which".

Since the options array is optional and defaults to an empty array in l() and url(), I put the optional tag at the beginning of the description. I also made the formatting and grammatical changes you requested.

+++ b/includes/theme.inc
@@ -1710,11 +1710,28 @@ function theme_status_messages($variables) {
+ *     - attributes: Can contain optional attributes:
+ *       - Classes must be declared in an array: 'class' =>
+ *         array('class_name').
+ *       - Titles must be strings: 'title' => 'Example title'
+ *       - Others are more flexible as long as they work with
+ *         drupal_attributes($variables['options']['attributes]).
+ *     - html: Boolean flag that tells whether text contains

I found this bit slightly jarring to read. The rest is all
- key: explanation
and this bit is
- explanation, maybe getting to the specific key at the end

So maybe the first two items in this array could be changed to be
- class: ...
- title: ...
And leave the "Others are... " item alone?

The rest of this patch looks great, thanks!

Here you go.

Much better, thanks!

But... I guess I didn't notice this before, sorry!

+++ b/includes/theme.inc
@@ -1710,11 +1710,28 @@ function theme_status_messages($variables) {
+ *   - text: The text of the link
+ *   - path: The URL of the link

These should end in .

And technically... is path the URL of the link, really? I don't think so. Probably should check the url() function to see what this needs to be, or say something like "... as in the $path parameter to url()".

I copied the description of $path from url() and changed it slightly to fit this function. "path: the internal path or external url being linked to."

Thanks! I think this is very good now. Let's do it!

Oh frabjous day!

Committed to 7.x - thanks!

Adjusted the following on commit to emphasize the security requirement a bit more (similar to how the documentation on l() does it):

diff --git a/includes/theme.inc b/includes/theme.inc
index befdc9b..1accdce 100644
--- a/includes/theme.inc
+++ b/includes/theme.inc
@@ -1722,9 +1722,9 @@ function theme_status_messages($variables) {
  *       - title: must be a string. Example: 'title' => 'Example title'
  *       - Others are more flexible as long as they work with
  *         drupal_attributes($variables['options']['attributes]).
- *     - html: Boolean flag that tells whether text contains
- *       html or plain text.  If not set to TRUE the text value will be
- *       sanitized.
+ *     - html: Boolean flag that tells whether text contains HTML or plain
+ *       text. If set to TRUE, the text value will not be sanitized so the
+         calling function must ensure that it already contains safe HTML.
  *   The elements $variables['options']['attributes'] and
  *   $variables['options']['html'] are used in this function similarly to the
  *   way that $options['attributes'] and $options['html'] are used in l().