BUG Fix enable email subclasses to use their respective templates

src/Control/Email/Email.php

@@ -7,7 +7,11 @@ use SilverStripe\Control\HTTP;
 use SilverStripe\Core\Convert;
 use SilverStripe\Core\Injector\Injector;
 use SilverStripe\ORM\FieldType\DBDatetime;
+use SilverStripe\ORM\FieldType\DBField;
+use SilverStripe\ORM\FieldType\DBHTMLText;
 use SilverStripe\View\Requirements;
+use SilverStripe\View\SSViewer;
+use SilverStripe\View\ThemeResourceLoader;
 use SilverStripe\View\ViewableData;
 use Swift_Message;
 use Swift_MimePart;
@@ -58,12 +62,12 @@ class Email extends ViewableData
     /**
      * @var string The name of the HTML template to render the email with (without *.ss extension)
      */
-    private $HTMLTemplate = self::class;
+    private $HTMLTemplate = null;
 
     /**
      * @var string The name of the plain text template to render the plain part of the email with
      */
-    private $plainTemplate = '';
+    private $plainTemplate = null;
 
     /**
      * @var Swift_MimePart
@@ -648,7 +652,14 @@ class Email extends ViewableData
      */
     public function getHTMLTemplate()
     {
-        return $this->HTMLTemplate;
+        if ($this->HTMLTemplate) {
+            return $this->HTMLTemplate;
+        }
+
+        return ThemeResourceLoader::inst()->findTemplate(
+            SSViewer::get_templates_by_class(static::class, '', self::class),
+            SSViewer::get_themes()
+        );
     }
 
     /**
@@ -760,30 +771,49 @@ class Email extends ViewableData
             $this->getSwiftMessage()->detach($existingPlainPart);
         }
         unset($existingPlainPart);
-        if (!$this->getHTMLTemplate() && !$this->getPlainTemplate()) {
+
+        // Respect explicitly set body
+        $htmlPart = $plainOnly ? null : $this->getBody();
+        $plainPart = $plainOnly ? $this->getBody() : null;
+
+        // Ensure we can at least render something
+        $htmlTemplate = $this->getHTMLTemplate();
+        $plainTemplate = $this->getPlainTemplate();
+        if (!$htmlTemplate && !$plainTemplate && !$plainPart && !$htmlPart) {
             return $this;
         }
-        $HTMLPart = '';
-        $plainPart = '';
 
-        if ($this->getHTMLTemplate()) {
+        // Render plain part
-            $HTMLPart = $this->renderWith($this->getHTMLTemplate(), $this->getData());
+        if ($plainTemplate && !$plainPart) {
+            $plainPart = $this->renderWith($plainTemplate, $this->getData());
         }
 
-        if ($this->getPlainTemplate()) {
+        // Render HTML part, either if sending html email, or a plain part is lacking
-            $plainPart =  $this->renderWith($this->getPlainTemplate(), $this->getData());
+        if (!$htmlPart && $htmlTemplate && (!$plainOnly || empty($plainPart))) {
-        } elseif ($HTMLPart) {
+            $htmlPart = $this->renderWith($htmlTemplate, $this->getData());
-            $plainPart = Convert::xml2raw($HTMLPart);
         }
 
-        if ($HTMLPart && !$plainOnly) {
+        // Plain part fails over to generated from html
-            $this->setBody($HTMLPart);
+        if (!$plainPart && $htmlPart) {
+            /** @var DBHTMLText $htmlPartObject */
+            $htmlPartObject = DBField::create_field('HTMLFragment', $htmlPart);
+            $plainPart = $htmlPartObject->Plain();
+        }
+
+        // Fail if no email to send
+        if (!$plainPart && !$htmlPart) {
+            return $this;
+        }
+
+        // Build HTML / Plain components
+        if ($htmlPart && !$plainOnly) {
+            $this->setBody($htmlPart);
             $this->getSwiftMessage()->setContentType('text/html');
             $this->getSwiftMessage()->setCharset('utf-8');
             if ($plainPart) {
                 $this->getSwiftMessage()->addPart($plainPart, 'text/plain', 'utf-8');
             }
-        } elseif ($plainPart || $plainOnly) {
+        } else {
             if ($plainPart) {
                 $this->setBody($plainPart);
             }
@@ -812,7 +842,7 @@ class Email extends ViewableData
      */
     public function hasPlainPart()
     {
-        if ($this->getSwiftMessage()->getContentType() == 'text/plain') {
+        if ($this->getSwiftMessage()->getContentType() === 'text/plain') {
             return true;
         }
         return (bool) $this->findPlainPart();

src/View/ThemeResourceLoader.php

@@ -103,7 +103,7 @@ class ThemeResourceLoader
             if (count($parts) > 1) {
                 throw new InvalidArgumentException("Invalid theme identifier {$identifier}");
             }
-            return substr($identifier, 1);
+            return ltrim($identifier, '/');
         }
 
         // If there is no slash / colon it's a legacy theme
@@ -148,7 +148,7 @@ class ThemeResourceLoader
         }
 
         // Join module with subpath
-        return $modulePath . $subpath;
+        return ltrim($modulePath . $subpath, '/');
     }
 
     /**

tests/php/Control/Email/EmailTest.php

@@ -4,10 +4,16 @@ namespace SilverStripe\Control\Tests\Email;
 
 use PHPUnit_Framework_MockObject_MockObject;
 use SilverStripe\Control\Email\Email;
+use SilverStripe\Control\Email\Mailer;
 use SilverStripe\Control\Email\SwiftMailer;
+use SilverStripe\Control\Tests\Email\EmailTest\EmailSubClass;
+use SilverStripe\Core\Injector\Injector;
+use SilverStripe\Core\Manifest\ModuleResourceLoader;
 use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Dev\TestMailer;
 use SilverStripe\ORM\FieldType\DBDatetime;
 use SilverStripe\Security\Member;
+use SilverStripe\View\SSViewer;
 use Swift_Attachment;
 use Swift_Mailer;
 use Swift_Message;
@@ -84,37 +90,25 @@ class EmailTest extends SapphireTest
 
     public function testSendPlain()
     {
-        /** @var Email|PHPUnit_Framework_MockObject_MockObject $email */
+        $email = $this->makeEmailMock('Test send plain');
-        $email = $this->getMockBuilder(Email::class)
-            ->enableProxyingToOriginalMethods()
-            ->disableOriginalConstructor()
-            ->setConstructorArgs(array(
-                'from@example.com',
-                'to@example.com',
-                'Test send plain',
-                'Testing Email->sendPlain()',
-                'cc@example.com',
-                'bcc@example.com',
-            ))
-            ->getMock();
 
         // email should not call render if a body is supplied
