Convert mail variables to cmi

+++ b/core/modules/system/config/system.mail.ymlundefined
@@ -0,0 +1,2 @@
+system:
+  'default-system': 'Drupal\Core\Mail\PhpMail'

default-system looks like duplication. Can't we use default?

+++ b/core/modules/system/lib/Drupal/system/Tests/Mail/HtmlToTextTest.phpundefined
@@ -349,7 +349,7 @@ public function testVeryLongLineWrap() {
-    $eol = variable_get('mail_line_endings', MAIL_LINE_ENDINGS);

MAIL_LINE_ENDINGS should be in the.yml file

+++ b/core/modules/system/lib/Drupal/system/Tests/Mail/HtmlToTextTest.phpundefined
@@ -349,7 +349,7 @@ public function testVeryLongLineWrap() {
+    $eol = config('system.mail')->get('line_endings') ?: MAIL_LINE_ENDINGS;

That way we don't need to verify it exists...

We need to place the yaml config for the line break with double quotes otherwise it'll be escaped.
Also added upgrade on install.

-define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER_SOFTWARE'], 'Win32') !== FALSE ? "\r\n" : "\n");
+if (isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER_SOFTWARE'], 'Win32') !== FALSE) {
+  config('system.mail')
+    ->set('line_endings', '\r\n')
+    ->save();
+}

Surely we shouldn't be calling config()->save() outside of a function, as it would happen on every request? However, I am not sure how we would handle a default in CMI that changes depending on platform.

If it depends on the platform it probably belongs more to the state system than the config system.

#1798920: Change mail_line_endings into config. was open as a duplicate of converting the mail_line_endings variable. Merging the two different approaches as a different option for converting these two vars.

fixed some mistakes

+++ b/core/includes/mail.incundefined
@@ -262,7 +262,7 @@ function drupal_mail_system($module, $key) {
+  $configuration = config('system.mail')->get('system');

Shouldn't this be system.default?

+++ b/core/modules/system/config/system.mail.ymlundefined
@@ -0,0 +1,4 @@
+line_endings:
+  ¶
\ No newline at end of file

This needs some cleaning

+++ b/core/modules/system/lib/Drupal/system/Tests/Mail/HtmlToTextTest.phpundefined
@@ -349,7 +349,7 @@ public function testVeryLongLineWrap() {
+    $eol = config('system.mail')->get('line_endings') ?: MAIL_LINE_ENDINGS;

This in fact should be state.

1) You initialise this with an empty config in the .yml file
2) This means you will call MAIL_LINE_ENDINGS (good)
3) You never set the "correct" value in the .yml file (not good). SO your yml file is useless.

4) When you would save this in the yml file and move the file to a different OS your mails will break. This is OS dependant. Perfect state material.

+++ b/core/modules/system/lib/Drupal/system/Tests/Mail/MailTest.phpundefined
@@ -42,7 +42,7 @@ function setUp() {
+    config('system.mail')->set('system', array('default' => 'Drupal\system\Tests\Mail\MailTest'))->save();

Can't we do set('system.default' => '...'); (just wondering)

Looking through drupal_mail_system, the option is there to override the default mailer with other keys in the config. If no other keys are set then it looks like it should fall back to system.default.

Other than that I have made a few amendments to the files changed in accordance to #10. It does seem that state would be a better place to store the line endings (closer to the variable system insofar as it resides on the OS and is not ported). This means all instances of line_endings drawn from config can be instead drawn from state.

After discussing in IRC perhaps not putting line endings in any form of storage would be more appropriate.

Due to moving between OS as explained in #10 storing in state just seems like an unnecessary step when the MAIL_LINE_ENDINGS is already defined.

It also came up in IRC that some contrib modules may make use of the line endings variable in Drupal 7. Would it be sensible to assume that it is acceptable to delete the variable and require contrib to find their own workarounds.

#12: 1821420-mail_variables_cmi-drupal8-12.patch queued for re-testing.

updating patch in #14

I'm going to rtbc this. I let catch decide (or others if they see this patch being green) about the removal of the line ending setting.

Hm. That setting was introduced in #239825: drupal_mail(), CRLF, and Windows and according to #18 there, it's required in order to get things working on certain environments. Now whether that's a bug with the detection algorithm or whether that's the very use case for why the setting's there is unclear, but it seems like we need to preserve that.

I suppose after reading that issue, an option could be to amend the define to:

define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || isset($_SERVER['windir']) || strpos($_SERVER['SERVER_SOFTWARE'], 'Microsoft-IIS') || strpos($_SERVER['SERVER_SOFTWARE'], 'Win32') !== FALSE ? "\r\n" : "\n");

