The message telling that the new user has been e-mailed is always displayed, even if it's not true.

Tested and all seems to work fine.

Let's add an automated test for this. Also tagging for the backport.

I am trying to come up with a test case and found it is difficult to stop mail function with code.

Here is the test patch (It should fail before applying patch at #2). I am also put test patch together with the patch in #2.

The patch overall functionally looks great wuinfo! I would like a little better naming though, can't figure out what Mal is supposed to be. Also, the variable_get/variable_set function you are using should be converted to CMI. We don't want to introduce new old style variable functions after a conversion.

+++ b/core/lib/Drupal/Core/Mail/MalPhpMail.phpundefined
@@ -0,0 +1,23 @@
+ * Definition of Drupal\Core\Mail\MalPhpMail.

What is Mal?

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateFailMailTest.phpundefined
@@ -0,0 +1,54 @@
+    $mail_system_variable = variable_get('mail_system');
+    variable_set('mail_system', array('default-system' => 'Drupal\Core\Mail\MalPhpMail'));

this should be CMI since mail system is converted. Use: config('system.mail')->get('interface'); instead of variable_get('mail_system);

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateFailMailTest.phpundefined
@@ -0,0 +1,54 @@
+    variable_set('mail_system',$mail_system_variable);

This should be CMI as well.

"Mal" in "MalPhpMail is same as "Mal" in word "Malfunction".

In order to let mail function fail intentionally, I put the MalPhpMail there. Would love the change the name if you have one for it.

Meanwhile, I will convert variable_get/variable_set function to CMI.

variable_get/variable_set function were converted to CMI.

Thanks @wuinfo! That test should provide the needed coverage. I like the solution of defining a "broken" mail service.

First, one note about file locations and namespacing for classes that are used only for testing:

+++ b/core/lib/Drupal/Core/Mail/MalPhpMail.phpundefined
@@ -0,0 +1,23 @@
+ * Definition of Drupal\Core\Mail\MalPhpMail.
...
+class MalPhpMail extends PhpMail implements MailInterface {

I'm with @disasm that "MalPHPMail" is a bit confusing--generally, we prefer whole words in class names, functions, etc. rather than abbreviations. I'd suggest TestPhpMailFailure or something like that.

Since this class is purely for testing purposes, we should instead move it to the test namespace (including creating a test module to house it if appropriate).

Since the bug we're testing is in the user module, the place to create a test module would be in something like:

core/modules/user/tests/user_mail_failure_test/

We'd create an empty module file in that directory, and then move the test mail sender to the appropriate PSR-0 directory within that module. So, the namespace for the test class will then be:
Drupal\user_mail_failure_test; and we'll need to add a use statement for Drupal\Core\Mail\PhpMail.

Make sense? :)

Aside from that, there's just a couple minor cleanups to make:

1. +++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateFailMailTest.phpundefined
   @@ -0,0 +1,53 @@
   + * Definition of Drupal\user\Tests\UserCreateFailMailTest.
   

   Minor thing, but the standards for these docblocks since last summer are that they should start with "Contains" instead of "Definition of" and have a preceding slash on the namespace, so:

   Contains \Drupal\user\Tests\UserCreateFailMailTest.
   

   (A lot of core isn't actually updated to the standard yet, but we should use it here.)

2. +++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateFailMailTest.phpundefined
   @@ -0,0 +1,53 @@
   + * Test the create user administration page.
   ...
   +   * Create a user through the administration interface and ensure that it
   +   * displays proper message when mail does not work.
   

   Minor docblock style note: The docblocks should be a single line of 80 characters or fewer and start with an indicative verb, e.g. "Tests."

3. +++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateFailMailTest.phpundefined
   @@ -0,0 +1,53 @@
   +    // We stop mail function
   ...
   +    // We create a user, notify the created user and fail to send email.
   

   For inline comments, we normally use the imperative, e.g.:

   // Replace the mail functionality with a fake, malfunctioning service.
   

   // Create a user, but fail to send an email.
   

   (Also make sure that they are complete sentences with capitalization, periods, articles, etc.)

4. +++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateFailMailTest.phpundefined
   @@ -0,0 +1,53 @@
   +      'notify' => true,
   

   TRUE should be in all caps except in JS.

5. +++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateFailMailTest.phpundefined
   @@ -0,0 +1,53 @@
   +    $this->assertText(t('Unable to send e-mail. Contact the site administrator if the problem persists.'),'Error message for disfunctional mail.');
   +    $this->assertNoText(t('A welcome message with further instructions has been e-mailed to the new user @name.', array('@name' => $edit['name'])), 'Do not show welcome message');
   

   We can probably remove the assertion messages from these two assertions--the default assertion will be even more specific.

6. +++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateFailMailTest.phpundefined
   @@ -0,0 +1,53 @@
   +    // Set the old variable back.
   +    config('system.mail')->set('interface.default', $mail_system_variable)->save();
   

   Each test method gets its own fresh test installation, so AFAIK we don't need to do this. (Might be worth testing just to make sure it doesn't leak into the parent environment -- remove the line, run the test, and then see if the parent site's setting is affected.)

Adding the novice tag; all these changes should be fairly simple to make. If you do update @wuinfo's patch, please be sure to upload the failing patch as well as the combined patch again, and also provide an interdiff showing your changes. Thanks!

Thanks xjm, that makes a lot sense.

Changes are done according to @xjm's review.

Each test method gets its own fresh test installation, so AFAIK we don't need to do this. (Might be worth testing just to make sure it doesn't leak into the parent environment -- remove the line, run the test, and then see if the parent site's setting is affected.)

Tested mail function after running test. Testing did not leak into the parent environment.
1) Installed a new Drupal8 site.
2) Enabled the test module.
3) Tested the user part of tests tasks.
4) Tested the really mail function by sending a notification.(mail was sent.*)

