Goal

  • Convert the user account e-mail templates in User module to the configuration system.
Files: 
CommentFileSizeAuthor
#45 drupal-fix_user_admin_settings-1757566-45.patch1.27 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 49,414 pass(es).
[ View ]
#45 interdiff-41-45.txt1.13 KBYesCT
#43 drupal-fix_user_admin_settings-1757566-41.patch1.28 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] 49,382 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#43 interdiff-34-41.txt1.06 KBsmiletrl
#36 drupal-fix_user_admin_settings-1757566-34.patch1.24 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] 49,113 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#36 interdiff-32-34.txt592 bytessmiletrl
#35 drupal-remove_docblock-1757566-34.patch1.24 KBsmiletrl
PASSED: [[SimpleTest]]: [MySQL] 49,282 pass(es).
[ View ]
#32 drupal-fix_user_admin_settings-1757566-32.patch1.32 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 49,294 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#28 remove_abstract_method-1757566-28.patch709 bytessimolokid
FAILED: [[SimpleTest]]: [MySQL] 46,439 pass(es), 0 fail(s), and 4 exception(s).
[ View ]
#27 remove_abstract_method-1757566-27.patch625 bytessimolokid
PASSED: [[SimpleTest]]: [MySQL] 41,906 pass(es).
[ View ]
#18 13-18-interdiff.txt3.51 KBalexpott
#18 1757566_18.user_mails.drupal8.patch24.76 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 41,109 pass(es).
[ View ]
#13 9-13-interdiff.txt6.37 KBalexpott
#13 1757566_13.user_mails.drupal8.patch20.89 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 41,108 pass(es).
[ View ]
#9 7-9-interdiff.txt10.62 KBalexpott
#9 1757566_9.user_mails.drupal8.patch26.62 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 41,109 pass(es).
[ View ]
#7 1757566_7.user_mails.drupal8.patch26.59 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1757566_7.user_mails.drupal8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#7 2-7-interdiff.txt2.02 KBalexpott
#2 1757566_2.user_mails.drupal8.patch24.67 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 40,706 pass(es).
[ View ]

Comments

webchick’s picture

Issue tags:+D8MI

Also tagging D8MI since this affects them.

alexpott’s picture

Status:Active» Needs review
StatusFileSize
new24.67 KB
PASSED: [[SimpleTest]]: [MySQL] 40,706 pass(es).
[ View ]

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.

aspilicious’s picture

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

alexpott’s picture

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

Gábor Hojtsy’s picture

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

aspilicious’s picture

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

alexpott’s picture

Status:Needs work» Needs review
Issue tags:+Needs tests
StatusFileSize
new2.02 KB
new26.59 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1757566_7.user_mails.drupal8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Gábor Hojtsy’s picture

Status:Needs review» Needs work
Issue tags:+sprint

Tag it accordingly. Also bring on the D8MI sprint.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new26.62 KB
PASSED: [[SimpleTest]]: [MySQL] 41,109 pass(es).
[ View ]
new10.62 KB

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

Jose Reyero’s picture

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’s picture

@Jose Reyero: Hm. Really not sure about that. I don't think that any kind of metadata/schema/property/translation system should dictate the structure and/or grouping of configuration data. IMHO, it has to be the other way around; whatever system is tacked onto the configuration has to bend itself to support every possibility of how configuration might appear and be defined.

On the patch:

- I do not really understand why the default values are still in code? All default values should be migrated into default config objects.

- I also wonder about the manual newlines in the config strings. Is Symfony's Yaml Dumper not able to determine long text containing line-breaks and use the appropriate Yaml syntax for that? If so, we can go with this for now, but we should open a PR to fix it.

sun’s picture

Issue tags:+YAML

Introducing a new YAML issue tag to track the issues with it.

alexpott’s picture

StatusFileSize
new20.89 KB
PASSED: [[SimpleTest]]: [MySQL] 41,108 pass(es).
[ View ]
new6.37 KB

@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 :(

alexpott’s picture

@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’s picture

Status:Needs review» Reviewed & tested by the community

Thanks! :) Looks good to me.

Let's make sure to investigate the issue with yaml multi-line syntax in a separate issue. Certainly not the end of the world, but would be nice to have these values a bit more readable in user.mail.yml.

Jose Reyero’s picture

Status:Reviewed & tested by the community» Needs review

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

sun’s picture