Is this too simplistic to work? Is this getting too convoluted with huge numbers of specific lines required to cover our bases with other systems?

We just have to convert this variable to the state system.

Updated MAIL_LINE_ENDINGS to state system

+++ b/core/includes/mail.incundefined
@@ -6,13 +6,6 @@
-define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER_SOFTWARE'], 'Win32') !== FALSE ? "\r\n" : "\n");

You should leave this here. And when you check for line endings you first check if something is set in state. If nothing is set, use the default. It's ok to have noting set into state in most use cases as we fallback on this default. If people want to override it they have to set it in their own code.

So you need to set the state on installation.

Makes sense?

Oh crap you could previously override this stuf in settings.php...
"$conf['mail_line_endings'] will override this setting."

*sigh*

Someone else need to give some input I'm out of ideas.

I guess #22 makes sense as we need default.

Reg #23 We don't want to make this as configuration(?). Its going to be state system. So there is noway this can be overridden from settings.php in D8.

adding yml file

I too would have thought it was to be a state system. Line endings aren't suddenly going to change any differently to the system OS suddenly changing.

The only way I envisage change is if a website is migrated from say Windows to Linux. If filesystem and database is moved and line endings have explicitly been set then it could create issues. This can't happen very often but perhaps should be remembered.

I disagree with making this state. It is not computed, internal information, it is something that is explicitly configurable.

Instead of making it config, we could also do something like #1833516: Add a new top-level global for settings.php - move things out of $conf, making settings.php the only way to override it.

Which means that we should remove the upgrade path, update the documentation (not remove it) and create a change notice once it is in.

Ok Berdir, I guess, we need to wait to get #1833516: Add a new top-level global for settings.php - move things out of $conf committed.

That issue went in, so back to needs work with this.

Updating with settings().

