$conf['mail_system'] setting overrides Mail System module [#2841588]

In devel.mail.inc you are adviced to

 * To enable, save a variable in settings.php (or otherwise) whose value
 * can be as simple as:
 *
 * $conf['mail_system'] = array(
 *   'default-system' => 'DevelMailLog',
 *);

This causes an override because the simple fact that variable_get (used by the Mail System module) gets the value from settings.php in stead of the variable table. This also means that calls to mailsystem_set has no effect. (Which is what made my file this report - wasted some time on tracking down why mailsystem_set didn't work in my module.)

I think there should be a warning saying that you should remove this from settings.php before enabling the Mail System module - or maybe just recommend to use the Mail System module to enable this?

Update: The Devel developers suggests in comment 9:

The real solution is for the Mail System module to check this in hook_requirements() and tell the user what to do.

Comment	File	Size	Author
#6	devel-2841588-6.patch	1.16 KB	hansfn
#6	7.x-1.x: PHP 5.3 & MySQL 5.5, D7 3 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

6 January 2017 at 12:58

hansfn created an issue. See original summary.

Comment #2

gnuget

Spanish

CreditAttribution: gnuget as a volunteer and at Agaric commented 6 January 2017 at 22:24

I think this is the way how it should work.

Why you have:

$conf['mail_system'] = array(
   'default-system' => 'DevelMailLog',
);

If you don't want to use it.

Overwrite variables setting them in the settings.php file is a feature not a bug.

Comment #3

hansfn CreditAttribution: hansfn commented 6 January 2017 at 23:32

I just ask for the possible problem to be documented. No where do I claim it is a bug.

When I added the $conf setting to settings.php, before I started using Mail System, I didn't realize it could be a problem later. I'm just that stupid ;-)

Comment #4

gnuget

Spanish

CreditAttribution: gnuget as a volunteer and at Agaric commented 7 January 2017 at 00:56

oh I see, sorry :-)

Comment #5

willzyx CreditAttribution: willzyx commented 7 January 2017 at 09:49

Status:

Active

» Closed (works as designed)

@hansfn thanks for reporting.
I'm closing this issue because, as said by @gnuget, this is absolutely expected; is the way how it should work when you override variables in settings.php.
Feel free to reopen if you want to post a patch for improve the documentation. Thanks!

Comment #6

hansfn CreditAttribution: hansfn commented 8 January 2017 at 19:46

Status:

Closed (works as designed)

» Needs review

File	Size
devel-2841588-6.patch	1.16 KB
7.x-1.x: PHP 5.3 & MySQL 5.5, D7 3 pass

OK, I have no problem writing a patch - attached.

I think you are missing the point about this report. It is not obvious that the Mail System module uses the 'mail_system' variable and that the suggestion in devel.mail.inc will basically disable the module. The Mail System module could have used another variable.

PS! In D8 this doesn't seem to be a problem - the Mail System module works as intended even if the variable is added to settings.php.

Comment #7

willzyx CreditAttribution: willzyx commented 5 February 2017 at 21:01

+++ b/devel.mail.inc
@@ -3,15 +3,24 @@
+ * To enable, either use the Mail System module (and set
+ * DevelMailLog as site-wide MailSystemInterface class), or
...
+ * An added benefit of using the Mail System module is that you
+ * can use another class for formatting.
+ *
+ * PS! Note that adding a variable in settings.php as suggested
+ * above will completely override the Mail System module.

Thanks for the patch!

personally l do not think that the dockblock should contains any reference to the Mail System module
If you want a good inspiration for improve the documentation you can take a look at the 8.x DevelMailLog dockbloc :)

Comment #8

hansfn CreditAttribution: hansfn commented 6 February 2017 at 00:20

If something has unexpected consequences, I think it should be documented. (In Drupal 8 there is no such problems.) Anyway, I guess I have made enough traces in this issue that other people with the same problem will find it with Google. I will not spend anymore time on getting this committed.

Comment #9

salvis

he/his

CreditAttribution: salvis commented 16 February 2017 at 13:28

Status:

Needs review

» Closed (works as designed)

I agree with #7. We should not advertise a third-party module in our docblocks.

I think there should be a warning saying that you should remove this from settings.php before enabling the Mail System module - or maybe just recommend to use the Mail System module to enable this?

The real solution is for the Mail System module to check this in hook_requirements() and tell the user what to do.

Sorry for wasting your time, hansfn.

Comment #10

hansfn CreditAttribution: hansfn commented 16 February 2017 at 17:56

Title:	DevelMailLog - $conf['mail_system'] setting overrides Mail System module	» $conf['mail_system'] setting overrides Mail System module
Project:	Devel	» Mail System
Component:	devel	» Code
Issue summary:	View changes
Status:	Closed (works as designed)	» Active