Closed (won't fix)
Project:
Notify
Version:
5.x-1.3
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
1 Jun 2007 at 01:22 UTC
Updated:
24 Aug 2013 at 18:13 UTC
Jump to comment: Most recent file
I've got a drupal site name that has a single quote in it, e.g. Foo's Bar. When notify sends an email, otherwise working great, the subject line is Foo's Bar new content notification for admin.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | notify-5-148454-1.patch | 5.62 KB | Standart |
Comments
Comment #1
mygumbo commentedArgh. It shows up as
Foo' ;s Bar
Comment #2
nicokarp commentedI had similar behaviour with single quotes within title (nodes or comments).
I did a little hack by replacing a line in notify.module :
"$body = $node_body . $comment_body;"
to
"$body = notify_entities_to_utf8($node_body . $comment_body);"
For your problem, same thing could be done for the variable $subject
I did this very quickly... working for me but not clean (newbie as i am).
Comment #3
martinship commentedSingle quotes aren& #039;t the only problem: that& #039;s the apostrophe too. It& #039;s visually ugly to say the least. Since contractions are pretty common, I& #039;d say this is a normal bug, not a & #039;minor& #039; one. I& #039;m sure you see my point.
Sorry, I& #039;ve just seen this in all my notification emails and it& #039;s driving me nuts ...
Comment #4
gracearoha commentedI also have this problem with double quotes showing up like:
"My Pages"this is also driving me crazy. Has anyone found a solution to these odditites?
Comment #5
merilainen commentedI have the same problem. First I thought it has something to do with FCKeditor, but it doesn't. Email notifications for Organic Groups are pretty much unusable, as single quote is quite common character.
Comment #6
Standart commentedMany variables like sitename, title, author, etc are run through htmlspecialchars() which encodes '&', '"', ''', '<', and '>'. This makes sense if you're displaying the variables in HTML which we don't do here. The escaping takes place because of the usage of the '@' decorator for variables in the t()-function which in turn uses check_plain() which contains htmlspecialchars(). The docs say that you should use the '!' decorator for variables supposed to appear in an email (http://api.drupal.org/api/function/t/5). Why does notify use '@'? Also a statement like
doesn't make much senseto me as you translate a varibale only which is the same as doing
if I understand this correctly. Shouldn't we change all decorators from '@' to '!' in t() and also remove statements like the one above completely (we don't need to check for security in an HTML context at all, do we)?
Comment #7
matt2000 commentedI've noticed this issue while I've been doing some work on making messages templatable in notify by views.
If someone can provide a patch for notify, I'll apply it & release, but if I have to do it myself, I may not get to it for a little while yet.
Comment #8
Standart commentedAttached is my patch against 5.x-1.x-dev. I replaced all @ decorators with !. Also I removed the empty t() for the item title.
Comment #9
matt2000 commentedInitial glance at patch look good, so it's been applied to -dev branch.
Please test & report back.
Comment #10
matt2000 commentedAlso applied to 6.x -dev branch.
Comment #11
Standart commentedPlease apply it to the 5.x-dev branch, too.
Comment #12
matt2000 commentedHmm... trying to figure out what happened to the tagging with this module.
In the meantime, a 5.x release is here: http://drupal.org/node/303336
Comment #13
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #14
Standart commentedPlease apply to the 5.x-1.x branch and make a new release 5.x-1.2.
Comment #15
neclimdulRan across this patch on a random google search.
Knowing how bad some email clients can be about running scipts(not saying names but I think we all know) this kinda scares me. People aren't as good about keeping them up to date either.
Shouldn't you run some sort of filter on some of this information at least? It will look goofy but drupal will let you put
<em>test<script>alert('hello');</script></em>testinginto a title. It just takes one email client flaw and...Comment #16
Standart commentednotify.module uses PHP's
strip_tags()(Version 5) ordrupal_html_to_text()(Version 6) respectively so don't worry. We're not sending HTML here.Comment #17
Standart commentedLooks like it's been committed. Thanks.
Comment #19
chris55 commentedThe patch in comment #8 notify-5-148454-1.patch is flaky because the replacement has been done too thoroughly.
In particular, the format_plural() function assumes the use of @ not ! so all the counts generated by the module now show up as !count. So please change those back again. (3 instances)
e.g. t('Recent content - @count', array('@count' => format_plural(count($nodes), '1 new post', '@count new posts')))
Comment #20
chris55 commentedsee above
Comment #21
Standart commentedIs this valid for the Drupal 5 version? See http://drupal.org/node/316234#comment-1041160 for Drupal 6.
Comment #22
gisleVerified that this is not an issue is not present in 6.x-dev and 7.x-dev-branches.
It is not going to be backported to 5.x.