+++ b/core/lib/Drupal/Core/Mail/PhpMail.phpundefined
@@ -59,13 +59,12 @@ public function mail(array $message) {
-    // line-ending format appropriate for your system. If you need to
-    // override this, adjust $conf['mail_line_endings'] in settings.php.
+    // line-ending format appropriate for your system.

I think we should keep that sentence?

+++ b/core/modules/system/lib/Drupal/system/Tests/Mail/HtmlToTextTest.phpundefined
@@ -349,7 +349,7 @@ public function testVeryLongLineWrap() {
-    $eol = variable_get('mail_line_endings', MAIL_LINE_ENDINGS);
+    $eol = settings()->get('mail_line_endings') ?: MAIL_LINE_ENDINGS;

settings()->get() has support for a default value, so you don't need a ?: trick.

+++ b/core/modules/system/system.installundefined
@@ -515,6 +515,7 @@ function system_install() {
   state()->set('system.cron_key', $cron_key);
+

Unecessary newline.

Thanks Berdir, all fixed.

+++ b/core/modules/system/lib/Drupal/system/Tests/Mail/HtmlToTextTest.phpundefined
@@ -349,7 +349,7 @@ public function testVeryLongLineWrap() {
     // This awkward construct comes from includes/mail.inc lines 8-13.
-    $eol = variable_get('mail_line_endings', MAIL_LINE_ENDINGS);
+    $eol = settings()->get('mail_line_endings', MAIL_LINE_ENDINGS);
     // We must use strlen() rather than drupal_strlen() in order to count
     // octets rather than characters.
     $line_length_limit = 1000 - drupal_strlen($eol);

Those are interesting comments ;)

First it's not awkward anymore (it's just a constant, defining the constant is ugly) and b) it's not in that file even less those lines :)

While not really related, I suggest we remove that pointless comment. We're never going to touch these lines of code again anyway..

Also, it's interesting how the one below says "We need to use strlen() and not drupal_strlen()" and then uses drupal_strlen() but that is definitely not our problem here :)

+++ b/core/includes/mail.incundefined
@@ -260,13 +260,15 @@ function drupal_mail($module, $key, $to, $langcode, $params = array(), $from = N
+ *
+ * @throws Exception
...
 function drupal_mail_system($module, $key) {

If we add documentation then let's do it correct. @throws \Exception.

The whole function should probably move into the container and use services but I guess we can do that in a follow-up issue..

Tidied up the documentation as suggested. That $line_length_limit variable was totally unused, so I removed that and its attending comment. Maybe that's a little out of scope but I just couldn't let it stand :)

+++ b/core/includes/mail.incundefined
@@ -217,13 +217,13 @@ function drupal_mail($module, $key, $to, $langcode, $params = array(), $from = N
  * The selection of a particular implementation is controlled via the variable
- * 'mail_system', which is a keyed array.  The default implementation
- * is the class whose name is the value of 'default-system' key. A more specific

Hm. Didn't see this before, but this still talks about a 'variable'.

Asked in IRC if there is a standard for this, the answer that I got was "i use 'config.file.name:config.key', and usually refer to it as a config path" but might not be the official way of doing so.

+++ b/core/modules/system/lib/Drupal/system/Tests/Mail/MailTest.phpundefined
@@ -42,7 +42,7 @@ function setUp() {
     // Set MailTestCase (i.e. this class) as the SMTP library
-    variable_set('mail_system', array('default-system' => 'Drupal\system\Tests\Mail\MailTest'));
+    config('system.mail')->set('system.default', 'Drupal\system\Tests\Mail\MailTest')->save();

Looks like we only have test coverage for the default fallback configuration. So the tests alone can't guarantee that it is still possible to have specific configurations although there is no reason that it shouldn't be. Not necessary here, but we probably want to improve the test coverage if we convert this to a plugin (see below)

As suggested, we might want to open a follow-up issue and convert this into container services. Possibly a factory that receives the config object as an injected service. I guess it might even make sense to make this a plugin, as e.g. the mail_system module in contrib provides a UI for this. Another thing to consider is splitting up the formatting and sending into two different plugins. That's a rather frequent use case that is supported in a crazy way by mail_system (Generating a class that calls two different implementations on the fly and then using that). Someone up for creating an issue? :)

Amending comment change in #36 in this patch and created new issue for plugin changes at #1889644: Convert drupal_mail_system() to a MailFactory service to allow more flexible replacements. Feel free to update summary.

+++ b/core/modules/system/system.installundefined
@@ -2195,6 +2195,17 @@ function system_update_8045() {
+  update_variables_to_config('system.mail', array(
+    'mail_system' => 'system',

Hm, this won't work.

We have renamed the key within that variable, so we need to do it manually here. Which means variable_get() to get the old value and check if there is a non-default value (use NULL als default value and make sure it's not that), if so, change the key of default-system and use config()->set()->save() to set it.

That or just revert the key rename. Which might even make more sense, now that I think about it. Because 'default-system' isn't about system.module, it's about the default mail system. So if we want to rename it, default would be the better key.

We should also add upgrade tests, see other issues for examples.

Thanks @Berdir, Updated variable name and hope update_N would be fine as well.

Adding missing yml file.

#40: 1821420-mail_variables_cmi-drupal8-40.patch queued for re-testing.

That or just revert the key rename. Which might even make more sense, now that I think about it. Because 'default-system' isn't about system.module, it's about the default mail system. So if we want to rename it, default would be the better key.

seems there is a assumption that system.mail.system is always an array. So instead of system.mail.system.default, tried system.mail.default and failed. So now trying with system.mail.interface.default to make naming more specific. so basically this is what we have in #37 with name change.

+++ b/core/modules/system/lib/Drupal/system/Tests/Mail/MailTest.phpundefined
@@ -42,7 +42,7 @@ function setUp() {
 
     // Set MailTestCase (i.e. this class) as the SMTP library
-    variable_set('mail_system', array('default-system' => 'Drupal\system\Tests\Mail\MailTest'));
+    config('system.mail')->set('interface.default', 'Drupal\system\Tests\Mail\MailTest')->save();

This is nice :)

+++ b/core/modules/system/system.installundefined
@@ -2214,6 +2214,17 @@ function system_update_8046() {
+  update_variables_to_config('system.mail', array(
+    'mail_system' => 'interface.default',
+  ));

Hm. I still don't think this is correct. It will add all values as an sub array of the default value.

Let's add tests for it. It shouldn't take longer than a manual test.

Thanks for the review @Berdir. I have updated update_N to work convert new variable key to config system.

#46: 1821420-mail_variables_cmi-drupal8-46.patch queued for re-testing.

Hm. It occurred to me that we actually might not want to convert the old value at all.

The reason is relatively simple. Whatever the old value contained, it is no longer a valid class name. So just blindly taking over the old value will mean it's broken.

So, we should probably deliberately ignore the old value of the variable and maybe just display a message to inform users in case they had a customized value..

Will check with some of the CMI experts what they think :)

Ok, discussed with @alexpott, I think the following will work:

If there is a customized value of that variable, loop over the values and and apply the following rules (note that you need to use update_variable_get():
- If the key is default-system, change to default
- If the value is the old default class, change it to the new PSR-0 classname.
- If not, keep the previous value and add it to a list (key and value)

You will need to do this manually, update_variables_to_config() doesn't work for this.

Then, if the list of unconverted class names is not empty), display a warning message and list the unchanged values, something like "The following mail backends need to be re-configured: @list".

This will need tests that set a variable that has the default value for default-system and something custom for another key. Then assert that the value has been correctly converted.

Attached updated upgrade path as specified in #51, and a new test to make sure that the default mail backend is upgraded, and that a custom one is preserved.

+++ b/core/modules/system/system.installundefined
@@ -2214,6 +2214,39 @@ function system_update_8046() {
+  // Warn about non-core classes which may need updated.
+  drupal_set_message('The following mail backends need to be re-configured: @list', implode(', ', $old_mail_system_settings));

This needs to check if there actually is a message to print.

+++ b/core/modules/system/system.installundefined
@@ -2214,6 +2214,39 @@ function system_update_8046() {
+  // Save the updated variable, and let update_variables_to_config convert it.
+  variable_set('mail_system', $new_mail_system_settings);

And this needs to use update_variable_set().

To check for the warning message, I had to break up the performUpgrade() method so that the check for pending updates after upgrade is discrete from the upgrade itself.

Should we log this "reconfigure mail backends" message into watchdog as well/instead? Is the wording right, should we be more specific?

A way to simplify the message check would be to add a method call that in the default implementation does nothing and gives subclasses the possibility to intercept and do some checks.

+++ b/core/modules/system/system.installundefined
@@ -2234,16 +2236,18 @@ function system_update_8047() {
+    // Warn about non-core classes which may need to be updated.
+    drupal_set_message(t('The following mail backends need to be re-configured: @list', array('@list' => implode(', ', $old_mail_system_settings))), 'warning');

This is still displayed even if there was no class that needs to be re-configured I think.

Re-rolling with current code base with check for @list to check message display.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/MailUpgradePathTest.phpundefined
@@ -31,11 +31,11 @@ public function setUp() {
   function testMailSystemUpgrade() {
-    variable_set('mail_system', array(
-      'default-system' => 'DefaultMailSystem',
+    $mail_interface_defaults = array(
+      'default' => 'Drupal\Core\Mail\PhpMail',
       'maillog' => 'MaillogMailSystem',
-    ));
-
+    );
+    config('system.mail')->set('interface', $mail_interface_defaults)->save();
     // Perform the upgrade without redirecting to check for pending updates,

This is testing an upgrade, so the value needs to be set in D7 (ie, with variable_set) before the upgrade occurs. That's why the test is failing on config().

Yes, it should be added to a dump file, see e.g. core/modules/system/tests/upgrade/drupal-7.system.database.php. Then that file can be specified in the setUp() of the test.

thanks @cam8001 and @Berdir. Updated test case.

inserting instead of updating this variable.

+++ b/core/includes/mail.incundefined
@@ -231,7 +231,7 @@ function drupal_mail($module, $key, $to, $langcode, $params = array(), $from = N
+ *   'default' => 'Drupal\Core\Mail\PhpMail',

Is this still default and not the new "interface" key?

We had mail_system[default] and now it is system.mail.interface.default. So still we have default and other interfaces under system.mail.interface.

Re-rolling with latest code base.

#66: 1821420-mail_variables_cmi-66.patch queued for re-testing.

Re-testing

#66: 1821420-mail_variables_cmi-66.patch queued for re-testing.

Re-rolling...

Close :)

+++ b/core/includes/mail.incundefined
@@ -231,7 +231,7 @@ function drupal_mail($module, $key, $to, $langcode, $params = array(), $from = N
- *   'default-system' => 'Drupal\Core\Mail\PhpMail',
+ *   'default' => 'Drupal\Core\Mail\PhpMail',
  *   'user' => 'DevelMailLog',

@@ -241,7 +241,7 @@ function drupal_mail($module, $key, $to, $langcode, $params = array(), $from = N
- *   'default-system' => 'Drupal\Core\Mail\PhpMail',
+ *   'default' => 'Drupal\Core\Mail\PhpMail',
  *   'user' => 'DevelMailLog',
  *   'contact_page_autoreply' => 'DrupalDevNullMailSend',

Can we open a follow-up to fix those examples? (Those are not PSR-0 classes, should be updated with how it's really called now in devel/ technically correct examples)

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -854,7 +854,7 @@ protected function setUp() {
-    variable_set('mail_system', array('default-system' => 'Drupal\Core\Mail\VariableLog'));
+    config('system.mail')->set('interface.default', 'Drupal\Core\Mail\VariableLog')->save();

That should have been renamed when it was converted to use state and not a variable. Fix that in the same follow-up as the example code or a separate one? Not a problem in this patch.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/MailUpgradePathTest.phpundefined
@@ -0,0 +1,52 @@
+ * Definition of Drupal\system\Tests\Upgrade\MailUpgradePathTest.

Nitpick: Should be Contains \Drupal\...

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UpgradePathTestBase.phpundefined
@@ -258,7 +263,22 @@ protected function performUpgrade($register_errors = TRUE) {
       // don't process.
       throw new Exception('Errors during update process.');
     }
+    if ($check_pending_updates) {
+      return $this->checkPendingUpdates();

I'd still prefer if this would instead be done using an internal method that is called here and doesn't do anything by default but can be overridden by test classes. Then we don't need to alter the method arguments/docblock.

+++ b/core/modules/system/system.installundefined
@@ -2142,6 +2142,45 @@ function system_update_8049() {
+    // Save the updated variable, and let update_variables_to_config convert it.
+    if ($new_mail_system_settings) {
+      update_variable_set('mail_system', $new_mail_system_settings);
+      update_variables_to_config('system.mail', array(
+        'mail_system' => 'interface',
+      ));

If we only invoke update_variables_to_config() conditionally, don't we end up with no configuration at all in case there is no override? Can we maybe extend an existing upgrade test to verify that we have the default configuration?

Thanks @Berdir,

#73
1. Created an issue for interface changes #1928440: Rename incorrectly named MailInterface implementations
2. Fixed Drupal\system\Tests\Upgrade\ to \Drupal\system\Tests\Upgrade\
3. removed $check_pending_updates

Todo:
1. Exted MailUpgradePathTest to verify default config.

Here's what I meant with the method.

While looking at the code, also noticed/fixed the following:
- use format_string() in update functions, t() is tricky...
- Move the update_variables_to_config() call out of the conditional conversion, this always needs to run. Also added test coverage (confirmed that it did fail without the fix but a test-only patch is a bit tricky)
- Some minor adjustments with dot's and assert methods.

#76: mail-cmi-1821420-76.patch queued for re-testing.

+++ b/core/modules/system/tests/upgrade/drupal-7.system.database.phpundefined
@@ -119,6 +119,10 @@
+  ->values(array(
+  'name' => 'mail_system',
+  'value' => 'a:2:{s:14:"default-system";s:17:"DefaultMailSystem";s:7:"maillog";s:17:"MaillogMailSystem";}',
+))
 ->execute();

Intendation is incorrect.

But that is seriously minor. Please get this in.

patch in #79 is clean and good to go.

Committed/pushed to 8.x, thanks!