Fix 5244: Removed Email Subclasses #5271

bummzack · 2016-04-04T09:51:17Z

Removed Email Subclasses used by the Member class (Member_ChangePasswordEmail and Member_ForgotPasswordEmail).

Added a test for the forgot password email.
Improved the test for the change-password email.
It's not guaranteed that the TestMailer instance will remain the same across a unit-test, made $emailsSent a static member to fix this issue.

tractorcow · 2016-04-05T05:34:13Z

dev/TestMailer.php

@@ -4,14 +4,14 @@
 * @subpackage email
 */
 class TestMailer extends Mailer {
-	protected $emailsSent = array();
+	protected static $emailsSent = array();


This is going to be problematic if running across multiple tests... creating and re-setting a new TestMailer won't flush all old emails.

I suggest keeping these as non-static if you can.

I think that's what clearEmails is for (flushing emails, that is)? Since TestMailer isn't enforced to be a singleton during a test, you can get multiple instances of it, where one will have Emails stored and another one doesn't. This was the case for me when I triggered an Email via FunctionalTest::post (see my testForgotPasswordEmaling test). There seem to be two different instances of TestMailer being instantiated in that case and the Mailer used within the tests itself won't contain any mails, unless $emailsSent is a static member.

I think overall a static member makes sense, since you can't enforce the mailer instance via Mailer::set_mailer. Tests that send Emails should use $this->clearEmails at the start… I don't see any issues with this, unless you're starting to run tests in parallel (and even then, you can find your sent Email using the existing API).

I investigated a bit further. It seems like using Email::mailer() will get you the correct instance, while SapphireTest::$mailer will return another (unpopulated) instance. That's really strange, since SapphireTest sets Injector::inst()->registerService($this->mailer, 'Mailer'); in the startup, which should make them the same instance.

I guess it's time to start the debugger to see what's going on.

Ok, the issue wasn't related to FunctionalTest::post at all (I thought it might have something to do with Injector nesting that happens within Director::test).

The SapphireTest->mailer instance gets already overridden/canceled-out within the setUp method whenever self::create_temp_db() is being called (which in turn calls SapphireTest::resetDBSchema that clears the service cache, via Injector::unregisterAllObjects).

Solving the problem is relatively simple, by just setting up the Mailer at the end of the setUp method. Will amend this PR with these changes and revert the changes made to TestMailer (eg. not using static members).

Fantastic fix - I had this problem the other day and just gave up testing emails, I think there's even an issue open about this

tractorcow · 2016-04-05T05:34:53Z

Thanks, very good overall! Just a comment on the member variables.

tractorcow · 2016-04-05T22:48:03Z

tests/security/MemberTest.php

+			urldecode($response->getHeader('Location')));
+
+		// Check existance of reset link
+		$this->assertEmailSent("sam@silverstripe.com", null, 'Your password reset link',


Ah, just one more thing... can you please change this to not be a real email address? I just want to avoid the case where someday down the line sam ends up with 10000 test emails from travis.

Haha, sure thing. Maybe it would be a good idea to generally replace all "real" E-Mail Addresses from fixture-files?

…ordEmail and Member_ForgotPasswordEmail). Added a test for the forgot password email. Improved the test for the change-password email. Fixed issue where `SapphireTest::mailer` was cleared during `setUp` by moving instantiation of the mailer at the end of the `setUp` method. No longer use deprecated i18n method in test-setup. Replace potentially real Email Address with a fake one.

dhensby · 2016-04-06T15:00:54Z

OK - I'm not particularly happy with this because it actually makes these emails much more rigid than they used to be.

I'd assume the reason they are their own subclasses is because it allows better customisation, however there's no way here to change their behaviour (ie: no use of config or extension hooks)?

