There is a nice UI for the CVS admin to change the strings used for email messages to CVS users upon application, activation, etc. The strings for these messages are initially defined in cvs.module in the _cvs_get_strings() function and are saved as Drupal variables when changed.

When the status of a user changes, cvs_mail_user() is called to create the e-mail. From this function _cvs_get_strings() is called again to get the strings. Unfortunately, at no time are the Drupal variables, such as cvs_approved_email, checked. So even though the value of the Drupal variable cvs_approved_email is correct, cvs_mail_user() ends up using the hard coded value of the string instead.

Without thinking too hard about this, it seems that if the code in _cvs_get_strings() is changed to look like this:

  $strings['cvs_received_email'] = variable_get('cvs_received_email', t('%account-name,
  Your CVS account request has been received and will be processed as soon as possible.

Kind regards,
Drupal CVS administrator.'));

that we would get the expected behavior.

Can someone please confirm that this is the right way to fix the problem before I create a patch?

CommentFileSizeAuthor
#2 cvslog_mail_user_strings_0.txt1.44 KBaclight

Comments

dww’s picture

dww
we/he/they
commented

Ok, there's definitely a bug here, and I'll agree this code is confusing. However, grep just revealed something important. Every call site that touches the array you get back from _cvs_get_strings() manually does the variable_get() itself, except in cvs_mail_user(). :( So, if you change _cvs_get_strings() to call variable_get() as you propose (an utterly sane solution to this bug), that means you'll have to go change all the rest of the code in the module...

That said, I think core's _user_mail_text() helper is a good model for how this interface should work without being too confusing and error prone. That's basically what you propose. Seems like that reduces possible code duplication the most (all the variable_get() is handled in one place, as is all the strtr(), etc, etc). Unfortunately, that means this patch will be a lot bigger.

The smaller solution to this bug would be to just fix the code in cvs_mail_user() to do the variable_get() and strtr() itself, like all the rest of this module's code.

Frankly, given that I'd like to see this module die in the near future and move over to Version control API, I'd be happy with the smaller, easier patch for this bug, instead of the more complete, long-term solution. I just looked and versioncontrol_account_status.module gets this right, and the equivalent function returns the saved values (if they exist) and only returns the hard-coded defaults if the admin never customized anything...

aclight’s picture

aclight commented
Assigned: Unassigned » aclight
Status: Active » Needs review
StatusFileSize
new cvslog_mail_user_strings_0.txt1.44 KB

I'm with the smaller, easier solution here as well. The attached patch calls variable_get from within cvs_mail_user.

dww’s picture

dww
we/he/they
commented
Title: _cvs_get_strings() should call variable_get » cvs_mail_user() should call variable_get()
Status: Needs review » Fixed

Reviewed, tested, committed to HEAD, backported to DRUPAL-4-7--2, and committed there. I didn't backport to DRUPAL-4-7 since there are no settings there -- it's just all hardcoded. RIP, cvs.module. ;)

Thanks for the fix, aclight!

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)