Files: 
CommentFileSizeAuthor
#79 mail-cmi-1821420-79.patch20.63 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 52,546 pass(es).
[ View ]
#79 interdiff.txt6.94 KBAlbert Volkman
#76 mail-cmi-1821420-76.patch14.4 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 52,312 pass(es).
[ View ]
#76 mail-cmi-1821420-76-interdiff.txt6.83 KBBerdir
#74 1821420-mail_variables_cmi-74.patch14.85 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,252 pass(es).
[ View ]
#74 1821420-diff-72-74.txt3.22 KBvijaycs85
#72 1821420-mail_variables_cmi-72.patch14.33 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 51,270 pass(es).
[ View ]
#66 1821420-mail_variables_cmi-66.patch14.28 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1821420-mail_variables_cmi-66.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#64 1821420-mail_variables_cmi-64.patch14.28 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 48,730 pass(es).
[ View ]
#64 1821420-diff-62-64.txt937 bytesvijaycs85
#62 1821420-mail_variables_cmi-62.patch14.33 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 48,679 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#62 1821420-diff-57-62.txt1.87 KBvijaycs85
#58 1821420-diff-55-57.txt1.94 KBvijaycs85
#57 1821420-mail_variables_cmi_upgrade_path-test-57.patch13.82 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 48,633 pass(es), 3 fail(s), and 6 exception(s).
[ View ]
#55 1821420-mail_variables_cmi_upgrade_path-test-55.patch13.7 KBcam8001
PASSED: [[SimpleTest]]: [MySQL] 49,323 pass(es).
[ View ]
#55 interdiff.txt4.69 KBcam8001
#52 1821420-mail_variables_cmi_upgrade_path-test.patch11.5 KBcam8001
FAILED: [[SimpleTest]]: [MySQL] 48,856 pass(es), 25 fail(s), and 49 exception(s).
[ View ]
#46 1821420-mail_variables_cmi-drupal8-46.patch9.06 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 48,777 pass(es), 24 fail(s), and 24 exception(s).
[ View ]
#46 1821420-44-46-diff.txt814 bytesvijaycs85
#44 1821420-mail_variables_cmi-drupal8-44.patch8.67 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 49,293 pass(es).
[ View ]
#44 1821420-40-44-diff.txt3.2 KBvijaycs85
#40 1821420-mail_variables_cmi-drupal8-40.patch8.62 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 49,220 pass(es), 52 fail(s), and 54 exception(s).
[ View ]
#39 1821420-mail_variables_cmi-drupal8-39.patch8.37 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 48,693 pass(es), 78 fail(s), and 81 exception(s).
[ View ]
#37 1821420-mail_variables_cmi-drupal8-37.patch8.66 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 50,822 pass(es).
[ View ]
#37 1821420-35-37.txt773 bytesvijaycs85
#35 1821420-mail_variables_cmi-drupal8-35.patch8.51 KBcam8001
PASSED: [[SimpleTest]]: [MySQL] 50,657 pass(es).
[ View ]
#35 interdiff.txt1.45 KBcam8001
#33 1821420-mail_variables_cmi-drupal8-33.patch8.3 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 50,558 pass(es).
[ View ]
#33 1821420-31-33-diff.txt3.09 KBvijaycs85
#31 1821420-mail_variables_cmi-drupal8-31.patch8.49 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 50,571 pass(es).
[ View ]
#26 1821420-mail_variables_cmi-drupal8-26.patch8.54 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 49,993 pass(es).
[ View ]
#24 1821420-mail_variables_cmi-drupal8-24.patch8.27 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 49,292 pass(es), 20 fail(s), and 20 exception(s).
[ View ]
#24 1821420-21-24-diff.txt3.12 KBvijaycs85
#21 1821420-mail_variables_cmi-drupal8-21.patch8.85 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 49,664 pass(es).
[ View ]
#15 1821420-mail_variables_cmi-drupal8-15.patch7.61 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 49,427 pass(es).
[ View ]
#12 1821420-mail_variables_cmi-drupal8-12.patch7.34 KBtyphonius
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1821420-mail_variables_cmi-drupal8-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 1821420-mail_variables_cmi-drupal8-11.patch7.28 KBtyphonius
FAILED: [[SimpleTest]]: [MySQL] 48,684 pass(es), 16 fail(s), and 48 exception(s).
[ View ]
#9 1821420-mail_variables_cmi-drupal8-9.patch7.58 KBACF
PASSED: [[SimpleTest]]: [MySQL] 49,279 pass(es).
[ View ]
#7 1821420-mail_variables_cmi-drupal8-7.patch8 KBACF
FAILED: [[SimpleTest]]: [MySQL] 48,926 pass(es), 31 fail(s), and 10 exception(s).
[ View ]
#4 mail_variables_cmi-1821420-4.patch7.65 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 46,926 pass(es).
[ View ]
#2 mail_variables_cmi-1821420-2.patch7.04 KBAlbert Volkman
FAILED: [[SimpleTest]]: [MySQL] 46,427 pass(es), 82 fail(s), and 0 exception(s).
[ View ]
#2 interdiff.txt6.66 KBAlbert Volkman
mail_variables_cmi.patch4.34 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 46,247 pass(es).
[ View ]

