Convert user account e-mail templates to configuration system

Also tagging D8MI since this affects them.

I started work on this at the DrupalConMunich sprint. Not sure that this is a complete novice task as the install is a bit tricky since we can't use update_variables_to_config() as the user mail variable may or may not exist it the variable table.

Anyhow :) here's the patch.

I wonder if \n works on windows. Di you just exported the mail settings through config->set->save?

@Aspilicious yep I just ran user_update_8005() and exported using the new drush command :)

I think we should be fine here... someone just needs to apply this patch and have a look at the config page. The user mails will have always been sent with \n so it's just about the way it looks on that form.

I don't see a reason why \n would not work.

I can't find the code that is setting the email config settings when saving the form. This probably also means we are lacking tests.

Good point... here's the config->save();

Tag it accordingly. Also bring on the D8MI sprint.

Rerolled against 8.x and changed YAML to follow Drupal's 2 space indentation rule.

This looks great!

I think it would be even better if we kept every email on its own config file, like 'use.mail.cancel_confirm.yml', with 'subject' and 'body' elements each. There are some reasons for this:
- One is performance (loading and maybe localizing only the data you need) and we are likely to need only one or two emails at most for a typical request.
- Then there's metadata. Whatever schema we use for this (which is WIP) it will be easier if we have multiple config objects of 'mail type', that can fit some standard schema (subject, body..) instead of a big file for which we need to define each property.

@sun yep - there's no need for the default values.

I was concerned about the upgrade path. But looking at update_variables_to_config() again this only migrates variables that exist in the variables table. So if they variables have never been set (i.e. they have the default value) they will not be in this table - so there is no need for the defaults to remain in code - I've over complicated it :(

@sun: re the long lines - I generated the YAML file using the now removed _user_mail_text() function... so this what the current Symfony YAML component will do. Going to investigate an upstream patch to allow mutli-line strings to be dumped using the literal style shown here http://michael.f1337.us/2010/03/30/482836205/ - which Symfony's YAML parser already understands.

@sun,
Still, since we are really lagging behind moving stuff into configuration system, we need some 'best practices' to store this kind of data and I think it makes more sense one email per file, that would be kind of 'one object per file'. Note this would be the precedent for all other modules that store emails into the config system.

Okay since we have no tests for the saving of configuration forms I've implemented a generic test and included a partial implementation for the user_admin_settings form.

@xjm has suggested that the windsprint might take up the writing of tests for this patch so I've left the implementation incomplete.

@sun

This issue is about converting stuff from A to B. If there are no tests for this functionality, then there are no tests. If anyone wants to add tests » cool, but separate issue.

I completely agree with this.

It's actually that precedent impact that worries me. I don't want to have 100 additional single config objects just because there are 100 e-mail templates in the system. The grouping/nested keys within the object are sufficient for coping with any kind of additional properties/data that might be needed in the future.

What's wrong with having 100 additional config objects? Do we need to load (and process, and unserialize) a bunch of emails when we only need one?
So this is what I mean when I talk about standardizing stuff in the config system. Is there a reason why we should have bigger but less objects in the config system? Or is there a reason for the opposite?
If there are 100 email config objects in the system, what we need is to build some config wrapper for them, which will be easier if each email is on its own file. But that will make sense only when we have other modules having emails converted to cmi too.

This issue is about converting stuff from A to B. If there are no tests for this functionality, then there are no tests. If anyone wants to add tests » cool, but separate issue.

I disagree...

If I didn't noticed this, this probably would be broken.
The first thing we would do when we noticed this: set to major and add "Needs tests".
I want to prevent that.

Yes this issue is about converting stuff but that doesn't mean we shouldn't write a damn test.
If another issue is about introducing a new subsystem it has to have test too. Even if the issue is not about writing tests.
I had this discussion before in an other configuration issue. I don't want to fight this battle over and over again...

Second, to fully test user input interaction, we'd actually have to use TestBase::generatePermutations() in order to truly verify the expected behavior of a settings form depending on the user input for all fields.

It would be great if we have at least some tests that verifies the settings are *saved*.
I don't get what is wrong with a simple test that verifies it works. It seems like you only see two options
1) no tests
2) full features testsystem that tests *everything* that can happen

