This issue is to rename a method in HTMLMailSystem:
- protected static function _isShellSafe($string) {
+ protected static function isShellSafe($string) {Leading underscores to indicate a "private" function are not needed and not wanted - the method visibility (and not a naming convention) should be used to control access to the method.
This also addresses a @todo in the code and fixes one of the last remaining coding standards problems with this module.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 3167272-8-is-shell-safe.patch | 1.75 KB | salvis |
| #2 | 3167272-2-is-shell-safe.patch | 1.86 KB | tr |
Comments
Comment #2
tr commentedComment #3
tr commentedComment #4
tr commentedComment #6
tr commentedCommitted. Commit 6365635 actually belongs to #3163507: file_scan_directory() is deprecated, but I wrote the wrong commit message which is why it shows up here.
Comment #8
salvisThe ToDo
was copied from Core and is still there. I agree that it was a bit confusing to see it here.
The method moved to the PhpMail class, but its name in Core still has an underscore. If you want to get rid of the underscore, that's fine, but then we have to make an extra effort to keep the link to Core, because this is a security issue.
Let's use
getDefaultFromMail()though, to keep $site_mail consistent.Comment #10
salvisComment #11
tr commentedGood point. Thanks for catching that.
PhpMail::_isShellSafe() is the core method, but we're not overriding that here or extending/using PhpMail in any way. It is a static function, however, so perhaps we should just call that PhpMail method here instead of using a copy of its code? Oh, but we can't because it's protected ...
I think now I understand the comment better - it makes no sense to have this protected static method in PhpMail, it should probably be a public static method in the core MailFormatHelper. Perhaps we should take a step in that direction and move our copy of isShellSafe() into HtmlMailHelper?
Comment #12
salvisYes, it would be really, really nice to be able to call that function in Core, not just for HTML Mail but also for a bunch of other contribs. I've tried to get it public in s.d.o, but I was told we wanted to minimize changes to public interfaces in security issues. I agree with the general idea, but the damage of leaving behind this mess is much greater than the one of adding a simple function.
I doubt we can make a lasting impression with a purely symbolic act like this. :-)
I put it into HtmlMailSystem at the time because that was the only place where it's used. Keeping the security issue in a single file and not disturbing the history by moving things has its advantages, too.