Comments

aspilicious’s picture

Status:Needs review» Needs work
+++ 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...

Albert Volkman’s picture

Status:Needs work» Needs review
StatusFileSize
new6.66 KB
new7.04 KB
FAILED: [[SimpleTest]]: [MySQL] 46,427 pass(es), 82 fail(s), and 0 exception(s).
[ View ]

Thanks for the review @aspilicious!

Status:Needs review» Needs work

The last submitted patch, mail_variables_cmi-1821420-2.patch, failed testing.

pcambra’s picture

Status:Needs work» Needs review
StatusFileSize
new7.65 KB
PASSED: [[SimpleTest]]: [MySQL] 46,926 pass(es).
[ View ]

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

longwave’s picture

-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.

aspilicious’s picture

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

ACF’s picture

StatusFileSize
new8 KB
FAILED: [[SimpleTest]]: [MySQL] 48,926 pass(es), 31 fail(s), and 10 exception(s).
[ View ]

#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.

Status:Needs review» Needs work

The last submitted patch, 1821420-mail_variables_cmi-drupal8-7.patch, failed testing.

ACF’s picture

Status:Needs work» Needs review
StatusFileSize
new7.58 KB
PASSED: [[SimpleTest]]: [MySQL] 49,279 pass(es).
[ View ]

fixed some mistakes

aspilicious’s picture

Status:Needs review» Needs work
+++ 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)

typhonius’s picture

Status:Needs work» Needs review
StatusFileSize
new7.28 KB
FAILED: [[SimpleTest]]: [MySQL] 48,684 pass(es), 16 fail(s), and 48 exception(s).
[ View ]

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.

typhonius’s picture

StatusFileSize
new7.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1821420-mail_variables_cmi-drupal8-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

aspilicious’s picture

Status:Needs review» Needs work
Issue tags:+Configuration system

The last submitted patch, 1821420-mail_variables_cmi-drupal8-12.patch, failed testing.

vijaycs85’s picture

StatusFileSize
new7.61 KB
PASSED: [[SimpleTest]]: [MySQL] 49,427 pass(es).
[ View ]

updating patch in #14

vijaycs85’s picture

Status:Needs work» Needs review
aspilicious’s picture

Status:Needs review» Reviewed & tested by the community

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.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

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.

typhonius’s picture

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

<?php
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?

aspilicious’s picture

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

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new8.85 KB
PASSED: [[SimpleTest]]: [MySQL] 49,664 pass(es).
[ View ]

Updated MAIL_LINE_ENDINGS to state system

aspilicious’s picture

Status:Needs review» Needs work
+++ 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?

aspilicious’s picture

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.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new3.12 KB
new8.27 KB
FAILED: [[SimpleTest]]: [MySQL] 49,292 pass(es), 20 fail(s), and 20 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1821420-mail_variables_cmi-drupal8-24.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new8.54 KB
PASSED: [[SimpleTest]]: [MySQL] 49,993 pass(es).
[ View ]

adding yml file

typhonius’s picture

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.

Berdir’s picture

Status:Needs review» Needs work

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.

vijaycs85’s picture

Status:Needs work» Postponed

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.

Berdir’s picture

Status:Postponed» Needs work

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

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new8.49 KB
PASSED: [[SimpleTest]]: [MySQL] 50,571 pass(es).
[ View ]

Updating with settings().

Berdir’s picture

Status:Needs review» Needs work
+++ 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.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new3.09 KB
new8.3 KB
PASSED: [[SimpleTest]]: [MySQL] 50,558 pass(es).
[ View ]

Thanks Berdir, all fixed.

Berdir’s picture

+++ 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..

cam8001’s picture

StatusFileSize
new1.45 KB
new8.51 KB
PASSED: [[SimpleTest]]: [MySQL] 50,657 pass(es).
[ View ]

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 :)

Berdir’s picture

Status:Needs review» Needs work
+++ 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? :)

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new773 bytes
new8.66 KB
PASSED: [[SimpleTest]]: [MySQL] 50,822 pass(es).
[ View ]

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.