Well in these cases I prefer 3
3) add some tests for most common problems. When a bug report arrives extend the test.

So I'm not sure whether the abstraction will really buys us anything... what do you think

I have no real opinion about the abstraction. But it can be usefull to add some basic tests fast.

"Never Touch A Running System"

http://en.wiktionary.org/wiki/never_change_a_running_system
==> 'Don't change something that is working.'

1) We aren't changing anything by adding a test
2) The system was broken with the patch
3) If we should follow that rule we shouldn't convert this and leave variable_get in drupal core

Btw, about changing the status of the issue in #16 back to 'needs review', that was unintended (I just posted simultaneously with @sun) so changing back to RTBC. Having one email per file or not can be very well a follow up issue once we make up our minds about it, which looks more like a generic CMI discussion.

One thing to consider that's in favour of the abstracted tests is this:

The point of the abstraction is to not test user interaction wit the form but to test the submit handler... This wasn't important in the days of system_settings_form() but now it is because we have to write a submit handler for every form...

Subtaging for D8MI.

Regarding the one-vs.-multiple YAML file issue, I'm not really sure which way to fall on that, myself. My guess is it will become more obvious as we work with these settings a bit more in future patches, and I think this is a decent guess at what the right behaviour might be.

The test actually looks quite useful to me. I agree that when a bug is found is the proper time to introduce tests, and these tests aren't locked to any specific form, which makes them really nice to re-use elsewhere. I'm sure there's more that could be done to improve them, but this can be done in subsequent patches.

The one quibble I have with them is:

+++ b/core/modules/system/lib/Drupal/system/Tests/System/SystemConfigFormBase.php
@@ -0,0 +1,80 @@
+    $this->setUpSystemConfigForm();
...
+  abstract function setUpSystemConfigForm();

This seems weird. AFAIK, no other tests make a distinction between setUp of the test and... more setUp of something else? Why can't it all be in setUp?

If we are going to keep it, that setUpSystemConfigForm() function needs PHPdoc.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/SystemConfigFormBase.php
@@ -0,0 +1,80 @@
+ * Full generic test suite for any form that saves data using the configuration
+ * system.
...
+   * Submit the system_config_form and then test the configuration has the
+   * expected values.

(nitpick) First line of PHPdoc comments should be shortened to one sentence, < 80 chars. More details can be added in subsequent paragraphs.

Fixed in commit.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/SystemConfigFormBase.php
@@ -0,0 +1,80 @@
+   * Conatins details for form key, configuration object name, and config key.

"Contains"

Fixed in commit.

