Proposed hook_message_alter
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Gman |
| Status: | needs work |
Originally noted at http://www.tech-wanderings.com/Drupal-hook_message_alter, I want to propose a new hook, hook_message_alter.
As I have been working on the functional implementation for a new community Drupal site, I have been seeing the need to customize many of the core Drupal messages (the drupal_set_message function), as required by the needs of the site. A typical example would be when a contact message has been sent, the default message is 'The message has been sent.' An small change might be to 'Thank you, your message has been sent.' A more dynamic change would be 'Thank you Greg, your message has been sent'.
The first change could be accomplished using the locale module methodologies found in http://drupal.org/node/58030. But there are two short-comings to this method. First, even though all core modules use t() for the text to make it translation ready, many contributed modules do not. Second, the translation method only allows for string replacements, not dynamic insertions. Granted some of the messages place the user's name or other dynamic information into the message, but the basic idea is that you can only change what is given you, you can't add to it. The contact message is a perfect example, there is no way to add the user's name to the message.
That is why I am going to propose a new drupal hook, hook_message_alter(&$message = NULL), allowing the $message to be changed by reference. It would hook into the drupal_set_message function. All messages would invoke this hook, giving designers and developers the ability to customize those messages completely to suit their site.
Since the purpose of hooks is to 'Allow modules to interact with the Drupal core', then this hook is needed for any time that the core (or any contributed) messages are not composed the way a site owner would want them to be.
This hook would most likely be used as part of theming/customization of site, and thus not used much in a contibuted module, though it may be used in a contibuted theme if the designer wanted to update the message prose. Personally I would use such a hook to dynamically insert text into the messages at key points in the user experience (especially if I am seeking to make certian conversion points of a client's site personalized).
The new drupal_set_message would pass the $message by reference and change to:
function drupal_set_message($message_id, $message = NULL, $type = 'status') {
if ($message) {
if (!isset($_SESSION['messages'])) {
$_SESSION['messages'] = array();
}
if (!isset($_SESSION['messages'][$type])) {
$_SESSION['messages'][$type] = array();
}
foreach (module_implements('message_alter) as $module) {
$function = $module .'_message_alter';
$function($message);
}
$_SESSION['messages'][$type][] = $message;
}
// messages not set when DB connection fails
return isset($_SESSION['messages']) ? $_SESSION['messages'] : NULL;
} With an implementation of a hook looking like:
function example1_message_alter (&$message = NULL) {
if (strcmp($message, 'The message has been sent.') {
global $user;
$message = 'Thank you ' . $user->name . ' for contacting us. Your message has been sent.';
}
}The return value is discarded. Modify the $message directly.
Please let me know any of your thoughts, as I will present this as an issue at Drupal.org once I have better composed this idea. -Greg
| Attachment | Size |
|---|---|
| hook_message_alter.patch | 527 bytes |
#1
I can not see any negatives of this and there is a beneift. Why not?
The patch was broken I rerolled.
#2
I can not see any negatives of this and there is a beneift. Why not?
The patch was broken I rerolled.
#3
Thanks for the help on the correct implementation chx, and for rerolling the patch. Also, who takes care of the api.drupal.org hook reference? I would hate to 'fire and forget' on this, I want to make sure I learn all the ways to help out.
Also, if approved, would there be a chance to roll this into a 5.x release, since it is fully backward compatible with present code?
Thanks. -Greg
#4
Looks like there was a parse error in there with a missing end quote. Also, I like this, but are there any performance issues to be discussed since dsm is called so much? I know module_implements has a static cache so I guess it wouldn't be too bad.
#5
WIth the current implementation the problem doesn't go away easily, consider the message "The %post has been updated."
How would you go for replacing it with "Thank you Greg, the %post has been updated."?
I'm not sure what would be a good way to achieve this, maybe modify drupal_set_message() to accept the t() array, invoke the hook then pass the message to t(), which would also be a way to force t()ing of all messages.
Btw, wouldn't it be possible to do what this hook does by a using hook_exit() and modifying the $_SESSION['messages'] directly?
#6
In drupal_mail there are several more arguments:
drupal_mail($mailkey, $to, $subject, $body, $from = NULL, $headers = array())
A $mailkey strikes me as the kind of thing we may want in hook_message:
Perhaps a $messagekey ?
@alienbrain this is a patch that needs review and discussion . I think we have enough "info" now we need to discuss implementation.
#7
I essentially proposed adding a message ID here: http://drupal.org/node/106634
and was rejected, FYI.
#8
greggles, drupal_mail() was actually my original inspiration for this and I tried to explore the idea of $message_id/$messagekey, for drupal_set_message(). In a differenet email exchange we hashed out that maybe it wasn't needed, but we didn't take into account already dynamic insertions like %node,which makes strcmp() replacements much harder to implement.
Hmm, catching all those variations of a message would turn into a preg_match pain, so maybe a $messagekey would solve that. In creating the hook, I would just need to switch on the $messagekey, alter the message by reference as desired and be done. Like is:
function example1_message_alter ($messagekey, &$message = NULL) {switch ($messagekey) {
case 'contact_site_mail':
global $user;
$message = 'Thank you ' . $user->name . ' for contacting us. Your message has been sent.';
break;
}
}
Many times the $messagekey can just be the $form_id for consistency. But this kind of change would result in rewriting all the dsm() function calls, more work but it if is the right call I can help out.
Again, the chief reason for desiring this hook is that I don't want to hack core or contributed modules to produce the exact dynamic messages that product managers and design specification demand. The other option is to suppress $messages in the theme or intercept the $_SESSION variable and update the messages, which seems hacky as a long term solution.
pwolanin, maybe this will be a new reason to revisit the topic.
#9
May I ask for +1/-1s for the approach I suggested earlier?
Code may explain better. Instead of the current:
drupal_set_message(t('The %post has been updated.', array('%post' => $node_type));It would be:
drupal_set_message('The %post has been updated.', array('%post' => $node_type));drupal_set_message() will then pass the string AND the array to interested modules via hook_message_alter() which can make modifications if needed, then drupal_set_message() will pass the string and the array to t() to get the final message.
An example of such an implementation of the hook would be:
function example_message_alter(&$message, &$params = array()) {
global $user;
if ($message == 'The %post has been updated.') {
$message = 'Thanks !user, the %post has been updated.';
$params['!user'] = theme('username', $user);
}
}
I think the downside for this approach is that calls to drupal_set_message() must not use t() by themselves which is contrary to the general approach in Drupal. However, drupal_set_message() deals essentially with messages/strings so it makes some sense that it doesn't need to take a t()ed string as it will t() it by itself (i.e. it's not hard for developers to remember that), we already have format_plural() which also behaves in the same way.
#10
+1 from me- seems like a clean and simple approach.
#11
In the development mailing list, there was discussion of acceptable reasons not to use t() in a message, but no further guidance was given there. If there is never an acceptable reason to not t() the string and its arguments, then this approach may be sufficient.
I must admin that I was not thinking about the translation aspect of the processing, but if I am running a multilingual site, then altering the message before translation is highly desirable.
On the other hand, should we force people to alter English messages before they hit translation, or just let them alter the messages after they have been translated into the language they want?
I lean to the former since they have to at least look at the core message to know what to alter anyway.
To keep the ball rolling I think the code from this proposal would look like:
function drupal_set_message(&$message = NULL, &$params = array(), $type = 'status') {
if ($message) {
if (!isset($_SESSION['messages'])) {
$_SESSION['messages'] = array();
}
if (!isset($_SESSION['messages'][$type])) {
$_SESSION['messages'][$type] = array();
}
foreach (module_implements('message_alter) as $module) {
$function = $module .'_message_alter';
$function($message, $params);
}
$message = t($message, $params);
$_SESSION['messages'][$type][] = $message;
}
// messages not set when DB connection fails
return isset($_SESSION['messages']) ? $_SESSION['messages'] : NULL;
}
This change will force a rewrite of all dsm() function calls though.
#12
Would we really want $message and $params to be passed by reference?
#13
Well, this method lets multiple modules have the option to change the message, but since I believe the normal use for this hook will be site specific, then their will only be one module altering the message. Then the by-reference calls won't be needed.
#14
Note- per recent post on the devel list- that there are some points in the Drupal bootstrap cycle where t() is not available.
#15
I was doing more research and I was glad to note that form_set_error() calls drupal_set_message, so that all such form validation messages can be updated in a similar fashion. If and when an API page is written for this, both drupal_set_message and form_set_error messages should be included as examples of messages that can be changed with this hook.
Just this afternoon, my project manager asked for all the 'error' text that they want to 'sanitize'. I slimmed the list down by excluding all the admin and backend messages, but still had a list of 75+ messages that they may change to some extent or other.
Again, if they merely update the text, then I can use the translation method to update the messages, BUT their will be many times they want to include dynamic text, like the example above.
Since it seems that the idea of building the t() inside the drupal_set_message will break certain essential messages (like when the db is down, during installs and the like), then we can't go at that approach.
So we are back to a very simple design of only passing the $message and doing strcmp on it to test if it is the string you want to update from chx (which is essentially backwards compatible), or we add a $messagekey for identification like the form greggles suggested.
Thank you again for reviewing this, I will be patching my site ASAP, but I believe this change can be valuable to many serious site designers going forward.
#16
I was taking my morning swim inside bootstrap.inc and look what I've found :-)
http://api.drupal.org/api/5/function/get_t
#17
I am not sure I am getting your implication correct, but then wouldn't we have to have to do the $t = get_t(); code every time in dsm() or switch it depending on the state of the bootstrap?
Maybe the more basic question is do we want people to alter English messages before translation or to alter already translated text?
I am glad that everyone believe that such alterations are valuable, so I will keep driving for the best implementation. -G
#18
Yes, that's what I meant actually.
I don't think that would affect performance as get_t() seems to be very straightforward and fast as all what it will be doing after the first call is returning the correct localization function from a static variable.
#19
There has not been much input from core devs, though I did get a chance to remind chx about this at the OSCMS Summit, and I mentioned it to Dries in passing as well. I will try to get a brief bit of focus on this small, but needed feature, and get the right implementation at the hackfest after the Summit.
I would like to see the simple, lean implementation of just passing the $message as usual, possibly with a $messagekey if I can get it past chx.
Too much changes without much benefit if we move the t() to inside the drupal_set_message function.
#20
Also, if we use the patch from http://drupal.org/node/127262#comment-212710, it can actually be included in the next 5.x release, not just for 6.x, since it is fully backwards compatible with current usage.
#21
I took a look at this, and I'd like to see two things: an optional message id, and the alterations invoked at *display* time rather than *set* time. Why? On a screen that displays numerous messages, we'll end up calling the alter hooks once for each message. if we call it on the entire list of all messages that are about to be displayed, we can save some time and we can give developers the ability to discover some context in complex scenerios -- for example, seeing that 100+ versions of the same message were set, etc...
#22
Eaton, I definitely see the performance issue with not calling the hook every message. Without the message id, it is no trouble at all, but... I we add a $messagekey of some sort, then it will have to be added to the $_SESSION['messages'] array for the id to persist long enough to be handled by this altering hook before display., versus the id only being needed for the setting of the message, but then discarded before being written into the session variable. Will the extra data storage offset the extra innvocations?
On another note, I believe the use case for this hook is more from the 'design and messaging the user' perspective, not messaging a developer/admin. A dev/admin will usually be content with whatever messages are sent to him/her, but the public face of a site to its user base (which usually are one-off messages, not loads of db error messages during developement) sometimes needs to be precisely managed. Just noting how this hook was conjured up in the first place, as my project manager wants to change many of the stock messages with dynamics messages.
I will churn out a quick patch with the drupal_get_message() implementation.
#23
Note implementation should probably use the new drupal_alter() function: http://drupal.org/node/114774#drupal-alter
and for extra points, and 5.x implementation should easily be adapted to use this for 6.x
(side note- it makes no sense to me why this wasn't called something more evocative like drupal_invoke_alter() )
#24
An alternate suggestion- the main problem highlighted is the inability to add additional tokens when using the locale module to translate a DSM message. Right?
I have the feeling that there is a push to include a set of standard tokens in core, and so for 6.x, the problem might be addressed in this way.
#25
I will update to the drupal_alter() form implementation, thanks for the lead. Also I am not sure that tokens will work to execute the hook into the alteration, but for simplifying adding dynamic content, it may be handy.
I did have a few concerns regarding adding an optional $messagekey if we go the drupal_get_message route.
Can't just use in drupal_set_message
$_SESSION['messages'][$type][$messagekey] = $message;
in the place of
$_SESSION['messages'][$type][] = $message;
because the theme_status_messages function looks for $messages[0] when there is only one message of that type. So we need to fix this theming function OR embed the key into something like which I think is rather messy (set in drupal_set:
$_SESSION['messages'][$type][] = array ('messagekey' => $messagekey, 'message' => $message);
The dgm() or dsm() implementation is almost the same as far as the changes in calling the drupal_alter() hook.
A sample implementation WITHOUT the $messagekey would look like:
function example1_message_alter(&$messages) {foreach($messages['status'] as $key => $message) {
if(!strcmp('Your message has been sent.', $message)) {
global $user;
$messages['status'][$key] = 'Thank you, ' . $user->name . ', your message has been sent.';
}
}
}
The below code is NOT possible unless we fix the theming function and then ONLY if modules are supporting a $messagekey:
function example2_message_alter(&$messages) {if($messages['status']['contact_mail_page']) {
global $user;
$messages['status']['contact_mail_page'] = 'Thank you, ' . $user->name . ', your message has been sent.';
}
}
Again, is we just call during dsm() then we don't have to worry about the structure of the $messages array, since we are altering the message before it is included.
Which route should I go? Continue with optional keys, in dgm() and update the theme function, or do it in dsm()?
#26
Here is the patch that I had put off.
Changed to the drupal_alter() function in drupal_get_message, per Peter (thanks for the insight on that one).
If the optional $messagekey is present, then it is used as the key in the $messages['type'][$messagekey] = $message assignment. Then you can use the example2 code above to kept for the right key and make the changes. If no key is given, or you are too lazy to find it, then you can just do a strstr() or !strcmp() check as you loop through the entire $messages array.
Also,I edited the theme_status_message function to take a keyed array, since that is what must be done for us to place the logic in drupal_get_message().
Please review and comment.
#27
Update patch.
Updated theme_status_messages() to use array shift if only one element instead of array[0], and added note of explanation.
#28
One little nit: $messagekey should be $message_key to be consistent with core var naming.
#29
No problem. Patch updated.
#30
Possibly related:
Very fast t() for a small set of strings
#31
alienbrain, that is a cool feature, but it does not allow for dynamic updates that were the requirement for this code.
I will probably keep up on that other issue to meet other needs though.
I forgot to mark my above as 'code needs review'. It is marked appropriately now.
#32
Could I have another check of this patch? I am hoping not to have let this get stale. The need for the chance to dynamically update messages really is a must in certain situations. Thanks.
#33
A much needed feature.
+1
#34
Nice feature, but patch no longer applies
# patch -p0 < hook_message_alter_5.patch
(Stripping trailing CRs from patch.)
patching file includes/bootstrap.inc
Hunk #1 succeeded at 980 (offset 295 lines).
Hunk #3 FAILED at 721.
1 out of 3 hunks FAILED -- saving rejects to file includes/bootstrap.inc.rej
(Stripping trailing CRs from patch.)
patching file includes/theme.inc
patch: **** malformed patch at line 56:
#35
subscribe
#36
I am waiting for the D6 to get out before starting to re-roll this patch, since it missed the cut off.
I don't want to distract the D6 progress by trying to refocus on this patch now, but once D6 is released I would still like to revisit this issue.
For true, interactive sites with owners desiring precise message control, being able to easily intercept all messages sent through drupal_set_message should be exposed via drupal_alter() or some other mechanism.
-G
#37
In that case, updating status.
Queue maintenance brought to you by Patch Bingo.
#38
Subscribe.
#39
Duplicate of #234320: Add hook_message_alter().
#40
No, this is the original issue. Marked the other issue as the duplicate.
#41
Subscribe
#42
here is a rerole
i'm not sure whether $repeat is used anywhere else i'm not a grep master
i checked it by using
grep "drupal_set_message" . -rn | grep "FALSE"grep "drupal_set_message" . -rn | grep "TRUE"
#43
The last submitted patch failed testing.
#44
grep -rnH drupal_set_message * | grep FALSE
Finds several examples from contributed modules in a local 6x installation.
#45
what does the "H" do?
#46
-H: print filename for each match.