Custom Sender address?

Sounds like a useful feature. I will certainly take a look at the patch. I prefer to make things options instead of running by default.

Thanks
Robert

OK, find attached my cack-handed attempt at generating a patch file. (It seems to be more or less the same format as a patch file - I generated it using Komodo Edit's "compare" function, comparing the new file to an unmodified version. I don't know what an actual patch interpreting application would make of it.)

I elected to use PHP's strtr() function directly instead of going by Drupal's t(), broadly because I don't want email addresses added to the translation system. I also amended the other uses of t() - we're basically using it for string substitution here, instead of actual translation, which probably isn't best practice, and besides if any translation does happen it will be into the native language of the person registering, not the person receiving the mail (the language code argument isn't introduced until Drupal 6).

While I was in there I tinkered with your use of variable_get(). From the #description of user_register_notify_mailto, it looks like you're expecting the $default argument to be used if the variable fetched is blank (as opposed to not set). It's entirely possible to run variable_get($var, $default) and get a result of '' (empty string) instead of $default, if the variable exists and is set to ''. With that in mind, instead of calling variable_get('user_register_notify_mailto', variable_get('site_mail', ini_get('sendmail_from'))), I've switched it to a simple variable_get('user_register_notify_mailto', '') followed by a variable_get('site_mail', ini_get('sendmail_from')) if this returns an empty string.

kingandy,

Just so you know a few things.

1) Instead of strtr() you should use drupal_strtr()
2) There are ways to pass though an email address without adding it to the translation and the way you set it up is consider a security issue because drupal doesn't clean up user input until it is used. So you are allow anything typed in the $from field to be used without checking it.
3) variable_get() only returns the seconded item if it isn't set. That mean that if you variable_set() to "" then variable_get() will return "" it will only return the second parm if variable_set has never been run or someone removes the variable from the database table.
4) That looks like a standard patch file. If you had named it .patch I wouldn't have known the diff.

The patch looks good enough. I will clean it up some and it should be in x.x-1.11

Thanks
Robert

1) I can't find any mention of drupal_strtr() anywhere ... and it doesn't seem to be included in my Drupal installation. Is there an API page for it?
2) I did think of pushing for some sort of validation on the email address (on submission of the settings form?), but of course if we're to allow token replacement on the sender address then it's not going to be an email address. Similarly I wouldn't want to use check_plain() as this would interfere with emails in the format "Name <name@email.com>" - I think the chevrons would be replaced with &lt; and &gt;. The thought occurs, however, that the same is true of the 'to' address - there's no email validation or security checking going on on that variable until the point it's entered into watchdog.
3) My point exactly. The #description of user_register_notify_mailto is currently "If you leave this blank, the site email address, %mailto, will be used" - with the code as-is, this won't happen. If the field is left blank, the variable will be set as ""; this will then be retrieved by the variable_get() in the user_register_notify_setup_email() function, and this empty string would then be passed through to drupal_mail(). I'm happy to leave the initial #default_value in the form as a variable_get(variable_get(ini_get())) call, but I do feel very strongly that the retrieved variable in the user_register_notify_setup_email() function should be checked for empty strings.
4) Hooray! I was worried the random filenames at the top would throw things off, but as long as it's usable, I'm happy.

1) http://api.drupal.org/api/function/drupal_substr/5
2) I always have to look them up but there is still a way to check things without using check_plain. I recently have an issue with a security problem in another module so I am more on the alert for things like this and might tighten my own code up some in the next version.
3) I might have to update that text as you are right blank would override that.
4) even with real live patches created by patch those filenames tend to be kinda random sometimes as everyone else a diff way they work on patching.

Thanks
Robert

1) Hmm ... that's a drupal equivalent for substr(). I'm using strtr(), which substitutes strings for other strings (using the same key=>replacement format as t() - in fact t() passes its params through to strtr after processing, which is how I found the function in the first place).

3) For what it's worth, I kind of like the behaviour as described. People can always "reset" the config options if they can't remember their site mail, but it's nice to have the ability to reset just one field (in this case, by blanking it).

Thanks for your patience, and sorry for being such a pain :)

I never mind when someone is submitting code and helping out.

3) I need to look over how it works a little closer and make sure it does what it says it does.

Thanks
Robert

I completely forgot about this. Need to spend some time getting this in the module.

Thanks
Robert