+++ b/core/modules/user/user.admin.inc
@@ -441,13 +443,13 @@ function user_admin_settings($form, &$form_state) {
-    '#default_value' => _user_mail_text('register_admin_created_subject', NULL, array(), FALSE),
+    '#default_value' => $mail_config->get('register_admin_created.subject'),

Wow, that transformation is beautiful! :D

+++ b/core/modules/user/user.module
@@ -2124,172 +2124,28 @@ function user_build_content($account, $view_mode = 'full', $langcode = NULL) {
+function _user_mail_text($key, $language = NULL, $variables = array()) {

The amount of ugly, horribly code removed here brings a tear of joy to my eye! :D

Great work, and it's lovely to now have our first piece of multilingual configuration use case in Drupal 8! :D

Committed and pushed to 8.x. Yay!

Moving down to needs work for setUpSystemConfigForm(). We should either remove it or document it.

Also, that newline-separated YAML output is ooglay as hell, as was previously observed. Can we make sure we're tracking that in an issue somewhere? I didn't seem to spot one in reading the above.

Moving off of D8MI sprint given that we got our data to work with for translatability, which is where this was a hard requirement for us.

Removed method; in discussion with zendoodles we can to the conclusion the method is best removed.

- setUp gives $this-> access aswell
- abstract methods tend to be a standardization in coding. setUp already provides this.
- setUp currently only intitializes an empty array.

Darnit, the setUp() still had the call to the abstract method. Fixed that now.

However, in the example in $values doc is a reference to a test already making use of the abstract method.

This doc needs to be updated + the method gone.

This still needs fixing. (UserAdminSettingsFormTest).

#28: remove_abstract_method-1757566-28.patch queued for re-testing.

Attached is a patch that fixes user admin settings to not expect that abstract method to exist. I also grep'd the entire codebase with a fresh pull to verify no other tests were using this abstract method. I'm upping the priority because I don't think we want anyone else trying to use that base admin config form until that abstract method is removed. No interdiff because previous patch was two lines, so I didn't bother.

Ok looks good

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminSettingsFormTest.phpundefined
@@ -19,7 +19,11 @@ public static function getInfo() {
+  /**
+   * Overrides \Drupal\system\Tests\System\SystemConfigFormBase::setUp().
+   */

This should not have a docblock. Yeah, it's inconsistent, but that's the standard: http://drupal.org/node/325974

Also, this is not a major. See definition of issue priorities here: http://drupal.org/node/45111 :)

@xjm
I recreated #32 disasm's patch, removing that docblock. Here're some questions about what I've done.

1. Why don't we want that docblcok is because we are overriding a function provided by parent Class. If this function is provided by current child Class, then it's ok to add this docblock, just like:

  /**
   * One-sentence description of what test entails.
   */
  public function testNextExampleThing() {
    // Assertions, etc. go here.
  }

Is this true?

2. Steps to create this patch are:

1. git pull --rebase. Get the newest drupal8;
2. git apply drupal-fix_user_admin_settings-1757566-32.patch. Apply that patch I want to modify;
3. Manually remove that docblock from file, with an editor;
4. git diff 8.x. See the difference between this branch;
5. git diff 8.x > drupal-remove_docblock-1757566-34.patch. Get the new patch.

Firstly, I tried to follow doc here http://drupal.org/node/707484 and http://drupal.org/node/1054616 to create this patch. Both two docs say something like this, 'commit with all changes before try git diff branch_name or git diff branch_name > some_patch_name'.

My problem is, after I do git commit -m"" to commit all changes, and then do git diff branch_name, e.g., git diff 8.x, it shows nothing! It's supposed to show the difference I've made ,so I could create a patach then. But it doesn't. So I created this patch before git commit. git diff 8.x will show the difference after I mannualy change the file, or after I do git add, but it won't show after git commit.

I thought it might only apply to drupal projects(theme/module), not drupal core. So I did another test with drupal module. This scenario happens again! After git commit, patch can't be created.

Is there anything I'v done wrong? Thank you.

New patch with interdiff.
And, because of this line 'There is no PHPDoc on this function since it is an inherited method.' in http://drupal.org/node/325974, we don't need this docblock.

based on the rtbc in #33 the latest change to remove the docblock on setUp() in the tests looks good.

(nice interdiff too!)

#32: drupal-fix_user_admin_settings-1757566-32.patch queued for re-testing.

#35: drupal-remove_docblock-1757566-34.patch queued for re-testing.

I googled method visibility. @sun do you mean to change
public function setUp() {
to
function setUp() {
?

http://drupal.org/node/325974
shows the example with the public

+++ b/core/modules/system/lib/Drupal/system/Tests/System/SystemConfigFormBase.phpundefined
@@ -45,12 +45,8 @@
   public function setUp() {
     $this->values = array();
     parent::setUp();
...
   }

Since the particular setup line is being removed, I wonder if the whole setUp() method can be removed.

http://drupal.org/node/325974 says "setUp() is optional. [...] If the module don't have to perform any particular setup, it should not be defined."

OK, according to @sun in #41, new patch attached.
This patch is created from #35. So, let's see if this works. I haven't tested patch in #36 in local environment. If this patch fails, will test this in local installation.
It seems that failed patch has some bad effect on node type and core update. Maybe some new features added or something else.

I talked with some folks in irc and we agree with @sun that generally in the past we have not specified visibility, but now we do for everything.

Should be protected than in stead of public

@aspilicious hm? if so, please go ahead and add a 8.x section to http://drupal.org/node/325974 saying that it should be protected in 8.x

If the docs say so...

(Related to the test standard question: #1869794: Update tests coding standards doc and make consistant with 1354 where appropriate.)

Committed and pushed to 8.x. Thanks!