Add Variable module integration.

Hi Deciphered,

Thanks a lot for bringing up this great idea and submitting this very nice patch.

I certainly think this would be an improvement for the module and I have taken a little bit of time to review the patch, test it and dig a bit more into the Variable API.

I would greatly appreciate to have your feedback on the following minor comments noted below:

• How about adding a specific Reroute Email variables group in the variables admin page: Implements hook_variable_group_info.
• For the default value of the variable REROUTE_EMAIL_ADDRESS, could it be initialized to site_mail first, with ini_get('sendmail_from') as a fallback?
  variable_get('site_mail', ini_get('sendmail_from')),

Please find attached to this comment a patch against reroute_email-7.x-1.x at 039c28b which is just a re-roll of the initial patch submitted at #1 with the changes mentioned above.
File attached as: reroute_email-add-variable-module-integration-1964070-2.patch

Please also find attached a merge interdiff, so you could quickly see the changes made to the previous patch (interdiff-7151486-2.txt)

This new patch has been tested and seems to work as expected, but I would greatly appreciate to have your feedback, questions, comments, reviews, suggestions, recommendations, improvements and testing.

Feel free to let me know if you would have any further comments, issues, questions, objections, recommendations, suggestions, testing, reporting or concerns on the attached patch or any other aspects of this ticket in general, I would be glad to provide more information or explain in further details.

Thanks again @Deciphered very much for the great work and to all in advance for your feedback, reviews, testing and reporting.
Cheers!

Hi Deciphered,

Thanks very much for your prompt and kind follow-up on this new feature.

Since there were no further updates on this ticket, objections, comments or replies, I went ahead and got it committed against the 7.x-1.x branch at fa9f4b5.

I allowed myself to mark this issue as fixed for now, but feel free to re-open it, or post a new ticket, at any time if you have any further objections with the solution suggested at #2 (we would surely be happy to hear your feedback).

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on any of the related commits or this ticket in general, I would be glad to provide more information or explain in more details.

Thanks again to everyone for your help, reviews, feedback and comments on this issue.
Cheers!

While reviewing the recent changes to #1571500: Separate the recipient (list) from the address whitelist. Add domain whitelist., I noticed that although the requested change to REROUTE_EMAIL_ADDRESS (to use the site_mail variable as a default, and ini_get('sendmail_from') as a fallback; see comment #2 above) was implemented in reroute_email.variable.inc, the associated changes were never made in the module file itself, meaning that this is inconsistent at present.

i.e. site_mail is never actually used by the module, despite what reroute_email_variable_info() now declares the default to be.

Because the patch for #1571500 updates the way in which this default is defined and used in the module, I've gone ahead and implemented the change so that the module now uses variable_get('site_mail', ini_get('sendmail_from')).

https://drupal.org/node/1571500#comment-7480002

Could you please verify that it was not intentional that this change was only made to the declaration in reroute_email_variable_info()?

Hi @jweowu,

Thanks again very much for your kind feedback on this and sorry for this late follow up.

Indeed, I think I see what you mean:
We should probably be changing the line 73 in reroute_email.module:

<?php
    if (!variable_get(REROUTE_EMAIL_ADDRESS, '')) {
      // If email address not in settings, then do nothing.
      return;
    }
?>

I think this has something to do with #1722572: What happens if it's enabled with no email added?, because it seems to be related with the default init of the reroute_email_address or module's handling when config field is left empty.
Since the other issue is still active and I would like to get back to it next after fixing a bit more of the tests, I am going to cross post your comment in the other issue so we could keep following up with this there.
 
Feel free to let me know if you would have any further comments, issues, questions, objections, recommendations, suggestions, testing, reporting or concerns on any aspects of this ticket or related changes, I would be glad to provide more information or explain in further details.
 
Thanks again very much for your feedback, testing, reviews and reporting.
Cheers!

Mistakenly updated issue status.

Back to closed (fixed).

Hi @jweowu,

Sorry for this confusing answer at #7, but I think I didn't completely understand what you meant.