@alexpott: Yep, the parser does. But AFAICS, the dumper doesn't. The binary/literal syntax is what we're looking for, but the dumper needs a way to determine when to use it (@see Yaml\Inline::dump()). Anyway, slightly OT for this issue...

@Jose Reyero:
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.

When I first saw this patch, I actually felt quite tempted to ask why these templates are not in the user.settings config object but in a new user.mail object. Though I guess it's ok to have them in a separate user.mail file, since user.settings is needed + loaded more often.

alexpott’s picture

StatusFileSize
new24.76 KB
PASSED: [[SimpleTest]]: [MySQL] 41,109 pass(es).
[ View ]
new3.51 KB

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’s picture

Hm. I'm a fan of standardizing things, but I'm not sure whether this abstraction will actually work out as intended.

For one, many settings forms are different and allow for advanced interaction patterns, which we won't be able to support with this.

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.

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

Personally, I'd go one step further and say that requiring to add tests for this issue/patch is sorta in the mantra of "Never Touch A Running System"... in other words: 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.

Jose Reyero’s picture

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

aspilicious’s picture

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

Jose Reyero’s picture

Status:Needs review» Reviewed & tested by the community

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.

alexpott’s picture

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

Gábor Hojtsy’s picture

Issue tags:+language-config

Subtaging for D8MI.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

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.

Gábor Hojtsy’s picture

Issue tags:-sprint

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.

simolokid’s picture

Status:Needs work» Needs review
StatusFileSize
new625 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,906 pass(es).
[ View ]

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.

simolokid’s picture

StatusFileSize
new709 bytes
FAILED: [[SimpleTest]]: [MySQL] 46,439 pass(es), 0 fail(s), and 4 exception(s).
[ View ]

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

Status:Needs review» Needs work
Issue tags:-Needs tests, -Configuration system, -D8MI, -language-config, -Config novice, -YAML

The last submitted patch, remove_abstract_method-1757566-28.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Needs tests, +Configuration system, +D8MI, +language-config, +Config novice, +YAML

The last submitted patch, remove_abstract_method-1757566-28.patch, failed testing.

disasm’s picture

Priority:Normal» Major
Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new1.32 KB
FAILED: [[SimpleTest]]: [MySQL] 49,294 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

aspilicious’s picture

Status:Needs review» Reviewed & tested by the community

Ok looks good

xjm’s picture

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

smiletrl’s picture

StatusFileSize
new1.24 KB
PASSED: [[SimpleTest]]: [MySQL] 49,282 pass(es).
[ View ]

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

<?php
 
/**
   * 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.

smiletrl’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new592 bytes
new1.24 KB
FAILED: [[SimpleTest]]: [MySQL] 49,113 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

YesCT’s picture

Status:Needs review» Reviewed & tested by the community

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

(nice interdiff too!)

Status:Needs review» Needs work
Issue tags:-Configuration system, -D8MI, -language-config, -Config novice, -YAML

The last submitted patch, drupal-fix_user_admin_settings-1757566-34.patch, failed testing.

smiletrl’s picture

Status:Needs work» Needs review
YesCT’s picture

sun’s picture

#35 and #36 appear to be identical patches, so I do not understand why #36 failed.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/SystemConfigFormBase.php
@@ -45,12 +45,8 @@
   public function setUp() {

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminSettingsFormTest.php
@@ -19,7 +19,8 @@ public static function getInfo() {
+  public function setUp() {

setUp() is a protected method.

We should either remove the visibility entirely from both test classes, or change it to protected. Since we generally do not specify the method visibility for setUp(), tearDown(), and test* methods, I'd actually prefer to remove the visibility.

YesCT’s picture

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

smiletrl’s picture

StatusFileSize
new1.06 KB
new1.28 KB
FAILED: [[SimpleTest]]: [MySQL] 49,382 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal-fix_user_admin_settings-1757566-41.patch, failed testing.

YesCT’s picture

StatusFileSize
new1.13 KB
new1.27 KB
PASSED: [[SimpleTest]]: [MySQL] 49,414 pass(es).
[ View ]

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.

YesCT’s picture

Status:Needs work» Needs review
aspilicious’s picture

Status:Needs review» Needs work

Should be protected than in stead of public

YesCT’s picture

@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

aspilicious’s picture

Status:Needs work» Reviewed & tested by the community

If the docs say so...

YesCT’s picture

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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