Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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 witht()
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.
Comment | File | Size | Author |
---|---|---|---|
#4 | drupal-drupal_set_message-docblock-1697750-4.patch | 2.05 KB | TravisCarden |
#4 | interdiff.txt | 1.73 KB | TravisCarden |
#2 | drupal-drupal_set_message-docblock-1697750-2.patch | 2.02 KB | TravisCarden |
#1 | drupal-drupal_set_message-docblock-1697750-1.patch | 2.03 KB | TravisCarden |
Comments
Comment #0.0
TravisCarden CreditAttribution: TravisCarden commentedAdded "Patch to follow."
Comment #1
TravisCarden CreditAttribution: TravisCarden commentedIn the attached patch:
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).
Add a slightly expanded description explaining where set messages go and how they're displayed.
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?"
Grammar/style issue: Use a pronoun instead of repeating the subject in the same sentence.
Indicate that the message string should be translated. As long as I'm touching every line anyway, add the variable data types.
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.)
Indicate the default value for the parameter.
Specify the return value!
Link to a couple of related functions.
Comment #2
TravisCarden CreditAttribution: TravisCarden commentedOops. I got an extra blank docblock line at the end there.
Comment #3
jhodgdonThanks for the patch!
A few questions/things to fix:
a)
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.
Comment #4
TravisCarden CreditAttribution: TravisCarden commenteda) 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.
Comment #5
jhodgdonLooks good! I'll get this committed to 8.x and most likely 7.x shortly.
Comment #6
jhodgdonCommitted to both 8.x and 7.x. Thanks!
Comment #7.0
(not verified) CreditAttribution: commentedUpdated issue summary.