This is more a problem with the injector that should be fixed IMO rather than this (see #4774 where it's been decided that we shouldn't override all child classes). This means it's on the dev to override all subclasses individually if needed.

dhensby · 2016-04-06T15:00:57Z

related: #5000

bummzack · 2016-04-06T21:36:25Z

What kind of customizations did you have in mind? The most common one is obviously overriding the template which is still possible.
For me, Emails are pretty much presentational and shouldn't contain any logic, or at least no logic that is specific to the member tasks being discussed here? As a developer you might want to have a more fine grained control when and under what circumstances these Emails are being sent, but this is out of the scope of an Email subclass, isn't it? Same goes for what data is being fed into the Email template… it's something that happens where the Email is instantiated, not in the Email class itself.

We could add an Injector "placeholder", something like this:

Injector:
  Member_ChangePasswordEmail:
    class: Email

And then use Injector::inst()->create('Member_ChangePasswordEmail'); in code. This will still create an instance of Email by default, but could easily be swapped out for something else. As outlined before, I don't see much benefit in doing this, though.

And: Yeah, it seems like the issue #5000 is the same I encountered when writing tests for this fix.

dhensby · 2016-04-06T22:41:28Z

You're right. But that doesn't mean the devs aren't doing it and without providing other means to allow them to, makes it more rigid.

Your placeholder solution compounds the problem in #5000, surely? Unless the injector looks for the class that replaces email when insttantiating this?

bummzack · 2016-04-07T06:40:01Z

All I'm saying is that issue #5000 would be fixed by this PR.
The injector placeholder was just a suggestion to mitigate the drawbacks of having these Email subclasses removed and is not related to issue #5000 at all. Sorry if that was unclear.

bummzack · 2016-04-07T07:41:24Z

And a note regarding Email subclasses: I think these exist, because in earlier versions of SilverStripe (2.2 and prior) there wasn't a setTemplate method on Email, so subclassing was the only way of using a separate Template.

The shop module also deprecated Email subclasses in favor of using setTemplate.

My point is: These subclasses are here because of legacy reasons and no longer represent best-practice.

I'll leave it up to you ( @dhensby , @tractorcow anyone else? ) to decide if you want to keep these subclasses or not. Please let me know, so that I can amend this PR, because I think it would contain other useful improvements (such as improving Email testing). Cheers.

dhensby · 2016-04-08T16:09:45Z

@bummzack OK - I've spoken to @kinglozzer and I think it's worth merging

I do think this is more rigid that it was before, but allowing custom templates should do away with most of the customisations people will need.

dhensby · 2016-04-08T16:09:54Z

thanks

bummzack · 2016-04-08T16:54:39Z

Great. From my point of view, issue #5000 could also be closed now. Should I create a separate PR for 3.x to fix the Email testing issue there as well?

On 08.04.2016, at 18:09, Daniel Hensby notifications@github.com wrote:

@bummzack OK - I've spoken to @kinglozzer and I think it's worth merging

I do think this is more rigid that it was before, but allowing custom templates should do away with most of the customisations people will need.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

dhensby · 2016-04-08T16:57:02Z

@bummzack yes, please patch 3.x for that issue (even patching 3.2.x would make sense)

Updated/added tests for changed- and forgot-password Emails. Updated fixture and tests to no longer use a real Email address.

Fix Email test issue discovered in #5271.

bummzack mentioned this pull request Apr 4, 2016

Security Email subclasses such as Member_ForgotPasswordEmail can cause issues with injector overrides. #5244

Closed

tractorcow reviewed Apr 5, 2016
View reviewed changes

bummzack force-pushed the 5244-remove-email-subclasses branch 2 times, most recently from e1ef3d2 to 01731cc Compare April 5, 2016 11:42

tractorcow reviewed Apr 5, 2016
View reviewed changes

bummzack force-pushed the 5244-remove-email-subclasses branch from 01731cc to ca4036b Compare April 6, 2016 07:54

dhensby merged commit d7289a1 into silverstripe:master Apr 8, 2016

bummzack added a commit to bummzack/silverstripe-framework that referenced this pull request Apr 11, 2016

Fix Email test issue discovered in silverstripe#5271.

8d92d6f

Updated/added tests for changed- and forgot-password Emails. Updated fixture and tests to no longer use a real Email address.

bummzack mentioned this pull request Apr 11, 2016

Fix Email test issue discovered in #5271. #5296

Closed

bummzack added a commit to bummzack/silverstripe-framework that referenced this pull request Apr 11, 2016

Fix Email test issue discovered in silverstripe#5271.

9146450

Updated/added tests for changed- and forgot-password Emails. Updated fixture and tests to no longer use a real Email address.

dhensby added a commit that referenced this pull request Apr 11, 2016

Merge pull request #5298 from bummzack/patch-email-tests-3.2

e57d26b

Fix Email test issue discovered in #5271.

dhensby mentioned this pull request Jul 17, 2016

SapphireTest fix for issue that caused assertEmailSent to always fail #5000

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 5244: Removed Email Subclasses #5271

Fix 5244: Removed Email Subclasses #5271

bummzack commented Apr 4, 2016

tractorcow Apr 5, 2016

bummzack Apr 5, 2016

bummzack Apr 5, 2016

bummzack Apr 5, 2016

dhensby Apr 6, 2016

tractorcow commented Apr 5, 2016

tractorcow Apr 5, 2016

bummzack Apr 6, 2016

dhensby commented Apr 6, 2016

dhensby commented Apr 6, 2016

bummzack commented Apr 6, 2016

dhensby commented Apr 6, 2016

bummzack commented Apr 7, 2016

bummzack commented Apr 7, 2016

dhensby commented Apr 8, 2016

dhensby commented Apr 8, 2016

bummzack commented Apr 8, 2016

dhensby commented Apr 8, 2016

Fix 5244: Removed Email Subclasses #5271

Fix 5244: Removed Email Subclasses #5271

Conversation

bummzack commented Apr 4, 2016

tractorcow Apr 5, 2016

Choose a reason for hiding this comment

bummzack Apr 5, 2016

Choose a reason for hiding this comment

bummzack Apr 5, 2016

Choose a reason for hiding this comment

bummzack Apr 5, 2016

Choose a reason for hiding this comment

dhensby Apr 6, 2016

Choose a reason for hiding this comment

tractorcow commented Apr 5, 2016

tractorcow Apr 5, 2016

Choose a reason for hiding this comment

bummzack Apr 6, 2016

Choose a reason for hiding this comment

dhensby commented Apr 6, 2016

dhensby commented Apr 6, 2016

bummzack commented Apr 6, 2016

dhensby commented Apr 6, 2016

bummzack commented Apr 7, 2016

bummzack commented Apr 7, 2016

dhensby commented Apr 8, 2016

dhensby commented Apr 8, 2016

bummzack commented Apr 8, 2016

dhensby commented Apr 8, 2016