While working on #1722572: What happens if it's enabled with no email added? and trying to look at your comment a bit more in detail I think I managed getting a slightly better understanding of the problem you reported and so I went ahead and got the default Reroute Email address changed in all other places in the module (see reroute_email.admin.inc, line 21 and reroute_email.module, line 117).

So basically, the default rerouted_email_address, if not yet configured through the Reroute Email Settings form, would be the Site's Email address, then ini_get('sendmail_from') (If site_mail isn't configured).

In terms of code, this would correspond to changing calls from:

variable_get(REROUTE_EMAIL_ADDRESS, ini_get('sendmail_from'))

to

variable_get(REROUTE_EMAIL_ADDRESS, variable_get('site_mail', ini_get('sendmail_from')))

Additionally, I noticed there wasn't any test for this particular limit case, so I allowed myself to add a new test case called RerouteEmailDefaultAddressTestCase which tests default reroute destination email address when it is not configured.

While writing the tests, I found out the call to variable_get in:
variable_get(REROUTE_EMAIL_ADDRESS, variable_get('site_mail', ini_get('sendmail_from')))
replaced in line 117, wouldn't even be executed, because reroute_email_mail_alter would have already returned at line 75:

<?php
    if (!variable_get(REROUTE_EMAIL_ADDRESS, '')) {
      // If email address not in settings, then do nothing.
      return;
    }
?>

if REROUTE_EMAIL_ADDRESS would have happened to be NULL (not set).
In other words, if REROUTE_EMAIL_ADDRESS wasn't set, we wouldn't even get to executing line 117.
This is probably something we would like to get back to in issue #1722572: What happens if it's enabled with no email added?.
 

For now, I went ahead and committed the changes against the 7.x-1.x branch at 3fa7bc5, followed by a minor commit at 6bea0ec to remove a few unnecessary tests in test case class testRerouteDefaultAddress.

The changes were back-ported to the 6.x-1.x branch at f5d0d7d.

Feel free to re-open this issue, or post a new ticket, at any time if you have any further objections with this update/change or any of the related commits (3fa7bc5, 6bea0ec, f5d0d7d - we would surely be happy to hear your feedback).

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on any of these commits or this ticket in general, I would be glad to provide more information or explain in more details.

Thanks again to everyone for your help, reviews, feedback and comments on this issue.
Cheers!

Thanks DYdave; the basic variable_get() changes in 3fa7bc5 look good.

Noted that one of them will never be used, as per your test case results, however I think that #1571500: Separate the recipient (list) from the address whitelist. Add domain whitelist. (which will need to be re-rolled given the recent changes) would make that a non-issue, as in that patch I set define('REROUTE_EMAIL_DEFAULT_ADDRESS', variable_get('site_mail', ini_get('sendmail_from'))); once and use it everywhere in place of those variable_get() calls.

Thanks again @jweowu for your great feedback, kind reviews and help, it is certainly highly appreciated.

For sure, I saw the issue #1571500: Separate the recipient (list) from the address whitelist. Add domain whitelist. got RTBC-ed with a nice and clean patch but I still didn't find enough time to review it in-depth and get back to you with a more complete answer.

Basically, I wanted to catch up and update the tests as much as possible to try to to stabilize for a potential upcoming stable release that could include the Test Email form (which I think is a great new feature and would allow updating project's page, more officially).
Then, I thought I could get back to the patch you suggested for the next release.

Additionally, given all the work that's been done recently to try to get as many of the past changes as possible to be tested in code, one way or another, the patch from #1571500: Separate the recipient (list) from the address whitelist. Add domain whitelist. would have to integrate corresponding changes to tests and provide proper test cases.... but I will have to get back to you later on that.

In any case, thank you very much for your great work, reviews and especially for keeping a friendly eye on this module.
Cheers!

Sure, that seems reasonable at this point. It is a little unfortunate that the whitelist patch is still pending, but c'est la vie. Hopefully we can get it in sometime soon after this next release, to minimise any further re-rolls.