Another patch in #2 is combined with test patch. The combined patch should pass the test. The test patch itself should fail.

Thanks @wuinfo! Note that if you put the test-only patch first and the passing patch second, the testbot won't mark it needs work. :)

I think this is pretty much ready. I just noticed a couple small things re-reading it:

1. +++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateFailMailTest.phpundefined
   @@ -0,0 +1,49 @@
   + * Test the create user administration page.
   

   Tests, not test. :) http://drupal.org/node/1354#functions

2. +++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateFailMailTest.phpundefined
   @@ -0,0 +1,49 @@
   +  // Enable user_mail_failure_test module.
   +  public static $modules = array('user_mail_failure_test');
   ...
   +  // Test the create user administration page.
   +  protected function testUserAdd() {
   

   Ah, sorry if I was unclear: These should be one line only, but they should still be docblocks: http://drupal.org/node/1354#functions

   Also, "Tests". :)

3. +++ b/core/modules/user/tests/modules/user_mail_failure_test/lib/Drupal/user_mail_failure_test/TestPhpMailFailure.phpundefined
   @@ -0,0 +1,26 @@
   +  public function mail(array $message) {
   +    return FALSE;
   

   Maybe just add an inline comment here as well to explicitly say:

   // Instead of attempting to send a message, just return failure.
   

   (Above the return but inside the function.)

4. +++ b/core/modules/user/tests/modules/user_mail_failure_test/user_mail_failure_test.info.ymlundefined
   @@ -0,0 +1,8 @@
   diff --git a/core/modules/user/tests/modules/user_mail_failure_test/user_mail_failure_test.module b/core/modules/user/tests/modules/user_mail_failure_test/user_mail_failure_test.module
   

   Let's also add an opening <?php tag and an @file docblock for the module file: http://drupal.org/node/1354#file

Tagging novice for those cleanups--removing the "Needs tests" tag because the current tests are exactly what we need.

Hi @xjm, thanks for the quick review.

Changes are done.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateFailMailTest.phpundefined
@@ -0,0 +1,51 @@
+  // Enable user_mail_failure_test module.
+  public static $modules = array('user_mail_failure_test');

Oops, one more thing -- This still needs to be a docblock as well. (Otherwise api.drupal.org and IDEs won't be able to parse it to list it as a member for the class.)

I really want to RTBC this, so I just fixed this myself. :) (In general one wouldn't RTBC one's own patch, but this is super minor and doesn't change any functional code.)

#18: user-mail-1843380-18.patch queued for re-testing.

The new user_mail_failure_test module is nice functionality... that should be used elsewhere so lets move it to system/tests and change it's name to system_mail_failure_test.

Minor nits...

+++ b/core/modules/user/tests/modules/user_mail_failure_test/user_mail_failure_test.info.ymlundefined
@@ -0,0 +1,8 @@
+description: 'Create situation of mail failure.'

The description can be improved to something like 'Provides a malfunctioning mail service for testing purposes.'

+++ b/core/modules/user/tests/modules/user_mail_failure_test/lib/Drupal/user_mail_failure_test/TestPhpMailFailure.phpundefined
@@ -0,0 +1,27 @@
+ * This class is for running tests or for development.

We should improve the class's doc block to say how to use it... something along the lines of

 * This class is for running tests or for development. To use set the 
 * configuration:
 * @code
 * config('system.mail')->set('interface.default', 'Drupal\system_mail_failure_test\TestPhpMailFailure')->save();
 * @endcode

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateFailMailTest.phpundefined
@@ -0,0 +1,53 @@
+  /**
+   * Enable the user_mail_failure_test module.
+   */

Normally this is:

  /**
   * Modules to enable.
   *
   * @var array
   */

+++ b/core/modules/user/tests/modules/user_mail_failure_test/user_mail_failure_test.info.ymlundefined
@@ -0,0 +1,8 @@
+dependencies:
+  - user

There is no dependency on the user module.

Thanks @alexpott,

Since I have changed the location of the module, the interdiff.txt file is large. So, I think it is not necessarily to include it.

Same as #22. Upload patch again for test.

@wuinfo You don't need to re-upload the patch to call testbot. Just change the status to needs review, and it will run the tests on any patches that haven't had tests ran on them.

Since I have changed the location of the module, the interdiff.txt file is large. So, I think it is not necessarily to include it.

It's still useful for the other changes, to make it easier for reviewers to check your changes against the committer's review in #21. :) Also, you can make moved/renamed files appear as just one line so long as you didn't also change a majority of the code in the file. You'll want to add this to your .gitconfig:

[diff]
  renames = copies

(See http://drupal.org/documentation/git/configure for more information.)

For this patch, I just re-read the whole thing and checked it against @alexpott's review.

1. FWIW, I'd actually disagree with @alexpott's recommendation here:

   +++ b/core/modules/user/lib/Drupal/user/Tests/UserCreateFailMailTest.phpundefined
   @@ -0,0 +1,53 @@
   +  /**
   +   * Enable the user_mail_failure_test module.
   +   */
   

   Normally this is:

     /**
      * Modules to enable.
      *
      * @var array
      */
   

   "Modules to enable" is IMO less useful in general than what we're enabling and why. ;) (However, the missing @var was a bug). It's fine to leave it as it is now, though; just wanted to point this out for future reference. :)

2. +++ b/core/modules/system/tests/modules/system_mail_failure_test/lib/Drupal/system_mail_failure_test/TestPhpMailFailure.phpundefined
   @@ -0,0 +1,31 @@
   + * Defines a mail sending implementation that return false.
   

   Typo here: "return" should be "returns".

3. +++ b/core/modules/system/tests/modules/system_mail_failure_test/lib/Drupal/system_mail_failure_test/TestPhpMailFailure.phpundefined
   @@ -0,0 +1,31 @@
   + * @code
   + * config('system.mail')->set('interface.default', 'Drupal\system_mail_failure_test\TestPhpMailFailure')->save();
   + * @endcode
   

   The config() line should be indented by two spaces here.

4. +++ b/core/modules/system/tests/modules/system_mail_failure_test/system_mail_failure_test.moduleundefined
   @@ -0,0 +1,7 @@
   + * Disables the email function for testing purpose.
   

   Typo: "purpose" should be "purposes." Also, maybe "Disables email sending functionality for testing purposes" would be clearer?

Looks great to me other than those small things. The rest of @alexpott's feedback seems to be covered.

@disasm, yes, I realized it soon after I changed the status on second comment.

interdiff between this and #23. Thanks @xjm, My English is getting better now :)

Requesting a retest as failure is unrelated...

The test did not complete due to a fatal error.	Completion check	AreaEntityTest.php	51	Drupal\views\Tests\Handler\AreaEntityTest->testEntityAreaData()

#27: contradictory-messages-creating-new-user-include-fix-1843380-27.patch queued for re-testing.

Thanks!

Committed 42bfe92 and pushed to 8.x. Thanks!

better written steps to reproduce

I am working on the backport!!

Hello, absolute beginner here!
I applied the patch using
patch -p2 < path/you/saved/patch/in.patch
which didn't work because of the various directory name changes and filename changes between D7 and D8.

So, I have manually tried to locate the code that needed changing and created a patch

Applied the patch on my local repo where it works but not sure how those test file works.

Please review and provide me the required suggestions!

Tested:
"A welcome message with further instructions has been e-mailed to the new user asdasd". Problem seems to be solved.

@nileshleve : Please add Test case for the suggested patch .

Cheers
SwapS