The docblock for drupal_set_message() needs improvement. It has at least the following issues:

  • It doesn't specify the return value.
  • It doesn't specify the parameter defaults.
  • It doesn't indicate whether the $message should be translated with t() or not.
  • It doesn't @see closely-related functions.
  • It doesn't explain what Drupal does with the message.
  • The grammar and English style can be improved a little.

Patch to follow.

#4 drupal-drupal_set_message-docblock-1697750-4.patch2.05 KBTravisCarden
PASSED: [[SimpleTest]]: [MySQL] 37,106 pass(es).
[ View ]
#4 interdiff.txt1.73 KBTravisCarden
#2 drupal-drupal_set_message-docblock-1697750-2.patch2.02 KBTravisCarden
PASSED: [[SimpleTest]]: [MySQL] 37,104 pass(es).
[ View ]
#1 drupal-drupal_set_message-docblock-1697750-1.patch2.03 KBTravisCarden
PASSED: [[SimpleTest]]: [MySQL] 37,099 pass(es).
[ View ]


Issue summary:View changes

Added "Patch to follow."

Status:Active» Needs review
new2.03 KB
PASSED: [[SimpleTest]]: [MySQL] 37,099 pass(es).
[ View ]

In the attached patch:

  1. - * Sets a message which reflects the status of the performed operation.
    + * Sets a message to display to the user.

    Tweak the one-line summary to indicate who set messages are for. Since not all messages are, per se, in response to a "performed operation", I removed that clause (but I wouldn't be adamant about that change).
  2. + *
    + * Messages are stored in a session variable and displayed in the "System help"
    + * block.

    Add a slightly expanded description explaining where set messages go and how they're displayed.
  3. + *
    + * Example usage:
    + * @code
    + * drupal_set_message(t('An error occurred and processing did not complete.'), 'error');
    + * @endcode

    A simple example never hurts, and it quickly answers the question I most frequently come here to answer: "Is the message string supposed to be translated or not?"
  4. - * If the function is called with no arguments, this function returns all set
    - * messages without clearing them.
    + * If the function is called without arguments, it returns all set messages
    + * without clearing them.

    Grammar/style issue: Use a pronoun instead of repeating the subject in the same sentence.
  5. - * @param $message
    - *   The message to be displayed to the user. For consistency with other
    - *   messages, it should begin with a capital letter and end with a period.
    + * @param string $message
    + *   The translated message to be displayed to the user. For consistency with
    + *   other messages, it should begin with a capital letter and end with a
    + *   period.

    Indicate that the message string should be translated. As long as I'm touching every line anyway, add the variable data types.
  6. - * @param $type
    - *   The type of the message. One of the following values are possible:
    + * @param string $type
    + *   The message's type. Defaults to 'status'. These values are supported:

    Make the first sentence more direct. Add the default value of the parameter. Change "possible" to "supported". (You can technically use anything for a type—e.g. 'catastrophe'—it just won't get styled in the theme and such.)
  7. - * @param $repeat
    + * @param bool $repeat
      *   If this is FALSE and the message is already set, then the message won't
    - *   be repeated.
    + *   be repeated. Defaults to TRUE.

    Indicate the default value for the parameter.
  8. + *
    + * @return array|null
    + *   A multidimensional array with keys corresponding to the set message types.
    + *   The indexed array values of each contain the set messages for that type.
    + *   Or, if there are no messages set, the function returns NULL.

    Specify the return value!
  9. + *
    + * @see drupal_get_messages()
    + * @see theme_status_messages()

    Link to a couple of related functions.

new2.02 KB
PASSED: [[SimpleTest]]: [MySQL] 37,104 pass(es).
[ View ]

Oops. I got an extra blank docblock line at the end there.

Status:Needs review» Needs work

Thanks for the patch!

A few questions/things to fix:


+ * Messages are stored in a session variable and displayed in the "System help"
+ * block.

I do not think this is accurate. system_process_page() puts them in the $messages theme variable, and the I don't think any existing core themes' page.tpl.php put them in a help region (they definitely don't go in a "block" at all, and many themes don't even have a help region).

b) The $message param description should say it's optional.

c) That paragraph from before about it returning all the messages if there is no input... It *always* returns all the messages, and never clears them, whether or not there is input, so maybe it could just be omitted? It's covered in the return value docs anyway.

Status:Needs work» Needs review
new1.73 KB
new2.05 KB
PASSED: [[SimpleTest]]: [MySQL] 37,106 pass(es).
[ View ]

a) You're right! How about "Messages are stored in a session variable and displayed in page.tpl.php via the $messages theme variable."?

b) Right again. In fact, now that you mention it, all the arguments are optional.

c) I agree.

Updated patch attached.

Status:Needs review» Reviewed & tested by the community

Looks good! I'll get this committed to 8.x and most likely 7.x shortly.

Status:Reviewed & tested by the community» Fixed

Committed to both 8.x and 7.x. Thanks!

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

Issue summary:View changes

Updated issue summary.