-        $email->expects($this->never())->method('render');
+        $email->expects($this->never())->method('renderWith');
-
-        $email->addAttachment(__DIR__ . '/EmailTest/attachment.txt', null, 'text/plain');
         $successful = $email->sendPlain();
 
         $this->assertTrue($successful);
         $this->assertEmpty($email->getFailedRecipients());
 
-        $sentMail = $this->mailer->findEmail('to@example.com');
+        /** @var TestMailer $mailer */
+        $mailer = Injector::inst()->get(Mailer::class);
+        $sentMail = $mailer->findEmail('to@example.com');
 
         $this->assertTrue(is_array($sentMail));
 
         $this->assertEquals('to@example.com', $sentMail['To']);
         $this->assertEquals('from@example.com', $sentMail['From']);
         $this->assertEquals('Test send plain', $sentMail['Subject']);
-        $this->assertEquals('Testing Email->sendPlain()', $sentMail['Content']);
+        $this->assertEquals('Body for Test send plain', $sentMail['Content']);
 
         $this->assertCount(1, $sentMail['AttachedFiles']);
         $child = reset($sentMail['AttachedFiles']);
@@ -126,36 +120,25 @@ class EmailTest extends SapphireTest
     public function testSend()
     {
         /** @var Email|PHPUnit_Framework_MockObject_MockObject $email */
-        $email = $this->getMockBuilder(Email::class)
+        $email = $this->makeEmailMock('Test send HTML');
-            ->enableProxyingToOriginalMethods()
-            ->disableOriginalConstructor()
-            ->setConstructorArgs(array(
-                'from@example.com',
-                'to@example.com',
-                'Test send HTML',
-                'Testing Email->send()',
-                'cc@example.com',
-                'bcc@example.com',
-            ))
-            ->getMock();
 
         // email should not call render if a body is supplied
-        $email->expects($this->never())->method('render');
+        $email->expects($this->never())->method('renderWith');
-
-        $email->addAttachment(__DIR__ . '/EmailTest/attachment.txt', null, 'text/plain');
         $successful = $email->send();
 
         $this->assertTrue($successful);
         $this->assertEmpty($email->getFailedRecipients());
 
-        $sentMail = $this->mailer->findEmail('to@example.com');
+        /** @var TestMailer $mailer */
+        $mailer = Injector::inst()->get(Mailer::class);
+        $sentMail = $mailer->findEmail('to@example.com');
 
         $this->assertTrue(is_array($sentMail));
 
         $this->assertEquals('to@example.com', $sentMail['To']);
         $this->assertEquals('from@example.com', $sentMail['From']);
         $this->assertEquals('Test send HTML', $sentMail['Subject']);
-        $this->assertEquals('Testing Email->send()', $sentMail['Content']);
+        $this->assertEquals('Body for Test send HTML', $sentMail['Content']);
 
         $this->assertCount(1, $sentMail['AttachedFiles']);
         $child = reset($sentMail['AttachedFiles']);
@@ -169,12 +152,9 @@ class EmailTest extends SapphireTest
         /** @var Email|PHPUnit_Framework_MockObject_MockObject $email */
         $email = $this->getMockBuilder(Email::class)
             ->enableProxyingToOriginalMethods()
-            ->disableOriginalConstructor()
-            ->setConstructorArgs(array(
-              'from@example.com',
-              'to@example.com',
-            ))
             ->getMock();
+        $email->setFrom('from@example.com');
+        $email->setTo('to@example.com');
         $email->setData(array(
             'EmailContent' => 'test',
         ));
@@ -188,6 +168,31 @@ class EmailTest extends SapphireTest
         $this->assertNotEmpty($email->getBody());
     }
 
