Posted by benoit.borrel on November 5, 2009 at 1:41pm
Jump to:
| Project: | Forward |
| Version: | 6.x-1.10 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Hi,
This patch applies the following on forward.module file:
- fix the regression on email subject + message (caused by modification of line 682)
- fix the bug in flood control message (caused by
%numberwhich I replaced with!number) - add the new email subject placeholder
!title(until token support) - add the new template variable
$raw_link - fix minor coding standard issues (revealed by coder module)
One has to resubmit the Forward admin settings form if applying this patch in order to replace obsolete permanent variables forward_page_subject and forward_page_message with forward_email_subject and forward_email_message respectively.
| Attachment | Size |
|---|---|
| forward.module.patch | 11.53 KB |
Comments
#1
+++ sites/all/modules/forward/forward_fixed.module 2009-11-05 13:27:06.000000000 +0000@@ -634,7 +634,7 @@ function forward_form_validate($form, &$
if (!flood_is_allowed('forward', variable_get('forward_flood_control', 10) - count($recipient_addresses) + 1)) {
- form_set_error('recipients', t(variable_get('forward_flood_error', 'You can\'t send more than %number messages per hour. Please try again later.'), array('%number' => variable_get('forward_flood_control', 10))));
+ form_set_error('recipients', t(variable_get('forward_flood_error', 'You can\'t send more than !number messages per hour. Please try again later.'), array('!number' => variable_get('forward_flood_control', 10))));
}
t() shouldn't be used on user-supplied text, only on the string literal supplied as the default.
Also, what is the flood message bug you mentioned? Using %number seems like it would work ok there. If you're going to use !number, then the output of variable_get needs to be run through check_plain to prevent an XSS vulnerability.
I'm on crack. Are you, too?
#2
Hi grendzy,
>t() shouldn't be used on user-supplied text, only on the string literal supplied as the default.
I totally agree.
>Also, what is the flood message bug you mentioned?
When I reached the flood limit, the line 637 was triggered but displayed
You can't send more than !number messages per hour.instead ofYou can't send more than 10 messages per hour.. This was because of a mismatch between placeholders used in variable_get() and t(): variable_get('forward_flood_error', ...) uses !number whereas the t(...) uses %number. And because this is a computed number, ie. not a user submitted data, I fixed the issue by using !number in t(...).On another matter, note that t() shouldn't use escaping quotation: ie not
t('...can\'t...')butt("...can't...").#3
Regarding item #1, the correct value should be page, not email. That was deliberately changed in a recent version. There were related bugs as you discovered, but those should actually already be fixed now (they weren't committed until today). All of the updates have been applied to the dev version. Thanks.
#4
Automatically closed -- issue fixed for 2 weeks with no activity.