Berdir’s picture

Status:Needs review» Needs work
+++ 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.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new8.37 KB
FAILED: [[SimpleTest]]: [MySQL] 48,693 pass(es), 78 fail(s), and 81 exception(s).
[ View ]

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

vijaycs85’s picture

StatusFileSize
new8.62 KB
FAILED: [[SimpleTest]]: [MySQL] 49,220 pass(es), 52 fail(s), and 54 exception(s).
[ View ]

Adding missing yml file.

Status:Needs review» Needs work
Issue tags:-Configuration system

The last submitted patch, 1821420-mail_variables_cmi-drupal8-40.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Configuration system

The last submitted patch, 1821420-mail_variables_cmi-drupal8-40.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new3.2 KB
new8.67 KB
PASSED: [[SimpleTest]]: [MySQL] 49,293 pass(es).
[ View ]

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.

Berdir’s picture

+++ 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.

vijaycs85’s picture

StatusFileSize
new814 bytes
new9.06 KB
FAILED: [[SimpleTest]]: [MySQL] 48,777 pass(es), 24 fail(s), and 24 exception(s).
[ View ]

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

Status:Needs review» Needs work
Issue tags:-Configuration system

The last submitted patch, 1821420-mail_variables_cmi-drupal8-46.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Configuration system

The last submitted patch, 1821420-mail_variables_cmi-drupal8-46.patch, failed testing.

Berdir’s picture

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 :)

Berdir’s picture

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.

cam8001’s picture

Component:menu system» mail system
Status:Needs work» Needs review
StatusFileSize
new11.5 KB
FAILED: [[SimpleTest]]: [MySQL] 48,856 pass(es), 25 fail(s), and 49 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1821420-mail_variables_cmi_upgrade_path-test.patch, failed testing.

Berdir’s picture

+++ 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().

cam8001’s picture

Status:Needs work» Needs review
StatusFileSize
new4.69 KB
new13.7 KB
PASSED: [[SimpleTest]]: [MySQL] 49,323 pass(es).
[ View ]

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?

Berdir’s picture

Status:Needs review» Needs work

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.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new13.82 KB
FAILED: [[SimpleTest]]: [MySQL] 48,633 pass(es), 3 fail(s), and 6 exception(s).
[ View ]

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

vijaycs85’s picture

StatusFileSize
new1.94 KB

Status:Needs review» Needs work

The last submitted patch, 1821420-mail_variables_cmi_upgrade_path-test-57.patch, failed testing.

cam8001’s picture

+++ 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().

Berdir’s picture

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.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new1.87 KB
new14.33 KB
FAILED: [[SimpleTest]]: [MySQL] 48,679 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

thanks @cam8001 and @Berdir. Updated test case.

Status:Needs review» Needs work

The last submitted patch, 1821420-mail_variables_cmi-62.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new937 bytes
new14.28 KB
PASSED: [[SimpleTest]]: [MySQL] 48,730 pass(es).
[ View ]

inserting instead of updating this variable.

aspilicious’s picture

+++ 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?

vijaycs85’s picture

StatusFileSize
new14.28 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1821420-mail_variables_cmi-66.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Needs work
Issue tags:-Configuration system

The last submitted patch, 1821420-mail_variables_cmi-66.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
Issue tags:+Configuration system

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

vijaycs85’s picture

Re-testing

vijaycs85’s picture

Issue tags:-Configuration system

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

Status:Needs review» Needs work
Issue tags:+Configuration system

The last submitted patch, 1821420-mail_variables_cmi-66.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new14.33 KB
PASSED: [[SimpleTest]]: [MySQL] 51,270 pass(es).
[ View ]

Re-rolling...

Berdir’s picture

Status:Needs review» Needs work

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?

vijaycs85’s picture

StatusFileSize
new3.22 KB
new14.85 KB
PASSED: [[SimpleTest]]: [MySQL] 52,252 pass(es).
[ View ]

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.

vijaycs85’s picture

Status:Needs work» Needs review
Berdir’s picture

StatusFileSize
new6.83 KB
new14.4 KB
PASSED: [[SimpleTest]]: [MySQL] 52,312 pass(es).
[ View ]

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.

Berdir’s picture

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

aspilicious’s picture

Status:Needs review» Reviewed & tested by the community
+++ 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.

Albert Volkman’s picture

StatusFileSize
new6.94 KB
new20.63 KB
PASSED: [[SimpleTest]]: [MySQL] 52,546 pass(es).
[ View ]

Here's some code style clean-up, if there's interest.

vijaycs85’s picture

patch in #79 is clean and good to go.

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.