+    public function testRenderedSendSubclass()
+    {
+        // Include dev theme
+        SSViewer::set_themes([
+            'silverstripe/framework:/tests/php/Control/Email/EmailTest',
+            '$default',
+        ]);
+
+        /** @var Email|PHPUnit_Framework_MockObject_MockObject $email */
+        $email = $this->getMockBuilder(EmailSubClass::class)
+            ->enableProxyingToOriginalMethods()
+            ->getMock();
+        $email->setFrom('from@example.com');
+        $email->setTo('to@example.com');
+        $email->setData(array(
+            'EmailContent' => 'test',
+        ));
+        $this->assertFalse($email->hasPlainPart());
+        $this->assertEmpty($email->getBody());
+        $email->send();
+        $this->assertTrue($email->hasPlainPart());
+        $this->assertNotEmpty($email->getBody());
+        $this->assertContains('<h1>Email Sub-class</h1>', $email->getBody());
+    }
+
     public function testConsturctor()
     {
         $email = new Email(
@@ -460,8 +465,33 @@ class EmailTest extends SapphireTest
 
     public function testHTMLTemplate()
     {
+        // Include dev theme
+        SSViewer::set_themes([
+            'silverstripe/framework:/tests/php/Control/Email/EmailTest',
+            '$default',
+        ]);
+
+        // Find template on disk
+        $emailTemplate = ModuleResourceLoader::singleton()->resolveResource(
+            'silverstripe/framework:templates/SilverStripe/Control/Email/Email.ss'
+        );
+        $subClassTemplate = ModuleResourceLoader::singleton()->resolveResource(
+            'silverstripe/framework:tests/php/Control/Email/EmailTest/templates/'
+            . str_replace('\\', '/', EmailSubClass::class)
+            .'.ss'
+        );
+        $this->assertTrue($emailTemplate->exists());
+        $this->assertTrue($subClassTemplate->exists());
+
+        // Check template is auto-found
         $email = new Email();
-        $this->assertEquals(Email::class, $email->getHTMLTemplate());
+        $this->assertEquals($emailTemplate->getPath(), $email->getHTMLTemplate());
+        $email->setHTMLTemplate('MyTemplate');
+        $this->assertEquals('MyTemplate', $email->getHTMLTemplate());
+
+        // Check subclass template is found
+        $email2 = new EmailSubClass();
+        $this->assertEquals($subClassTemplate->getPath(), $email2->getHTMLTemplate());
         $email->setHTMLTemplate('MyTemplate');
         $this->assertEquals('MyTemplate', $email->getHTMLTemplate());
     }
@@ -480,8 +510,8 @@ class EmailTest extends SapphireTest
         /** @var Swift_NullTransport|PHPUnit_Framework_MockObject_MockObject $transport */
         $transport = $this->getMockBuilder(Swift_NullTransport::class)->getMock();
         $transport->expects($this->once())
-                  ->method('send')
+            ->method('send')
-                  ->willThrowException(new Swift_RfcComplianceException('Bad email'));
+            ->willThrowException(new Swift_RfcComplianceException('Bad email'));
         $mailer->setSwiftMailer(new Swift_Mailer($transport));
         $email = new Email();
         $email->setTo('to@example.com');
@@ -495,7 +525,7 @@ class EmailTest extends SapphireTest
         $this->assertTrue((new Email)->IsEmail());
     }
 
-    public function testRender()
+    public function testRenderAgain()
     {
         $email = new Email();
         $email->setData(array(
@@ -578,4 +608,24 @@ class EmailTest extends SapphireTest
         $plainPart = reset($children);
         $this->assertContains('Test', $plainPart->getBody());
     }
+
+    /**
+     * @return PHPUnit_Framework_MockObject_MockObject|Email
+     */
+    protected function makeEmailMock($subject)
+    {
+        /** @var Email|PHPUnit_Framework_MockObject_MockObject $email */
+        $email = $this->getMockBuilder(Email::class)
+            ->enableProxyingToOriginalMethods()
+            ->getMock();
+
+        $email->setFrom('from@example.com');
+        $email->setTo('to@example.com');
+        $email->setSubject($subject);
+        $email->setBody("Body for {$subject}");
+        $email->setCC('cc@example.com');
+        $email->setBCC('bcc@example.com');
+        $email->addAttachment(__DIR__ . '/EmailTest/attachment.txt', null, 'text/plain');
+        return $email;
+    }
 }

tests/php/Control/Email/EmailTest/EmailSubClass.php

@@ -0,0 +1,11 @@
+<?php
+
+namespace SilverStripe\Control\Tests\Email\EmailTest;
+
+use SilverStripe\Control\Email\Email;
+use SilverStripe\Dev\TestOnly;
+
+class EmailSubClass extends Email implements TestOnly
+{
+
+}

tests/php/Control/Email/EmailTest/templates/SilverStripe/Control/Tests/Email/EmailTest/EmailSubClass.ss

@@ -0,0 +1,14 @@
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
+<html>
+<head>
+	<% base_tag %>
+</head>
+<body>
+
+<div class="body">
+    <h1>Email Sub-class</h1>
+	$EmailContent
+</div>
+
+</body>
+</html>