When you "Edit notification texts" on the admin/settings/login_security screen, only certain field changes are actually affecting the output of the notifications when used. So this is not an issue of the admin screen form submitting properly (I think) because I can see the changes I've saved/made on the admin screen. The problem is the changes actually being output in the email notification.

My example is with the "Email to be sent to the administrator user (uid 1) for ongoing attack detections." I've changed the Email subject and Email body for this item. When I get the email notification it has the Subject changes, but not the Body changes. The body of the email is just the canned, default message. Not sure about the on-screen notifications as I didn't test any of those.

Also, placeholders seem to be wrapped in <em></em> tags by default, even if I don't write them in there.

P.S. Great module!

CommentFileSizeAuthor
#13 login_security-730288-13.patch23.75 KBilo
#8 login_security-730288-8.patch21.23 KBheltem
#1 login_security-mail_body-730288.patch2.7 KBheltem

Comments

heltem’s picture

heltem commented
StatusFileSize
new login_security-mail_body-730288.patch2.7 KB

Here is a patch for mail body.

But I haven't found where does the <em></em> placeholder wrapping come from, yet.

deekayen’s picture

deekayen commented

#1 committed to DRUPAL-6--1 and HEAD

deekayen’s picture

deekayen commented

When using % with t(), that adds em tags.

%variable, which indicates that the string should be HTML escaped and highlighted with theme_placeholder() which shows up by default as emphasized.

I didn't see any use of core's t() to cause em additions, unless somehow the declaration at define() is doing it.

heltem’s picture

heltem commented

I guess the guilty is login_security_t() function which call theme('placeholder', ....) which does the emphasis.

I suggest :

--- login_security.module
+++ login_security.module
@@ -383,7 +383,7 @@
  */
 function login_security_t($message, $variables = array()) {
   foreach ($variables as $key => $value) {
-    $variables[$key] = theme('placeholder', $value);
+    $variables[$key] = check_plain($value);
   }
   return strtr($message, $variables);
 }
deekayen’s picture

deekayen commented

How about instead swapping the % out for @ in the relevant define()s up top?

heltem’s picture

heltem commented

Probably because, change only this won't change anything.
The emphasis does not come from the % but from the call to theme('placeholder', ...) that calls theme_placeholder()

But, maybe you can combine @ use with a t() call instead of calling theme('placeholder', ....)
Indeed, I see no reason to call theme() rather than t()...

deekayen’s picture

deekayen commented

@heltem: Care to propose a patch to wrap this up?

heltem’s picture

heltem commented
Status: Active » Needs review
StatusFileSize
new login_security-730288-8.patch21.23 KB

Here is a patch for 6.x-1.0.

Please review.

grub3’s picture

grub3 commented

I cannot even see admin/settings/login_security
using latest dev release.

deekayen’s picture

deekayen commented

@jmpoure: you mean it shows just a totally white screen, you get a 403 denied error, or the strings in the patch aren't displaying?

grub3’s picture

grub3 commented

Sorry my late reply. I cannot see a link in the admin panel for admin/settings/login_security screen.
When I enter the URL manually http://www.mysite.org/admin/settings/login_security, nothing displays.

I am using Drupal latest version with PostgreSQL.

timwood’s picture

timwood
Primary language English
Location Rockville, Maryland
commented

Sorry, I haven't been able to review the latest patch. Has anyone else been able to do so? Will this patch make it into a full release soon?

ilo’s picture

ilo commented
StatusFileSize
new login_security-730288-13.patch23.75 KB

I have updated the patch to remove a pair of coder issues. I haven't tested appart of the tests included in the module. Could anyone using smtp server (Heltem?) verify the email now does not contain the tags?

(I've verified that admin/settings/login_security is already there)..

deekayen’s picture

deekayen commented
Status: Needs review » Closed (duplicate)

#1722814: <em> tags are showing up in email subject line when using %site

  • deekayen committed bf79c2b on 6.x-1.x, 8.x-1.x 
    #730288 by heltem - use correct email body variables