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.

Files: 
CommentFileSizeAuthor
#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 ]

Comments

TravisCarden’s picture

Issue summary:View changes

Added "Patch to follow."

TravisCarden’s picture

Status:Active» Needs review
StatusFileSize
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.

TravisCarden’s picture

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

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

jhodgdon’s picture

Status:Needs review» Needs work

Thanks for the patch!

A few questions/things to fix:

a)

+ * 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.

TravisCarden’s picture

Status:Needs work» Needs review
StatusFileSize
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.

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community

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

jhodgdon’s picture

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.

Anonymous’s picture

Issue summary:View changes

Updated issue summary.