From e211e27470ec4ff21ccc30656741458d5344df2c Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 14:27:57 +0200
Subject: [PATCH 01/19] Add more unit tests for
 DebugViewFriendlyErrorFormatter, tidy up Director::is_ajax() return

---
 src/Control/Director.php                      | 10 +--
 .../DebugViewFriendlyErrorFormatter.php       |  7 ++-
 .../DebugViewFriendlyErrorFormatterTest.php   | 61 ++++++++++++++++++-
 3 files changed, 69 insertions(+), 9 deletions(-)

diff --git a/src/Control/Director.php b/src/Control/Director.php
index 8b85e1fee..86491bd05 100644
--- a/src/Control/Director.php
+++ b/src/Control/Director.php
@@ -1004,12 +1004,12 @@ class Director implements TemplateGlobalProvider
         $request = self::currentRequest($request);
         if ($request) {
             return $request->isAjax();
-        } else {
-            return (
-                isset($_REQUEST['ajax']) ||
-                (isset($_SERVER['HTTP_X_REQUESTED_WITH']) && $_SERVER['HTTP_X_REQUESTED_WITH'] == "XMLHttpRequest")
-            );
         }
+
+        return (
+            isset($_REQUEST['ajax']) ||
+            (isset($_SERVER['HTTP_X_REQUESTED_WITH']) && $_SERVER['HTTP_X_REQUESTED_WITH'] == "XMLHttpRequest")
+        );
     }
 
     /**
diff --git a/src/Logging/DebugViewFriendlyErrorFormatter.php b/src/Logging/DebugViewFriendlyErrorFormatter.php
index fca78fcf4..7356076c4 100644
--- a/src/Logging/DebugViewFriendlyErrorFormatter.php
+++ b/src/Logging/DebugViewFriendlyErrorFormatter.php
@@ -97,7 +97,7 @@ class DebugViewFriendlyErrorFormatter implements FormatterInterface
     public function format(array $record)
     {
         // Get error code
-        $code = empty($record['code']) ? $this->statusCode : $record['code'];
+        $code = empty($record['code']) ? $this->getStatusCode() : $record['code'];
         return $this->output($code);
     }
 
@@ -127,8 +127,9 @@ class DebugViewFriendlyErrorFormatter implements FormatterInterface
         $output = $renderer->renderHeader();
         $output .= $renderer->renderInfo("Website Error", $this->getTitle(), $this->getBody());
 
-        if (Email::config()->admin_email) {
-            $mailto = Email::obfuscate(Email::config()->admin_email);
+        $adminEmail = Email::config()->get('admin_email');
+        if ($adminEmail) {
+            $mailto = Email::obfuscate($adminEmail);
             $output .= $renderer->renderParagraph('Contact an administrator: ' . $mailto . '');
         }
 
diff --git a/tests/php/Logging/DebugViewFriendlyErrorFormatterTest.php b/tests/php/Logging/DebugViewFriendlyErrorFormatterTest.php
index 49d834457..04f2c64c1 100644
--- a/tests/php/Logging/DebugViewFriendlyErrorFormatterTest.php
+++ b/tests/php/Logging/DebugViewFriendlyErrorFormatterTest.php
@@ -2,18 +2,66 @@
 
 namespace SilverStripe\Logging\Tests;
 
+use PHPUnit_Framework_MockObject_MockObject;
 use SilverStripe\Control\Email\Email;
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Core\Injector\Injector;
+use SilverStripe\Dev\DebugView;
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\Logging\DebugViewFriendlyErrorFormatter;
 
 class DebugViewFriendlyErrorFormatterTest extends SapphireTest
 {
-    public function setUp()
+    protected function setUp()
     {
         parent::setUp();
         Email::config()->set('admin_email', 'testy@mctest.face');
     }
 
+    public function testFormatPassesRecordCodeToOutput()
+    {
+        /** @var DebugViewFriendlyErrorFormatter|PHPUnit_Framework_MockObject_MockObject $mock */
+        $mock = $this->getMockBuilder(DebugViewFriendlyErrorFormatter::class)
+            ->setMethods(['output'])
+            ->getMock();
+
+        $mock->expects($this->once())->method('output')->with(403)->willReturn('foo');
+        $this->assertSame('foo', $mock->format(['code' => 403]));
+    }
+
+    public function testFormatPassesInstanceStatusCodeToOutputWhenNotProvidedByRecord()
+    {
+        /** @var DebugViewFriendlyErrorFormatter|PHPUnit_Framework_MockObject_MockObject $mock */
+        $mock = $this->getMockBuilder(DebugViewFriendlyErrorFormatter::class)
+            ->setMethods(['output'])
+            ->getMock();
+
+        $mock->setStatusCode(404);
+
+        $mock->expects($this->once())->method('output')->with(404)->willReturn('foo');
+        $this->assertSame('foo', $mock->format(['notacode' => 'bar']));
+    }
+
+    public function testFormatBatch()
+    {
+        $records = [
+            ['message' => 'bar'],
+            ['open' => 'sausage'],
+            ['horse' => 'caballo'],
+        ];
+
+        /** @var DebugViewFriendlyErrorFormatter|PHPUnit_Framework_MockObject_MockObject $mock */
+        $mock = $this->getMockBuilder(DebugViewFriendlyErrorFormatter::class)
+            ->setMethods(['format'])
+            ->getMock();
+
+        $mock->expects($this->exactly(3))
+            ->method('format')
+            ->willReturn('foo');
+
+        $this->assertSame('foofoofoo', $mock->formatBatch($records));
+    }
+
     public function testOutput()
     {
         $formatter = new DebugViewFriendlyErrorFormatter();
@@ -34,4 +82,15 @@ TEXT
 
         $this->assertEquals($expected, $formatter->output(404));
     }
+
+    public function testOutputReturnsTitleWhenRequestIsAjax()
+    {
+        // Mock an AJAX request
+        Injector::inst()->registerService(new HTTPRequest('GET', '', ['ajax' => true]));
+
+        $formatter = new DebugViewFriendlyErrorFormatter();
+        $formatter->setTitle('The Diary of Anne Frank');
+
+        $this->assertSame('The Diary of Anne Frank', $formatter->output(200));
+    }
 }

From 73df3166b7d3eac4a3b7b4600a6033f668b8ca77 Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 14:30:15 +0200
Subject: [PATCH 02/19] Switch to short array syntax in DetailedErrorFormatter,
 add spaces between array values

---
 src/Logging/DetailedErrorFormatter.php | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/Logging/DetailedErrorFormatter.php b/src/Logging/DetailedErrorFormatter.php
index 2d47bd999..4b845d5d6 100644
--- a/src/Logging/DetailedErrorFormatter.php
+++ b/src/Logging/DetailedErrorFormatter.php
@@ -25,7 +25,7 @@ class DetailedErrorFormatter implements FormatterInterface
             );
         } else {
             $context = isset($record['context']) ? $record['context'] : $record;
-            foreach (array('code','message','file','line') as $key) {
+            foreach (['code', 'message', 'file', 'line'] as $key) {
                 if (!isset($context[$key])) {
                     $context[$key] = isset($record[$key]) ? $record[$key] : null;
                 }
@@ -57,7 +57,7 @@ class DetailedErrorFormatter implements FormatterInterface
 
     public function formatBatch(array $records)
     {
-        return implode("\n", array_map(array($this, 'format'), $records));
+        return implode("\n", array_map([$this, 'format'], $records));
     }
 
     /**

From 2694a47c45a01ae5f7228a61f665d0b15f300b31 Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 14:41:45 +0200
Subject: [PATCH 03/19] Add more tests for DetailedErrorFormatter

---
 .../DebugViewFriendlyErrorFormatterTest.php   |  1 -
 .../Logging/DetailedErrorFormatterTest.php    | 46 ++++++++++++++++++-
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/tests/php/Logging/DebugViewFriendlyErrorFormatterTest.php b/tests/php/Logging/DebugViewFriendlyErrorFormatterTest.php
index 04f2c64c1..0b40bcf38 100644
--- a/tests/php/Logging/DebugViewFriendlyErrorFormatterTest.php
+++ b/tests/php/Logging/DebugViewFriendlyErrorFormatterTest.php
@@ -6,7 +6,6 @@ use PHPUnit_Framework_MockObject_MockObject;
 use SilverStripe\Control\Email\Email;
 use SilverStripe\Control\HTTPRequest;
 use SilverStripe\Core\Injector\Injector;
-use SilverStripe\Dev\DebugView;
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\Logging\DebugViewFriendlyErrorFormatter;
 
diff --git a/tests/php/Logging/DetailedErrorFormatterTest.php b/tests/php/Logging/DetailedErrorFormatterTest.php
index 40f9b75a7..b36786f81 100644
--- a/tests/php/Logging/DetailedErrorFormatterTest.php
+++ b/tests/php/Logging/DetailedErrorFormatterTest.php
@@ -8,7 +8,7 @@ use SilverStripe\Logging\Tests\DetailedErrorFormatterTest\ErrorGenerator;
 
 class DetailedErrorFormatterTest extends SapphireTest
 {
-    public function testFormat()
+    public function testFormatWithException()
     {
         $generator = new ErrorGenerator();
         $formatter = new DetailedErrorFormatter();
@@ -27,4 +27,48 @@ class DetailedErrorFormatterTest extends SapphireTest
             $output
         );
     }
+
+    public function testFormatWithoutException()
+    {
+        $record = [
+            'code' => 401,
+            'message' => 'Denied',
+            'file' => 'index.php',
+            'line' => 4,
+        ];
+
+        $formatter = new DetailedErrorFormatter();
+        $result = $formatter->format($record);
+
+        $this->assertContains('ERRNO 401', $result, 'Status code was not found in trace');
+        $this->assertContains('Denied', $result, 'Message was not found in trace');
+        $this->assertContains('Line 4 in index.php', $result, 'Line or filename were not found in trace');
+        $this->assertContains(self::class, $result, 'Backtrace doesn\'t show current test class');
+    }
+
+    public function testFormatBatch()
+    {
+        $records = [
+            [
+                'code' => 401,
+                'message' => 'Denied',
+                'file' => 'index.php',
+                'line' => 4,
+            ],
+            [
+                'code' => 404,
+                'message' => 'Not found',
+                'file' => 'admin.php',
+                'line' => 7,
+            ],
+        ];
+
+        $formatter = new DetailedErrorFormatter();
+        $result = $formatter->formatBatch($records);
+
+        $this->assertContains('ERRNO 401', $result, 'First status code was not found in trace');
+        $this->assertContains('ERRNO 404', $result, 'Second status code was not found in trace');
+        $this->assertContains('Denied', $result, 'First message was not found in trace');
+        $this->assertContains('Not found', $result, 'Second message was not found in trace');
+    }
 }

From 9911c9c9ef0d78398f31431964d58a3055679045 Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 14:51:08 +0200
Subject: [PATCH 04/19] Use single quotes and getters over direct prop access
 in HTTPOutputHandler

---
 src/Logging/HTTPOutputHandler.php | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/Logging/HTTPOutputHandler.php b/src/Logging/HTTPOutputHandler.php
index 206039cd1..45f51c785 100644
--- a/src/Logging/HTTPOutputHandler.php
+++ b/src/Logging/HTTPOutputHandler.php
@@ -18,7 +18,7 @@ class HTTPOutputHandler extends AbstractProcessingHandler
     /**
      * @var string
      */
-    private $contentType = "text/html";
+    private $contentType = 'text/html';
 
     /**
      * @var int
@@ -155,8 +155,8 @@ class HTTPOutputHandler extends AbstractProcessingHandler
 
         // If headers have been sent then these won't be used, and may throw errors that we wont' want to see.
         if (!headers_sent()) {
-            $response->setStatusCode($this->statusCode);
-            $response->addHeader("Content-Type", $this->contentType);
+            $response->setStatusCode($this->getStatusCode());
+            $response->addHeader('Content-Type', $this->getContentType());
         } else {
             // To supress errors aboot errors
             $response->setStatusCode(200);
@@ -165,6 +165,6 @@ class HTTPOutputHandler extends AbstractProcessingHandler
         $response->setBody($record['formatted']);
         $response->output();
 
-        return false === $this->bubble;
+        return false === $this->getBubble();
     }
 }

From 5bd05a2deba610e67836c1f6f292ce02b6aa743a Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 14:51:53 +0200
Subject: [PATCH 05/19] Reduce setUp visibility and remove check for CLI -
 tests always run on CLI now

---
 tests/php/Logging/HTTPOutputHandlerTest.php | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/tests/php/Logging/HTTPOutputHandlerTest.php b/tests/php/Logging/HTTPOutputHandlerTest.php
index 352fdf13d..198361bf5 100644
--- a/tests/php/Logging/HTTPOutputHandlerTest.php
+++ b/tests/php/Logging/HTTPOutputHandlerTest.php
@@ -3,7 +3,6 @@
 namespace SilverStripe\Logging\Tests;
 
 use Monolog\Handler\HandlerInterface;
-use PhpParser\Node\Scalar\MagicConst\Dir;
 use SilverStripe\Control\Director;
 use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Dev\SapphireTest;
@@ -13,14 +12,12 @@ use SilverStripe\Logging\HTTPOutputHandler;
 
 class HTTPOutputHandlerTest extends SapphireTest
 {
-    public function setUp()
+    protected function setUp()
     {
         parent::setUp();
-        if (!Director::is_cli()) {
-            $this->markTestSkipped("This test only runs in CLI mode");
-        }
+
         if (!Director::isDev()) {
-            $this->markTestSkipped("This test only runs in dev mode");
+            $this->markTestSkipped('This test only runs in dev mode');
         }
     }
 

From 3cdb73bd44376161c79e0ccd38394f52203458c7 Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 15:00:08 +0200
Subject: [PATCH 06/19] NEW Add getLogger() to MonologErrorHandler and add test
 for exception without one

---
 src/Logging/MonologErrorHandler.php           | 16 ++++++++++++++--
 tests/php/Logging/MonologErrorHandlerTest.php | 19 +++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)
 create mode 100644 tests/php/Logging/MonologErrorHandlerTest.php

diff --git a/src/Logging/MonologErrorHandler.php b/src/Logging/MonologErrorHandler.php
index 8d2ccdb80..3b5e7727f 100644
--- a/src/Logging/MonologErrorHandler.php
+++ b/src/Logging/MonologErrorHandler.php
@@ -16,19 +16,31 @@ class MonologErrorHandler implements ErrorHandler
      * Set the PSR-3 logger to send errors & exceptions to
      *
      * @param LoggerInterface $logger
+     * @return $this
      */
     public function setLogger(LoggerInterface $logger)
     {
         $this->logger = $logger;
+        return $this;
+    }
+
+    /**
+     * Get the PSR-3 logger to send errors & exceptions to
+     *
+     * @return LoggerInterface
+     */
+    public function getLogger()
+    {
+        return $this->logger;
     }
 
     public function start()
     {
-        if (!$this->logger) {
+        if (!$this->getLogger()) {
             throw new \InvalidArgumentException("No Logger property passed to MonologErrorHandler."
                 . "Is your Injector config correct?");
         }
 
-        MonologHandler::register($this->logger);
+        MonologHandler::register($this->getLogger());
     }
 }
diff --git a/tests/php/Logging/MonologErrorHandlerTest.php b/tests/php/Logging/MonologErrorHandlerTest.php
new file mode 100644
index 000000000..c7ec4628e
--- /dev/null
+++ b/tests/php/Logging/MonologErrorHandlerTest.php
@@ -0,0 +1,19 @@
+<?php
+
+namespace SilverStripe\Logging\Tests;
+
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Logging\MonologErrorHandler;
+
+class MonologErrorHandlerTest extends SapphireTest
+{
+    /**
+     * @expectedException \InvalidArgumentException
+     * @expectedExceptionMessageRegExp /No Logger property passed to MonologErrorHandler/
+     */
+    public function testStartThrowsExceptionWithoutLoggerDefined()
+    {
+        $handler = new MonologErrorHandler();
+        $handler->start();
+    }
+}

From 60b375d99500284e71c2c60fbda583f4c4a9cd7b Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 15:18:34 +0200
Subject: [PATCH 07/19] Add more tests for CheckboxField_Readonly and
 CompositeField, improve PHPDocs

---
 src/Forms/CompositeField.php                  |  10 +-
 .../php/Forms/CheckboxField_ReadonlyTest.php  |  17 ++
 tests/php/Forms/CompositeFieldTest.php        | 150 +++++++++++++++++-
 3 files changed, 171 insertions(+), 6 deletions(-)
 create mode 100644 tests/php/Forms/CheckboxField_ReadonlyTest.php

diff --git a/src/Forms/CompositeField.php b/src/Forms/CompositeField.php
index e86b55fd8..e02a3c9df 100644
--- a/src/Forms/CompositeField.php
+++ b/src/Forms/CompositeField.php
@@ -21,11 +21,13 @@ class CompositeField extends FormField
 
     /**
      * Set to true when this field is a readonly field
+     *
+     * @var bool
      */
     protected $readonly;
 
     /**
-     * @var $columnCount int Toggle different css-rendering for multiple columns
+     * @var int Toggle different css-rendering for multiple columns
      * ("onecolumn", "twocolumns", "threecolumns"). The content is determined
      * by the $children-array, so wrap all items you want to have grouped in a
      * column inside a CompositeField.
@@ -35,12 +37,12 @@ class CompositeField extends FormField
     protected $columnCount = null;
 
     /**
-     * @var String custom HTML tag to render with, e.g. to produce a <fieldset>.
+     * @var string custom HTML tag to render with, e.g. to produce a <fieldset>.
      */
     protected $tag = 'div';
 
     /**
-     * @var String Optional description for this set of fields.
+     * @var string Optional description for this set of fields.
      * If the {@link $tag} property is set to use a 'fieldset', this will be
      * rendered as a <legend> tag, otherwise its a 'title' attribute.
      */
@@ -214,7 +216,7 @@ class CompositeField extends FormField
                 'tabindex' => null,
                 'type' => null,
                 'value' => null,
-                'title' => ($this->tag == 'fieldset') ? null : $this->legend
+                'title' => ($this->tag === 'fieldset') ? null : $this->legend
             )
         );
     }
diff --git a/tests/php/Forms/CheckboxField_ReadonlyTest.php b/tests/php/Forms/CheckboxField_ReadonlyTest.php
new file mode 100644
index 000000000..4996deda8
--- /dev/null
+++ b/tests/php/Forms/CheckboxField_ReadonlyTest.php
@@ -0,0 +1,17 @@
+<?php
+
+namespace SilverStripe\Forms\Tests;
+
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Forms\CheckboxField_Readonly;
+
+class CheckboxField_ReadonlyTest extends SapphireTest
+{
+    public function testPerformReadonlyTransformation()
+    {
+        $field = new CheckboxField_Readonly('Test');
+        $result = $field->performReadonlyTransformation();
+        $this->assertInstanceOf(CheckboxField_Readonly::class, $result);
+        $this->assertNotSame($result, $field);
+    }
+}
diff --git a/tests/php/Forms/CompositeFieldTest.php b/tests/php/Forms/CompositeFieldTest.php
index 6a4629fd8..382c339dc 100644
--- a/tests/php/Forms/CompositeFieldTest.php
+++ b/tests/php/Forms/CompositeFieldTest.php
@@ -2,13 +2,14 @@
 
 namespace SilverStripe\Forms\Tests;
 
+use PHPUnit_Framework_Error;
 use SilverStripe\Dev\CSSContentParser;
 use SilverStripe\Dev\SapphireTest;
-use SilverStripe\Forms\FieldList;
-use SilverStripe\Forms\TextField;
 use SilverStripe\Forms\CompositeField;
 use SilverStripe\Forms\DropdownField;
+use SilverStripe\Forms\FieldList;
 use SilverStripe\Forms\RequiredFields;
+use SilverStripe\Forms\TextField;
 
 class CompositeFieldTest extends SapphireTest
 {
@@ -36,6 +37,9 @@ class CompositeFieldTest extends SapphireTest
         $this->assertEquals(0, $compositeOuter->fieldPosition('A'));
         $this->assertEquals(1, $compositeOuter->fieldPosition('AB'));
         $this->assertEquals(2, $compositeOuter->fieldPosition('B'));
+
+        $this->assertFalse($compositeOuter->fieldPosition(null), 'Falsy input should return false');
+        $this->assertFalse($compositeOuter->fieldPosition('FOO'), 'Non-exitent child should return false');
     }
 
     public function testTag()
@@ -124,4 +128,146 @@ class CompositeFieldTest extends SapphireTest
         $this->assertEquals($expectedChildren, $field->getChildren());
         $this->assertEquals($field, $expectedChildren->getContainerField());
     }
+
+    public function testExtraClass()
+    {
+        $field = CompositeField::create();
+        $field->setColumnCount(3);
+        $result = $field->extraClass();
+
+        $this->assertContains('field', $result, 'Default class was not added');
+        $this->assertContains('CompositeField', $result, 'Default class was not added');
+        $this->assertContains('multicolumn', $result, 'Multi column field did not have extra class added');
+    }
+
+    public function testGetAttributes()
+    {
+        $field = CompositeField::create();
+        $field->setLegend('test');
+        $result = $field->getAttributes();
+
+        $this->assertNull($result['tabindex']);
+        $this->assertNull($result['type']);
+        $this->assertNull($result['value']);
+        $this->assertSame('test', $result['title']);
+    }
+
+    public function testGetAttributesReturnsEmptyTitleForFieldSets()
+    {
+        $field = CompositeField::create();
+        $field->setLegend('not used');
+        $field->setTag('fieldset');
+        $result = $field->getAttributes();
+        $this->assertNull($result['title']);
+    }
+
+    /**
+     * @expectedException PHPUnit_Framework_Error
+     * @expectedExceptionMessageRegExp /a field called 'Test' appears twice in your form.*TextField.*TextField/
+     */
+    public function testCollateDataFieldsThrowsErrorOnDuplicateChildren()
+    {
+        $field = CompositeField::create(
+            TextField::create('Test'),
+            TextField::create('Test')
+        );
+
+        $list = [];
+        $field->collateDataFields($list);
+    }
+
+    public function testCollateDataFieldsWithSaveableOnly()
+    {
+        $field = CompositeField::create(
+            TextField::create('Test')
+                ->setReadonly(false)
+                ->setDisabled(true)
+        );
+
+        $list = [];
+        $field->collateDataFields($list, true);
+        $this->assertEmpty($list, 'Unsaveable fields should not be collated when $saveableOnly = true');
+
+        $field->collateDataFields($list, false);
+        $this->assertNotEmpty($list, 'Unsavable fields should be collated when $saveableOnly = false');
+    }
+
+    public function testSetDisabledPropagatesToChildren()
+    {
+        $field = CompositeField::create(
+            $testField = TextField::create('Test')
+                ->setDisabled(false)
+        )->setDisabled(true);
+        $this->assertTrue($field->fieldByName('Test')->isDisabled(), 'Children should also be set to disabled');
+    }
+
+    public function testIsComposite()
+    {
+        $this->assertTrue(CompositeField::create()->isComposite());
+    }
+
+    public function testMakeFieldReadonlyPassedFieldName()
+    {
+        $field = CompositeField::create(
+            TextField::create('Test')->setDisabled(false)
+        );
+
+        $this->assertFalse($field->fieldByName('Test')->isReadonly());
+        $this->assertTrue($field->makeFieldReadonly('Test'), 'makeFieldReadonly should return true');
+        $this->assertTrue($field->fieldByName('Test')->isReadonly(), 'Named child field should be made readonly');
+    }
+
+    public function testMakeFieldReadonlyPassedFormField()
+    {
+        $field = CompositeField::create(
+            $testField = TextField::create('Test')->setDisabled(false)
+        );
+
+        $this->assertFalse($field->fieldByName('Test')->isReadonly());
+        $this->assertTrue($field->makeFieldReadonly($testField), 'makeFieldReadonly should return true');
+        $this->assertTrue($field->fieldByName('Test')->isReadonly(), 'Named child field should be made readonly');
+    }
+
+    public function testMakeFieldReadonlyWithNestedCompositeFields()
+    {
+        $field = CompositeField::create(
+            CompositeField::create(
+                TextField::create('Test')->setDisabled(false)
+            )
+        );
+
+        $this->assertFalse($field->getChildren()->first()->fieldByName('Test')->isReadonly());
+        $this->assertTrue($field->makeFieldReadonly('Test'), 'makeFieldReadonly should return true');
+        $this->assertTrue(
+            $field->getChildren()->first()->fieldByName('Test')->isReadonly(),
+            'Named child field should be made readonly'
+        );
+    }
+
+    public function testMakeFieldReadonlyReturnsFalseWhenFieldNotFound()
+    {
+        $field = CompositeField::create(
+            CompositeField::create(
+                TextField::create('Test')
+            )
+        );
+
+        $this->assertFalse(
+            $field->makeFieldReadonly('NonExistent'),
+            'makeFieldReadonly should return false when field is not found'
+        );
+    }
+
+    public function testDebug()
+    {
+        $field = new CompositeField(
+            new TextField('TestTextField')
+        );
+        $field->setName('TestComposite');
+
+        $result = $field->debug();
+        $this->assertContains(CompositeField::class . ' (TestComposite)', $result);
+        $this->assertContains('TestTextField', $result);
+        $this->assertContains('<ul', $result, 'Result should be formatted as a <ul>');
+    }
 }

From c418ee2915634ba4277688589a1ecf1d89fa6fba Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 16:34:36 +0200
Subject: [PATCH 08/19] NEW Add getters and setters for public properties in
 ConfirmPasswordField, add tests

Some of the validation parts of ConfirmPasswordField are previously untested, this adds tests
---
 src/Forms/ConfirmedPasswordField.php          | 168 +++++++++++++-----
 .../php/Forms/ConfirmedPasswordFieldTest.php  | 155 +++++++++++++---
 2 files changed, 256 insertions(+), 67 deletions(-)

diff --git a/src/Forms/ConfirmedPasswordField.php b/src/Forms/ConfirmedPasswordField.php
index 0b14bd037..e4b3840ec 100644
--- a/src/Forms/ConfirmedPasswordField.php
+++ b/src/Forms/ConfirmedPasswordField.php
@@ -140,12 +140,12 @@ class ConfirmedPasswordField extends FormField
         $title = isset($title) ? $title : _t('SilverStripe\\Security\\Member.PASSWORD', 'Password');
 
         // naming with underscores to prevent values from actually being saved somewhere
-        $this->children = new FieldList(
-            $this->passwordField = new PasswordField(
+        $this->children = FieldList::create(
+            $this->passwordField = PasswordField::create(
                 "{$name}[_Password]",
                 $title
             ),
-            $this->confirmPasswordfield = new PasswordField(
+            $this->confirmPasswordfield = PasswordField::create(
                 "{$name}[_ConfirmPassword]",
                 (isset($titleConfirmField)) ? $titleConfirmField : _t('SilverStripe\\Security\\Member.CONFIRMPASSWORD', 'Confirm Password')
             )
@@ -153,11 +153,11 @@ class ConfirmedPasswordField extends FormField
 
         // has to be called in constructor because Field() isn't triggered upon saving the instance
         if ($showOnClick) {
-            $this->children->push($this->hiddenField = new HiddenField("{$name}[_PasswordFieldVisible]"));
+            $this->getChildren()->push($this->hiddenField = HiddenField::create("{$name}[_PasswordFieldVisible]"));
         }
 
         // disable auto complete
-        foreach ($this->children as $child) {
+        foreach ($this->getChildren() as $child) {
             /** @var FormField $child */
             $child->setAttribute('autocomplete', 'off');
         }
@@ -176,8 +176,8 @@ class ConfirmedPasswordField extends FormField
 
     public function setTitle($title)
     {
-        parent::setTitle($title);
-        $this->passwordField->setTitle($title);
+        $this->getPasswordField()->setTitle($title);
+        return parent::setTitle($title);
     }
 
     /**
@@ -189,7 +189,7 @@ class ConfirmedPasswordField extends FormField
     {
         // Build inner content
         $fieldContent = '';
-        foreach ($this->children as $field) {
+        foreach ($this->getChildren() as $field) {
             /** @var FormField $field */
             $field->setDisabled($this->isDisabled());
             $field->setReadonly($this->isReadonly());
@@ -207,8 +207,8 @@ class ConfirmedPasswordField extends FormField
             return $fieldContent;
         }
 
-        if ($this->showOnClickTitle) {
-            $title = $this->showOnClickTitle;
+        if ($this->getShowOnClickTitle()) {
+            $title = $this->getShowOnClickTitle();
         } else {
             $title = _t(
                 __CLASS__ . '.SHOWONCLICKTITLE',
@@ -286,11 +286,11 @@ class ConfirmedPasswordField extends FormField
     /**
      * @param string $title
      *
-     * @return ConfirmedPasswordField
+     * @return $this
      */
     public function setRightTitle($title)
     {
-        foreach ($this->children as $field) {
+        foreach ($this->getChildren() as $field) {
             /** @var FormField $field */
             $field->setRightTitle($title);
         }
@@ -310,8 +310,8 @@ class ConfirmedPasswordField extends FormField
     public function setChildrenTitles($titles)
     {
         $expectedChildren = $this->getRequireExistingPassword() ? 3 : 2;
-        if (is_array($titles) && count($titles) == $expectedChildren) {
-            foreach ($this->children as $field) {
+        if (is_array($titles) && count($titles) === $expectedChildren) {
+            foreach ($this->getChildren() as $field) {
                 if (isset($titles[0])) {
                     /** @var FormField $field */
                     $field->setTitle($titles[0]);
@@ -350,7 +350,7 @@ class ConfirmedPasswordField extends FormField
                 : null;
 
             if ($this->showOnClick && isset($value['_PasswordFieldVisible'])) {
-                $this->children->fieldByName($this->getName() . '[_PasswordFieldVisible]')
+                $this->getChildren()->fieldByName($this->getName() . '[_PasswordFieldVisible]')
                     ->setValue($value['_PasswordFieldVisible']);
             }
         } else {
@@ -362,10 +362,10 @@ class ConfirmedPasswordField extends FormField
 
         //looking up field by name is expensive, so lets check it needs to change
         if ($oldValue != $this->value) {
-            $this->children->fieldByName($this->getName() . '[_Password]')
+            $this->getChildren()->fieldByName($this->getName() . '[_Password]')
                 ->setValue($this->value);
 
-            $this->children->fieldByName($this->getName() . '[_ConfirmPassword]')
+            $this->getChildren()->fieldByName($this->getName() . '[_ConfirmPassword]')
                 ->setValue($this->confirmValue);
         }
 
@@ -380,8 +380,8 @@ class ConfirmedPasswordField extends FormField
      */
     public function setName($name)
     {
-        $this->passwordField->setName($name . '[_Password]');
-        $this->confirmPasswordfield->setName($name . '[_ConfirmPassword]');
+        $this->getPasswordField()->setName($name . '[_Password]');
+        $this->getConfirmPasswordField()->setName($name . '[_ConfirmPassword]');
         if ($this->hiddenField) {
             $this->hiddenField->setName($name . '[_PasswordFieldVisible]');
         }
@@ -417,12 +417,12 @@ class ConfirmedPasswordField extends FormField
             return true;
         }
 
-        $this->passwordField->setValue($this->value);
-        $this->confirmPasswordfield->setValue($this->confirmValue);
-        $value = $this->passwordField->Value();
+        $this->getPasswordField()->setValue($this->value);
+        $this->getConfirmPasswordField()->setValue($this->confirmValue);
+        $value = $this->getPasswordField()->Value();
 
         // both password-fields should be the same
-        if ($value != $this->confirmPasswordfield->Value()) {
+        if ($value != $this->getConfirmPasswordField()->Value()) {
             $validator->validationError(
                 $name,
                 _t('SilverStripe\\Forms\\Form.VALIDATIONPASSWORDSDONTMATCH', "Passwords don't match"),
@@ -434,7 +434,7 @@ class ConfirmedPasswordField extends FormField
 
         if (!$this->canBeEmpty) {
             // both password-fields shouldn't be empty
-            if (!$value || !$this->confirmPasswordfield->Value()) {
+            if (!$value || !$this->getConfirmPasswordField()->Value()) {
                 $validator->validationError(
                     $name,
                     _t('SilverStripe\\Forms\\Form.VALIDATIONPASSWORDSNOTEMPTY', "Passwords can't be empty"),
@@ -446,29 +446,31 @@ class ConfirmedPasswordField extends FormField
         }
 
         // lengths
-        if (($this->minLength || $this->maxLength)) {
+        $minLength = $this->getMinLength();
+        $maxLength = $this->getMaxLength();
+        if ($minLength || $maxLength) {
             $errorMsg = null;
             $limit = null;
-            if ($this->minLength && $this->maxLength) {
-                $limit = "{{$this->minLength},{$this->maxLength}}";
+            if ($minLength && $maxLength) {
+                $limit = "{{$minLength},{$maxLength}}";
                 $errorMsg = _t(
-                    'SilverStripe\\Forms\\ConfirmedPasswordField.BETWEEN',
+                    __CLASS__ . '.BETWEEN',
                     'Passwords must be {min} to {max} characters long.',
-                    array('min' => $this->minLength, 'max' => $this->maxLength)
+                    ['min' => $minLength, 'max' => $maxLength]
                 );
-            } elseif ($this->minLength) {
-                $limit = "{{$this->minLength}}.*";
+            } elseif ($minLength) {
+                $limit = "{{$minLength}}.*";
                 $errorMsg = _t(
-                    'SilverStripe\\Forms\\ConfirmedPasswordField.ATLEAST',
+                    __CLASS__ . '.ATLEAST',
                     'Passwords must be at least {min} characters long.',
-                    array('min' => $this->minLength)
+                    ['min' => $minLength]
                 );
-            } elseif ($this->maxLength) {
-                $limit = "{0,{$this->maxLength}}";
+            } elseif ($maxLength) {
+                $limit = "{0,{$maxLength}}";
                 $errorMsg = _t(
-                    'SilverStripe\\Forms\\ConfirmedPasswordField.MAXIMUM',
+                    __CLASS__ . '.MAXIMUM',
                     'Passwords must be at most {max} characters long.',
-                    array('max' => $this->maxLength)
+                    ['max' => $maxLength]
                 );
             }
             $limitRegex = '/^.' . $limit . '$/';
@@ -478,16 +480,18 @@ class ConfirmedPasswordField extends FormField
                     $errorMsg,
                     "validation"
                 );
+
+                return false;
             }
         }
 
-        if ($this->requireStrongPassword) {
+        if ($this->getRequireStrongPassword()) {
             if (!preg_match('/^(([a-zA-Z]+\d+)|(\d+[a-zA-Z]+))[a-zA-Z0-9]*$/', $value)) {
                 $validator->validationError(
                     $name,
                     _t(
                         'SilverStripe\\Forms\\Form.VALIDATIONSTRONGPASSWORD',
-                        "Passwords must have at least one digit and one alphanumeric character"
+                        'Passwords must have at least one digit and one alphanumeric character'
                     ),
                     "validation"
                 );
@@ -502,8 +506,8 @@ class ConfirmedPasswordField extends FormField
                 $validator->validationError(
                     $name,
                     _t(
-                        'SilverStripe\\Forms\\ConfirmedPasswordField.CURRENT_PASSWORD_MISSING',
-                        "You must enter your current password."
+                        __CLASS__ . '.CURRENT_PASSWORD_MISSING',
+                        'You must enter your current password.'
                     ),
                     "validation"
                 );
@@ -516,7 +520,7 @@ class ConfirmedPasswordField extends FormField
                 $validator->validationError(
                     $name,
                     _t(
-                        'SilverStripe\\Forms\\ConfirmedPasswordField.LOGGED_IN_ERROR',
+                        __CLASS__ . '.LOGGED_IN_ERROR',
                         "You must be logged in to change your password."
                     ),
                     "validation"
@@ -532,7 +536,7 @@ class ConfirmedPasswordField extends FormField
                     $validator->validationError(
                         $name,
                         _t(
-                            'SilverStripe\\Forms\\ConfirmedPasswordField.CURRENT_PASSWORD_ERROR',
+                            __CLASS__ . '.CURRENT_PASSWORD_ERROR',
                             "The current password you have entered is not correct."
                         ),
                         "validation"
@@ -608,10 +612,84 @@ class ConfirmedPasswordField extends FormField
         $currentName = "{$name}[_CurrentPassword]";
         if ($show) {
             $confirmField = PasswordField::create($currentName, _t('SilverStripe\\Security\\Member.CURRENT_PASSWORD', 'Current Password'));
-            $this->children->unshift($confirmField);
+            $this->getChildren()->unshift($confirmField);
         } else {
-            $this->children->removeByName($currentName, true);
+            $this->getChildren()->removeByName($currentName, true);
         }
         return $this;
     }
+
+    /**
+     * @return PasswordField
+     */
+    public function getPasswordField()
+    {
+        return $this->passwordField;
+    }
+
+    /**
+     * @return PasswordField
+     */
+    public function getConfirmPasswordField()
+    {
+        return $this->confirmPasswordfield;
+    }
+
+    /**
+     * Set the minimum length required for passwords
+     *
+     * @param int $minLength
+     * @return $this
+     */
+    public function setMinLength($minLength)
+    {
+        $this->minLength = (int) $minLength;
+        return $this;
+    }
+
+    /**
+     * @return int
+     */
+    public function getMinLength()
+    {
+        return $this->minLength;
+    }
+
+    /**
+     * Set the maximum length required for passwords
+     *
+     * @param int $maxLength
+     * @return $this
+     */
+    public function setMaxLength($maxLength)
+    {
+        $this->maxLength = (int) $maxLength;
+        return $this;
+    }
+
+    /**
+     * @return int
+     */
+    public function getMaxLength()
+    {
+        return $this->maxLength;
+    }
+
+    /**
+     * @param bool $requireStrongPassword
+     * @return $this
+     */
+    public function setRequireStrongPassword($requireStrongPassword)
+    {
+        $this->requireStrongPassword = (bool) $requireStrongPassword;
+        return $this;
+    }
+
+    /**
+     * @return bool
+     */
+    public function getRequireStrongPassword()
+    {
+        return $this->requireStrongPassword;
+    }
 }
diff --git a/tests/php/Forms/ConfirmedPasswordFieldTest.php b/tests/php/Forms/ConfirmedPasswordFieldTest.php
index c034ef0cb..f4fd8b253 100644
--- a/tests/php/Forms/ConfirmedPasswordFieldTest.php
+++ b/tests/php/Forms/ConfirmedPasswordFieldTest.php
@@ -83,33 +83,40 @@ class ConfirmedPasswordFieldTest extends SapphireTest
         $field = new ConfirmedPasswordField(
             'Test',
             'Testing',
-            array(
-            "_Password" => "abc123",
-            "_ConfirmPassword" => "abc123"
-            )
+            [
+                '_Password' => 'abc123',
+                '_ConfirmPassword' => 'abc123',
+            ]
         );
         $validator = new RequiredFields();
-        /** @skipUpgrade */
-        new Form(Controller::curr(), 'Form', new FieldList($field), new FieldList(), $validator);
         $this->assertTrue(
             $field->validate($validator),
-            "Validates when both passwords are the same"
+            'Validates when both passwords are the same'
         );
-        $field->setName("TestNew"); //try changing name of field
+        $field->setName('TestNew'); //try changing name of field
         $this->assertTrue(
             $field->validate($validator),
-            "Validates when field name is changed"
+            'Validates when field name is changed'
         );
         //non-matching password should make the field invalid
-        $field->setValue(
-            array(
-            "_Password" => "abc123",
-            "_ConfirmPassword" => "123abc"
-            )
-        );
+        $field->setValue([
+            '_Password' => 'abc123',
+            '_ConfirmPassword' => '123abc',
+        ]);
         $this->assertFalse(
             $field->validate($validator),
-            "Does not validate when passwords differ"
+            'Does not validate when passwords differ'
+        );
+
+        // Empty passwords should make the field invalid
+        $field->setCanBeEmpty(false);
+        $field->setValue([
+            '_Password' => '',
+            '_ConfirmPassword' => '',
+        ]);
+        $this->assertFalse(
+            $field->validate($validator),
+            'Empty passwords should not be allowed when canBeEmpty is false'
         );
     }
 
@@ -123,17 +130,121 @@ class ConfirmedPasswordFieldTest extends SapphireTest
             new FieldList()
         );
 
-        $form->loadDataFrom(
-            array(
-            'Password' => array(
+        $form->loadDataFrom([
+            'Password' => [
                 '_Password' => '123',
                 '_ConfirmPassword' => '999',
-            )
-            )
-        );
+            ],
+        ]);
 
         $this->assertEquals('123', $field->children->first()->Value());
         $this->assertEquals('999', $field->children->last()->Value());
         $this->assertNotEquals($field->children->first()->Value(), $field->children->last()->Value());
     }
+
+    /**
+     * @param int|null $minLength
+     * @param int|null $maxLength
+     * @param bool $expectValid
+     * @param string $expectedMessage
+     * @dataProvider lengthValidationProvider
+     */
+    public function testLengthValidation($minLength, $maxLength, $expectValid, $expectedMessage = '')
+    {
+        $field = new ConfirmedPasswordField('Test', 'Testing', [
+            '_Password' => 'abc123',
+            '_ConfirmPassword' => 'abc123',
+        ]);
+        $field->setMinLength($minLength)->setMaxLength($maxLength);
+
+        $validator = new RequiredFields();
+        $result = $field->validate($validator);
+
+        $this->assertSame($expectValid, $result, 'Validate method should return its result');
+        $this->assertSame($expectValid, $validator->getResult()->isValid());
+        if ($expectedMessage) {
+            $this->assertContains($expectedMessage, $validator->getResult()->serialize());
+        }
+    }
+
+    /**
+     * @return array[]
+     */
+    public function lengthValidationProvider()
+    {
+        return [
+            'valid: within min and max' => [3, 8, true],
+            'invalid: lower than min with max' => [8, 12, false, 'Passwords must be 8 to 12 characters long'],
+            'valid: greater than min' => [3, null, true],
+            'invalid: lower than min' => [8, null, false, 'Passwords must be at least 8 characters long'],
+            'valid: less than max' => [null, 8, true],
+            'invalid: greater than max' => [null, 4, false, 'Passwords must be at most 4 characters long'],
+
+        ];
+    }
+
+    public function testStrengthValidation()
+    {
+        $field = new ConfirmedPasswordField('Test', 'Testing', [
+            '_Password' => 'abc',
+            '_ConfirmPassword' => 'abc',
+        ]);
+        $field->setRequireStrongPassword(true);
+
+        $validator = new RequiredFields();
+        $result = $field->validate($validator);
+
+        $this->assertFalse($result, 'Validate method should return its result');
+        $this->assertFalse($validator->getResult()->isValid());
+        $this->assertContains(
+            'Passwords must have at least one digit and one alphanumeric character',
+            $validator->getResult()->serialize()
+        );
+    }
+
+    public function testTitle()
+    {
+        $this->assertNull(ConfirmedPasswordField::create('Test')->Title(), 'Should not have it\'s own title');
+    }
+
+    public function testSetTitlePropagatesToPasswordField()
+    {
+        /** @var ConfirmedPasswordField $field */
+        $field = ConfirmedPasswordField::create('Test')
+            ->setTitle('My password');
+
+        $this->assertSame('My password', $field->getPasswordField()->Title());
+    }
+
+    public function testSetRightTitlePropagatesToChildren()
+    {
+        /** @var ConfirmedPasswordField $field */
+        $field = ConfirmedPasswordField::create('Test');
+
+        $this->assertCount(2, $field->getChildren());
+        foreach ($field->getChildren() as $child) {
+            $this->assertEmpty($child->RightTitle());
+        }
+
+        $field->setRightTitle('Please confirm');
+        foreach ($field->getChildren() as $child) {
+            $this->assertSame('Please confirm', $child->RightTitle());
+        }
+    }
+
+    public function testSetChildrenTitles()
+    {
+        /** @var ConfirmedPasswordField $field */
+        $field = ConfirmedPasswordField::create('Test');
+        $field->setRequireExistingPassword(true);
+        $field->setChildrenTitles([
+            'Current Password',
+            'Password',
+            'Confirm Password',
+        ]);
+
+        $this->assertSame('Current Password', $field->getChildren()->shift()->Title());
+        $this->assertSame('Password', $field->getChildren()->shift()->Title());
+        $this->assertSame('Confirm Password', $field->getChildren()->shift()->Title());
+    }
 }

From 8929b8204fdb9a9d1a48331feb38c2b4ba48ab1e Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 17:18:31 +0200
Subject: [PATCH 09/19] More validation tests for ConfirmedPasswordField

---
 src/Forms/ConfirmedPasswordField.php          |   2 +-
 .../php/Forms/ConfirmedPasswordFieldTest.php  | 112 +++++++++++++++++-
 2 files changed, 108 insertions(+), 6 deletions(-)

diff --git a/src/Forms/ConfirmedPasswordField.php b/src/Forms/ConfirmedPasswordField.php
index e4b3840ec..dd022be49 100644
--- a/src/Forms/ConfirmedPasswordField.php
+++ b/src/Forms/ConfirmedPasswordField.php
@@ -573,7 +573,7 @@ class ConfirmedPasswordField extends FormField
     public function performReadonlyTransformation()
     {
         /** @var ReadonlyField $field */
-        $field = $this->castedCopy('SilverStripe\\Forms\\ReadonlyField')
+        $field = $this->castedCopy(ReadonlyField::class)
             ->setTitle($this->title ? $this->title : _t('SilverStripe\\Security\\Member.PASSWORD', 'Password'))
             ->setValue('*****');
 
diff --git a/tests/php/Forms/ConfirmedPasswordFieldTest.php b/tests/php/Forms/ConfirmedPasswordFieldTest.php
index f4fd8b253..d5b7218d5 100644
--- a/tests/php/Forms/ConfirmedPasswordFieldTest.php
+++ b/tests/php/Forms/ConfirmedPasswordFieldTest.php
@@ -7,12 +7,12 @@ use SilverStripe\Dev\SapphireTest;
 use SilverStripe\Forms\ConfirmedPasswordField;
 use SilverStripe\Forms\FieldList;
 use SilverStripe\Forms\Form;
+use SilverStripe\Forms\ReadonlyField;
 use SilverStripe\Forms\RequiredFields;
 use SilverStripe\Security\Member;
 
 class ConfirmedPasswordFieldTest extends SapphireTest
 {
-
     public function testSetValue()
     {
         $field = new ConfirmedPasswordField('Test', 'Testing', 'valueA');
@@ -25,6 +25,9 @@ class ConfirmedPasswordFieldTest extends SapphireTest
         $this->assertEquals('valueB', $field->children->fieldByName($field->getName() . '[_ConfirmPassword]')->Value());
     }
 
+    /**
+     * @useDatabase true
+     */
     public function testHashHidden()
     {
         $field = new ConfirmedPasswordField('Password', 'Password', 'valueA');
@@ -202,6 +205,73 @@ class ConfirmedPasswordFieldTest extends SapphireTest
         );
     }
 
+    public function testCurrentPasswordValidation()
+    {
+        $field = new ConfirmedPasswordField('Test', 'Testing', [
+            '_Password' => 'abc',
+            '_ConfirmPassword' => 'abc',
+        ]);
+        $field->setRequireExistingPassword(true);
+
+        $validator = new RequiredFields();
+        $result = $field->validate($validator);
+
+        $this->assertFalse($result, 'Validate method should return its result');
+        $this->assertFalse($validator->getResult()->isValid());
+        $this->assertContains(
+            'You must enter your current password',
+            $validator->getResult()->serialize()
+        );
+    }
+
+    public function testMustBeLoggedInToChangePassword()
+    {
+        $field = new ConfirmedPasswordField('Test', 'Testing');
+        $field->setRequireExistingPassword(true);
+        $field->setValue([
+            '_CurrentPassword' => 'foo',
+            '_Password' => 'abc',
+            '_ConfirmPassword' => 'abc',
+        ]);
+
+        $validator = new RequiredFields();
+        $this->logOut();
+        $result = $field->validate($validator);
+
+        $this->assertFalse($result, 'Validate method should return its result');
+        $this->assertFalse($validator->getResult()->isValid());
+        $this->assertContains(
+            'You must be logged in to change your password',
+            $validator->getResult()->serialize()
+        );
+    }
+
+    /**
+     * @useDatabase true
+     */
+    public function testValidateCorrectPassword()
+    {
+        $this->logInWithPermission('ADMIN');
+
+        $field = new ConfirmedPasswordField('Test', 'Testing');
+        $field->setRequireExistingPassword(true);
+        $field->setValue([
+            '_CurrentPassword' => 'foo-not-going-to-be-the-correct-password',
+            '_Password' => 'abc',
+            '_ConfirmPassword' => 'abc',
+        ]);
+
+        $validator = new RequiredFields();
+        $result = $field->validate($validator);
+
+        $this->assertFalse($result, 'Validate method should return its result');
+        $this->assertFalse($validator->getResult()->isValid());
+        $this->assertContains(
+            'The current password you have entered is not correct',
+            $validator->getResult()->serialize()
+        );
+    }
+
     public function testTitle()
     {
         $this->assertNull(ConfirmedPasswordField::create('Test')->Title(), 'Should not have it\'s own title');
@@ -218,8 +288,7 @@ class ConfirmedPasswordFieldTest extends SapphireTest
 
     public function testSetRightTitlePropagatesToChildren()
     {
-        /** @var ConfirmedPasswordField $field */
-        $field = ConfirmedPasswordField::create('Test');
+        $field = new ConfirmedPasswordField('Test');
 
         $this->assertCount(2, $field->getChildren());
         foreach ($field->getChildren() as $child) {
@@ -234,8 +303,7 @@ class ConfirmedPasswordFieldTest extends SapphireTest
 
     public function testSetChildrenTitles()
     {
-        /** @var ConfirmedPasswordField $field */
-        $field = ConfirmedPasswordField::create('Test');
+        $field = new ConfirmedPasswordField('Test');
         $field->setRequireExistingPassword(true);
         $field->setChildrenTitles([
             'Current Password',
@@ -247,4 +315,38 @@ class ConfirmedPasswordFieldTest extends SapphireTest
         $this->assertSame('Password', $field->getChildren()->shift()->Title());
         $this->assertSame('Confirm Password', $field->getChildren()->shift()->Title());
     }
+
+    public function testPerformReadonlyTransformation()
+    {
+        $field = new ConfirmedPasswordField('Test', 'Change it');
+        $result = $field->performReadonlyTransformation();
+
+        $this->assertInstanceOf(ReadonlyField::class, $result);
+        $this->assertSame('Change it', $result->Title());
+        $this->assertContains('***', $result->Value());
+    }
+
+    public function testPerformDisabledTransformation()
+    {
+        $field = new ConfirmedPasswordField('Test', 'Change it');
+        $result = $field->performDisabledTransformation();
+
+        $this->assertInstanceOf(ReadonlyField::class, $result);
+    }
+
+    public function testSetRequireExistingPasswordOnlyRunsOnce()
+    {
+        $field = new ConfirmedPasswordField('Test', 'Change it');
+
+        $this->assertCount(2, $field->getChildren());
+
+        $field->setRequireExistingPassword(true);
+        $this->assertCount(3, $field->getChildren(), 'Current password field was not pushed');
+
+        $field->setRequireExistingPassword(true);
+        $this->assertCount(3, $field->getChildren(), 'Current password field should not be pushed again');
+
+        $field->setRequireExistingPassword(false);
+        $this->assertCount(2, $field->getChildren(), 'Current password field should not be removed');
+    }
 }

From d56bad7568df1b4da0e196c961b7d0d3b310f285 Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 17:33:59 +0200
Subject: [PATCH 10/19] Add tests for edge cases in CurrencyField

---
 src/Forms/CurrencyField.php           |  5 +--
 tests/php/Forms/CurrencyFieldTest.php | 59 +++++++++++++++++++--------
 2 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/src/Forms/CurrencyField.php b/src/Forms/CurrencyField.php
index 80bf1a51e..5e2555c70 100644
--- a/src/Forms/CurrencyField.php
+++ b/src/Forms/CurrencyField.php
@@ -39,9 +39,8 @@ class CurrencyField extends TextField
     {
         if ($this->value) {
             return preg_replace('/[^0-9.\-]/', '', $this->value);
-        } else {
-            return 0.00;
         }
+        return 0.00;
     }
 
     public function Type()
@@ -54,7 +53,7 @@ class CurrencyField extends TextField
      */
     public function performReadonlyTransformation()
     {
-        return $this->castedCopy('SilverStripe\\Forms\\CurrencyField_Readonly');
+        return $this->castedCopy(CurrencyField_Readonly::class);
     }
 
     public function validate($validator)
diff --git a/tests/php/Forms/CurrencyFieldTest.php b/tests/php/Forms/CurrencyFieldTest.php
index a985dffe1..dbb595996 100644
--- a/tests/php/Forms/CurrencyFieldTest.php
+++ b/tests/php/Forms/CurrencyFieldTest.php
@@ -5,6 +5,7 @@ namespace SilverStripe\Forms\Tests;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\Forms\CurrencyField;
+use SilverStripe\Forms\CurrencyField_Readonly;
 use SilverStripe\Forms\RequiredFields;
 use SilverStripe\ORM\FieldType\DBCurrency;
 
@@ -124,56 +125,56 @@ class CurrencyFieldTest extends SapphireTest
         //tests with default currency symbol setting
         $f->setValue('123.45');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '$123.45',
             'Prepends dollar sign to positive decimal'
         );
 
         $f->setValue('-123.45');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '$-123.45',
             'Prepends dollar sign to negative decimal'
         );
 
         $f->setValue('$1');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '$1.00',
             'Formats small value'
         );
 
         $f->setValue('$2.5');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '$2.50',
             'Formats small value'
         );
 
         $f->setValue('$2500000.13');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '$2,500,000.13',
             'Formats large value'
         );
 
         $f->setValue('$2.50000013');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '$2.50',
             'Truncates long decimal portions'
         );
 
         $f->setValue('test123.00test');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '$123.00',
             'Strips alpha values'
         );
 
         $f->setValue('test');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '$0.00',
             'Does not set alpha values'
         );
@@ -183,56 +184,56 @@ class CurrencyFieldTest extends SapphireTest
 
         $f->setValue('123.45');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '€123.45',
             'Prepends dollar sign to positive decimal'
         );
 
         $f->setValue('-123.45');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '€-123.45',
             'Prepends dollar sign to negative decimal'
         );
 
         $f->setValue('€1');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '€1.00',
             'Formats small value'
         );
 
         $f->setValue('€2.5');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '€2.50',
             'Formats small value'
         );
 
         $f->setValue('€2500000.13');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '€2,500,000.13',
             'Formats large value'
         );
 
         $f->setValue('€2.50000013');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '€2.50',
             'Truncates long decimal portions'
         );
 
         $f->setValue('test123.00test');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '€123.00',
             'Strips alpha values'
         );
 
         $f->setValue('test');
         $this->assertEquals(
-            $f->value,
+            $f->Value(),
             '€0.00',
             'Does not set alpha values'
         );
@@ -282,4 +283,30 @@ class CurrencyFieldTest extends SapphireTest
             -123.45
         );
     }
+
+    public function testDataValueReturnsEmptyFloat()
+    {
+        $field = new CurrencyField('Test', '', null);
+        $this->assertSame(0.00, $field->dataValue());
+    }
+
+    public function testPerformReadonlyTransformation()
+    {
+        $field = new CurrencyField('Test');
+        $result = $field->performReadonlyTransformation();
+        $this->assertInstanceOf(CurrencyField_Readonly::class, $result);
+    }
+
+    public function testInvalidCurrencySymbol()
+    {
+        $field = new CurrencyField('Test', '', '$5.00');
+        $validator = new RequiredFields();
+
+        DBCurrency::config()->update('currency_symbol', '€');
+        $result = $field->validate($validator);
+
+        $this->assertFalse($result, 'Validation should fail since wrong currency was used');
+        $this->assertFalse($validator->getResult()->isValid(), 'Validator should receive failed state');
+        $this->assertContains('Please enter a valid currency', $validator->getResult()->serialize());
+    }
 }

From c06cf4820e02a923a683419b03979bd3677f38db Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 17:41:41 +0200
Subject: [PATCH 11/19] BUG Readonly and disabled CurrencyFields no longer
 always returns dollar currency sign, now respect config

---
 src/Forms/CurrencyField_Disabled.php          |  6 ++-
 src/Forms/CurrencyField_Readonly.php          |  8 +--
 .../php/Forms/CurrencyField_DisabledTest.php  | 34 ++++++++++++
 .../php/Forms/CurrencyField_ReadonlyTest.php  | 53 +++++++++++++++++++
 4 files changed, 96 insertions(+), 5 deletions(-)
 create mode 100644 tests/php/Forms/CurrencyField_DisabledTest.php
 create mode 100644 tests/php/Forms/CurrencyField_ReadonlyTest.php

diff --git a/src/Forms/CurrencyField_Disabled.php b/src/Forms/CurrencyField_Disabled.php
index 7e743755e..68145d41f 100644
--- a/src/Forms/CurrencyField_Disabled.php
+++ b/src/Forms/CurrencyField_Disabled.php
@@ -3,6 +3,7 @@
 namespace SilverStripe\Forms;
 
 use SilverStripe\Core\Convert;
+use SilverStripe\ORM\FieldType\DBCurrency;
 
 /**
  * Readonly version of a {@link CurrencyField}.
@@ -13,7 +14,7 @@ class CurrencyField_Disabled extends CurrencyField
     protected $disabled = true;
 
     /**
-     * overloaded to display the correctly formated value for this datatype
+     * Overloaded to display the correctly formatted value for this data type
      *
      * @param array $properties
      * @return string
@@ -22,7 +23,8 @@ class CurrencyField_Disabled extends CurrencyField
     {
         if ($this->value) {
             $val = Convert::raw2xml($this->value);
-            $val = _t('SilverStripe\\Forms\\CurrencyField.CURRENCYSYMBOL', '$') . number_format(preg_replace('/[^0-9.-]/', "", $val), 2);
+            $val = DBCurrency::config()->get('currency_symbol')
+                . number_format(preg_replace('/[^0-9.-]/', '', $val), 2);
             $valforInput = Convert::raw2att($val);
         } else {
             $valforInput = '';
diff --git a/src/Forms/CurrencyField_Readonly.php b/src/Forms/CurrencyField_Readonly.php
index 775b9eb6b..292827600 100644
--- a/src/Forms/CurrencyField_Readonly.php
+++ b/src/Forms/CurrencyField_Readonly.php
@@ -3,6 +3,7 @@
 namespace SilverStripe\Forms;
 
 use SilverStripe\Core\Convert;
+use SilverStripe\ORM\FieldType\DBCurrency;
 
 /**
  * Readonly version of a {@link CurrencyField}.
@@ -11,19 +12,20 @@ class CurrencyField_Readonly extends ReadonlyField
 {
 
     /**
-     * Overloaded to display the correctly formated value for this datatype
+     * Overloaded to display the correctly formatted value for this data type
      *
      * @param array $properties
      * @return string
      */
     public function Field($properties = array())
     {
+        $currencySymbol = DBCurrency::config()->get('currency_symbol');
         if ($this->value) {
             $val = Convert::raw2xml($this->value);
-            $val = _t('SilverStripe\\Forms\\CurrencyField.CURRENCYSYMBOL', '$') . number_format(preg_replace('/[^0-9.-]/', "", $val), 2);
+            $val = $currencySymbol . number_format(preg_replace('/[^0-9.-]/', '', $val), 2);
             $valforInput = Convert::raw2att($val);
         } else {
-            $val = '<i>' . _t('SilverStripe\\Forms\\CurrencyField.CURRENCYSYMBOL', '$') . '0.00</i>';
+            $val = '<i>' . $currencySymbol . '0.00</i>';
             $valforInput = '';
         }
         return "<span class=\"readonly " . $this->extraClass() . "\" id=\"" . $this->ID() . "\">$val</span>"
diff --git a/tests/php/Forms/CurrencyField_DisabledTest.php b/tests/php/Forms/CurrencyField_DisabledTest.php
new file mode 100644
index 000000000..ed832521c
--- /dev/null
+++ b/tests/php/Forms/CurrencyField_DisabledTest.php
@@ -0,0 +1,34 @@
+<?php
+
+namespace SilverStripe\Forms\Tests;
+
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Forms\CurrencyField_Disabled;
+use SilverStripe\ORM\FieldType\DBCurrency;
+
+class CurrencyField_DisabledTest extends SapphireTest
+{
+    public function testFieldWithValue()
+    {
+        $field = new CurrencyField_Disabled('Test', '', '$5.00');
+        $result = $field->Field();
+
+        $this->assertContains('<input', $result, 'An input should be rendered');
+        $this->assertContains('disabled', $result, 'The input should be disabled');
+        $this->assertContains('$5.00', $result, 'The value should be rendered');
+    }
+
+    /**
+     * @todo: Update the expectation when intl for currencies is implemented
+     */
+    public function testFieldWithCustomisedCurrencySymbol()
+    {
+        DBCurrency::config()->update('currency_symbol', '€');
+        $field = new CurrencyField_Disabled('Test', '', '€5.00');
+        $result = $field->Field();
+
+        $this->assertContains('<input', $result, 'An input should be rendered');
+        $this->assertContains('disabled', $result, 'The input should be disabled');
+        $this->assertContains('€5.00', $result, 'The value should be rendered');
+    }
+}
diff --git a/tests/php/Forms/CurrencyField_ReadonlyTest.php b/tests/php/Forms/CurrencyField_ReadonlyTest.php
new file mode 100644
index 000000000..289327cd1
--- /dev/null
+++ b/tests/php/Forms/CurrencyField_ReadonlyTest.php
@@ -0,0 +1,53 @@
+<?php
+
+namespace SilverStripe\Forms\Tests;
+
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Forms\CurrencyField_Readonly;
+use SilverStripe\ORM\FieldType\DBCurrency;
+
+class CurrencyField_ReadonlyTest extends SapphireTest
+{
+    public function testPerformReadonlyTransformation()
+    {
+        $field = new CurrencyField_Readonly('Test', '', '$5.00');
+        $result = $field->performReadonlyTransformation();
+        $this->assertInstanceOf(CurrencyField_Readonly::class, $result);
+        $this->assertNotSame($result, $field, 'Should return a clone of the field');
+    }
+
+    public function testFieldWithValue()
+    {
+        $field = new CurrencyField_Readonly('Test', '', '$5.00');
+        $result = $field->Field();
+
+        $this->assertContains('<input', $result, 'An input should be rendered');
+        $this->assertContains('readonly', $result, 'The input should be readonly');
+        $this->assertContains('$5.00', $result, 'The value should be rendered');
+    }
+
+    public function testFieldWithOutValue()
+    {
+        DBCurrency::config()->update('currency_symbol', 'AUD');
+        $field = new CurrencyField_Readonly('Test', '', null);
+        $result = $field->Field();
+
+        $this->assertContains('<input', $result, 'An input should be rendered');
+        $this->assertContains('readonly', $result, 'The input should be readonly');
+        $this->assertContains('AUD0.00', $result, 'The value should be rendered');
+    }
+
+    /**
+     * @todo: Update the expectation when intl for currencies is implemented
+     */
+    public function testFieldWithCustomisedCurrencySymbol()
+    {
+        DBCurrency::config()->update('currency_symbol', '€');
+        $field = new CurrencyField_Readonly('Test', '', '€5.00');
+        $result = $field->Field();
+
+        $this->assertContains('<input', $result, 'An input should be rendered');
+        $this->assertContains('readonly', $result, 'The input should be readonly');
+        $this->assertContains('€5.00', $result, 'The value should be rendered');
+    }
+}

From 0e2847e2899b6587e3195e2bb8dc064cecd0d0c4 Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 17:59:55 +0200
Subject: [PATCH 12/19] Add tests for DatalessField

---
 tests/php/Forms/DatalessFieldTest.php | 45 +++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 tests/php/Forms/DatalessFieldTest.php

diff --git a/tests/php/Forms/DatalessFieldTest.php b/tests/php/Forms/DatalessFieldTest.php
new file mode 100644
index 000000000..a67cfc9a6
--- /dev/null
+++ b/tests/php/Forms/DatalessFieldTest.php
@@ -0,0 +1,45 @@
+<?php
+
+namespace SilverStripe\Forms;
+
+use PHPUnit_Framework_MockObject_MockObject;
+use SilverStripe\Dev\SapphireTest;
+
+class DatalessFieldTest extends SapphireTest
+{
+    public function testGetAttributes()
+    {
+        $field = new DatalessField('Name');
+        $result = $field->getAttributes();
+        $this->assertSame('hidden', $result['type']);
+    }
+
+    public function testFieldHolderAndSmallFieldHolderReturnField()
+    {
+        /** @var DatalessField|PHPUnit_Framework_MockObject_MockObject $mock */
+        $mock = $this->getMockBuilder(DatalessField::class)
+            ->disableOriginalConstructor()
+            ->setMethods(['Field'])
+            ->getMock();
+
+        $properties = [
+            'foo' => 'bar',
+        ];
+
+        $mock->expects($this->exactly(2))->method('Field')->with($properties)->willReturn('boo!');
+
+        $fieldHolder = $mock->FieldHolder($properties);
+        $smallFieldHolder = $mock->SmallFieldHolder($properties);
+
+        $this->assertSame('boo!', $fieldHolder);
+        $this->assertSame('boo!', $smallFieldHolder);
+    }
+
+    public function testPerformReadonlyTransformation()
+    {
+        $field = new DatalessField('Test');
+        $result = $field->performReadonlyTransformation();
+        $this->assertInstanceOf(DatalessField::class, $result);
+        $this->assertTrue($result->isReadonly());
+    }
+}

From 97209bc919e5ce6f991f224fc9deadbbd58332f7 Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 18:15:42 +0200
Subject: [PATCH 13/19] Add edge case unit tests for DateField

---
 src/Forms/DateField.php           |  2 +-
 tests/php/Forms/DateFieldTest.php | 52 +++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/src/Forms/DateField.php b/src/Forms/DateField.php
index a9c72e033..a58113b6f 100644
--- a/src/Forms/DateField.php
+++ b/src/Forms/DateField.php
@@ -10,7 +10,7 @@ use SilverStripe\ORM\FieldType\DBDatetime;
 use SilverStripe\ORM\ValidationResult;
 
 /**
- * Form used for editing a date stirng
+ * Form used for editing a date string
  *
  * Caution: The form field does not include any JavaScript or CSS when used outside of the CMS context,
  * since the required frontend dependencies are included through CMS bundling.
diff --git a/tests/php/Forms/DateFieldTest.php b/tests/php/Forms/DateFieldTest.php
index 5ea4d9a03..87065fc8a 100644
--- a/tests/php/Forms/DateFieldTest.php
+++ b/tests/php/Forms/DateFieldTest.php
@@ -5,8 +5,10 @@ namespace SilverStripe\Forms\Tests;
 use IntlDateFormatter;
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\Forms\DateField;
+use SilverStripe\Forms\DateField_Disabled;
 use SilverStripe\Forms\RequiredFields;
 use SilverStripe\i18n\i18n;
+use SilverStripe\ORM\FieldType\DBDate;
 use SilverStripe\ORM\FieldType\DBDatetime;
 
 /**
@@ -225,4 +227,54 @@ class DateFieldTest extends SapphireTest
         $dateField->setLocale('de_DE');
         $dateField->Value();
     }
+
+    public function testGetDateFormatHTML5()
+    {
+        $field = new DateField('Date');
+        $field->setHTML5(true);
+        $this->assertSame(DBDate::ISO_DATE, $field->getDateFormat());
+    }
+
+    public function testGetDateFormatViaSetter()
+    {
+        $field = new DateField('Date');
+        $field->setHTML5(false);
+        $field->setDateFormat('d-m-Y');
+        $this->assertSame('d-m-Y', $field->getDateFormat());
+    }
+
+    public function testGetAttributes()
+    {
+        $field = new DateField('Date');
+        $field
+            ->setHTML5(true)
+            ->setMinDate('1980-05-10')
+            ->setMaxDate('1980-05-20');
+
+        $result = $field->getAttributes();
+        $this->assertSame('1980-05-10', $result['min']);
+        $this->assertSame('1980-05-20', $result['max']);
+    }
+
+    public function testSetSubmittedValueNull()
+    {
+        $field = new DateField('Date');
+        $field->setSubmittedValue(false);
+        $this->assertNull($field->Value());
+    }
+
+    public function testPerformReadonlyTransformation()
+    {
+        $field = new DateField('Date');
+        $result = $field->performReadonlyTransformation();
+        $this->assertInstanceOf(DateField_Disabled::class, $result);
+        $this->assertTrue($result->isReadonly());
+    }
+
+    public function testValidateWithoutValueReturnsTrue()
+    {
+        $field = new DateField('Date');
+        $validator = new RequiredFields();
+        $this->assertTrue($field->validate($validator));
+    }
 }

From fd50ce629572673090c6a4fa64b718464b1a5a92 Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 18:29:07 +0200
Subject: [PATCH 14/19] Add more edge case tests for disabled DateFields and
 DatetimeField

---
 tests/php/Forms/DateField_DisabledTest.php | 10 ++-
 tests/php/Forms/DatetimeFieldTest.php      | 75 +++++++++++++++++++---
 2 files changed, 73 insertions(+), 12 deletions(-)

diff --git a/tests/php/Forms/DateField_DisabledTest.php b/tests/php/Forms/DateField_DisabledTest.php
index a8e5e3d1f..0108b751d 100644
--- a/tests/php/Forms/DateField_DisabledTest.php
+++ b/tests/php/Forms/DateField_DisabledTest.php
@@ -2,10 +2,8 @@
 
 namespace SilverStripe\Forms\Tests;
 
-use IntlDateFormatter;
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\Forms\DateField_Disabled;
-use SilverStripe\Forms\RequiredFields;
 use SilverStripe\i18n\i18n;
 use SilverStripe\ORM\FieldType\DBDatetime;
 
@@ -76,4 +74,12 @@ class DateField_DisabledTest extends SapphireTest
         $actual = DateField_Disabled::create('Test')->setValue('This is not a date')->Field();
         $this->assertEquals($expected, $actual);
     }
+
+    public function testType()
+    {
+        $field = new DateField_Disabled('Test');
+        $result = $field->Type();
+        $this->assertContains('readonly', $result, 'Disabled field should be treated as readonly');
+        $this->assertContains('date_disabled', $result, 'Field should contain date_disabled class');
+    }
 }
diff --git a/tests/php/Forms/DatetimeFieldTest.php b/tests/php/Forms/DatetimeFieldTest.php
index d3ca4db24..37fe628e3 100644
--- a/tests/php/Forms/DatetimeFieldTest.php
+++ b/tests/php/Forms/DatetimeFieldTest.php
@@ -86,6 +86,13 @@ class DatetimeFieldTest extends SapphireTest
         $this->assertEquals('2003-01-30 11:59:38', $f->dataValue()); // server timezone (Berlin)
     }
 
+    public function testSetSubmittedValueNull()
+    {
+        $field = new DatetimeField('Datetime');
+        $field->setSubmittedValue(false);
+        $this->assertNull($field->Value());
+    }
+
     public function testConstructorWithoutArgs()
     {
         $f = new DatetimeField('Datetime');
@@ -148,21 +155,24 @@ class DatetimeFieldTest extends SapphireTest
 
     public function testValidate()
     {
-        $f = new DatetimeField('Datetime', 'Datetime', '2003-03-29 23:59:38');
-        $this->assertTrue($f->validate(new RequiredFields()));
+        $field = new DatetimeField('Datetime', 'Datetime', '2003-03-29 23:59:38');
+        $this->assertTrue($field->validate(new RequiredFields()));
 
-        $f = new DatetimeField('Datetime', 'Datetime', '2003-03-29T23:59:38');
-        $this->assertTrue($f->validate(new RequiredFields()), 'Normalised ISO');
+        $field = new DatetimeField('Datetime', 'Datetime', '2003-03-29T23:59:38');
+        $this->assertTrue($field->validate(new RequiredFields()), 'Normalised ISO');
 
-        $f = new DatetimeField('Datetime', 'Datetime', '2003-03-29');
-        $this->assertFalse($f->validate(new RequiredFields()), 'Leaving out time');
+        $field = new DatetimeField('Datetime', 'Datetime', '2003-03-29');
+        $this->assertFalse($field->validate(new RequiredFields()), 'Leaving out time');
 
-        $f = (new DatetimeField('Datetime', 'Datetime'))
+        $field = (new DatetimeField('Datetime', 'Datetime'))
             ->setSubmittedValue('2003-03-29T00:00');
-        $this->assertTrue($f->validate(new RequiredFields()), 'Leaving out seconds (like many browsers)');
+        $this->assertTrue($field->validate(new RequiredFields()), 'Leaving out seconds (like many browsers)');
 
-        $f = new DatetimeField('Datetime', 'Datetime', 'wrong');
-        $this->assertFalse($f->validate(new RequiredFields()));
+        $field = new DatetimeField('Datetime', 'Datetime', 'wrong');
+        $this->assertFalse($field->validate(new RequiredFields()));
+
+        $field = new DatetimeField('Datetime', 'Datetime', false);
+        $this->assertTrue($field->validate(new RequiredFields()));
     }
 
     public function testSetMinDate()
@@ -446,6 +456,51 @@ class DatetimeFieldTest extends SapphireTest
         $this->assertEquals($attrs['max'], '2010-01-31T23:00:00'); // frontend timezone
     }
 
+    public function testAttributesNonHTML5()
+    {
+        $field = new DatetimeField('Datetime');
+        $field->setHTML5(false);
+        $result = $field->getAttributes();
+        $this->assertSame('text', $result['type']);
+    }
+
+    public function testFrontendToInternalEdgeCases()
+    {
+        $field = new DatetimeField('Datetime');
+
+        $this->assertNull($field->frontendToInternal(false));
+        $this->assertNull($field->frontendToInternal('sdfsdfsfs$%^&*'));
+    }
+
+    public function testInternalToFrontendEdgeCases()
+    {
+        $field = new DatetimeField('Datetime');
+
+        $this->assertNull($field->internalToFrontend(false));
+        $this->assertNull($field->internalToFrontend('sdfsdfsfs$%^&*'));
+    }
+
+    public function testPerformReadonlyTransformation()
+    {
+        $field = new DatetimeField('Datetime');
+
+        $result = $field->performReadonlyTransformation();
+        $this->assertInstanceOf(DatetimeField::class, $result);
+        $this->assertNotSame($result, $field, 'Readonly field should be cloned');
+        $this->assertTrue($result->isReadonly());
+    }
+
+    /**
+     * @expectedException \BadMethodCallException
+     * @expectedExceptionMessage Can't change timezone after setting a value
+     */
+    public function testSetTimezoneThrowsExceptionWhenChangingTimezoneAfterSettingValue()
+    {
+        date_default_timezone_set('Europe/Berlin');
+        $field = new DatetimeField('Datetime', 'Time', '2003-03-29 23:59:38');
+        $field->setTimezone('Pacific/Auckland');
+    }
+
     protected function getMockForm()
     {
         /** @skipUpgrade */

From 449b2cf29187ca7e786c2bbcdd1eeebef1bee0cf Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 19:47:11 +0200
Subject: [PATCH 15/19] Add tests for DefaultFormFactory

---
 src/Forms/DefaultFormFactory.php           |  1 +
 tests/php/Forms/DefaultFormFactoryTest.php | 38 ++++++++++++++++++++++
 2 files changed, 39 insertions(+)
 create mode 100644 tests/php/Forms/DefaultFormFactoryTest.php

diff --git a/src/Forms/DefaultFormFactory.php b/src/Forms/DefaultFormFactory.php
index 8aeaf3088..24c6e3feb 100644
--- a/src/Forms/DefaultFormFactory.php
+++ b/src/Forms/DefaultFormFactory.php
@@ -30,6 +30,7 @@ class DefaultFormFactory implements FormFactory
      * @param string $name
      * @param array $context
      * @return Form
+     * @throws InvalidArgumentException When required context is missing
      */
     public function getForm(RequestHandler $controller = null, $name = FormFactory::DEFAULT_NAME, $context = [])
     {
diff --git a/tests/php/Forms/DefaultFormFactoryTest.php b/tests/php/Forms/DefaultFormFactoryTest.php
new file mode 100644
index 000000000..c8ab715f3
--- /dev/null
+++ b/tests/php/Forms/DefaultFormFactoryTest.php
@@ -0,0 +1,38 @@
+<?php
+
+namespace SilverStripe\Forms\Tests;
+
+use InvalidArgumentException;
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Forms\DefaultFormFactory;
+use SilverStripe\ORM\DataObject;
+
+class DefaultFormFactoryTest extends SapphireTest
+{
+    /**
+     * @expectedException InvalidArgumentException
+     * @expectedExceptionMessageRegExp /Missing required context/
+     */
+    public function testGetFormThrowsExceptionOnMissingContext()
+    {
+        $factory = new DefaultFormFactory();
+        $factory->getForm();
+    }
+
+    public function testGetForm()
+    {
+        $record = new DataObject();
+        $record->Title = 'Test';
+
+        $factory = new DefaultFormFactory();
+        $form = $factory->getForm(null, null, ['Record' => $record]);
+
+        $this->assertSame($record, $form->getRecord());
+    }
+
+    public function testGetRequiredContext()
+    {
+        $factory = new DefaultFormFactory();
+        $this->assertContains('Record', $factory->getRequiredContext());
+    }
+}

From bea4101e216c98da0c14df009ebd8be5341c56ed Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 20 Oct 2018 19:49:21 +0200
Subject: [PATCH 16/19] Add tests for DisabledTransformation,
 PrintableTransformation and PrintableTransformation_TabSet

---
 src/Forms/PrintableTransformation_TabSet.php  |  4 +--
 src/Forms/TabSet.php                          |  3 +-
 .../php/Forms/DisabledTransformationTest.php  | 20 +++++++++++
 .../php/Forms/PrintableTransformationTest.php | 25 +++++++++++++
 .../PrintableTransformation_TabSetTest.php    | 36 +++++++++++++++++++
 5 files changed, 84 insertions(+), 4 deletions(-)
 create mode 100644 tests/php/Forms/DisabledTransformationTest.php
 create mode 100644 tests/php/Forms/PrintableTransformationTest.php
 create mode 100644 tests/php/Forms/PrintableTransformation_TabSetTest.php

diff --git a/src/Forms/PrintableTransformation_TabSet.php b/src/Forms/PrintableTransformation_TabSet.php
index f25598e5a..2be4f72d7 100644
--- a/src/Forms/PrintableTransformation_TabSet.php
+++ b/src/Forms/PrintableTransformation_TabSet.php
@@ -19,9 +19,9 @@ class PrintableTransformation_TabSet extends TabSet
     public function FieldHolder($properties = array())
     {
         // This gives us support for sub-tabs.
-        $tag = ($this->tabSet) ? "h2>" : "h1>";
+        $tag = $this->getTabSet() ? 'h2>' : 'h1>';
         $retVal = '';
-        foreach ($this->children as $tab) {
+        foreach ($this->getChildren() as $tab) {
             $retVal .= "<$tag" . $tab->Title() . "</$tag\n";
             $retVal .= $tab->FieldHolder();
         }
diff --git a/src/Forms/TabSet.php b/src/Forms/TabSet.php
index da18e9d48..dc0b60217 100644
--- a/src/Forms/TabSet.php
+++ b/src/Forms/TabSet.php
@@ -106,9 +106,8 @@ class TabSet extends CompositeField
     {
         if ($this->tabSet) {
             return $this->tabSet->ID() . '_' . $this->id . '_set';
-        } else {
-            return $this->id;
         }
+        return $this->id;
     }
 
     /**
diff --git a/tests/php/Forms/DisabledTransformationTest.php b/tests/php/Forms/DisabledTransformationTest.php
new file mode 100644
index 000000000..74b460f39
--- /dev/null
+++ b/tests/php/Forms/DisabledTransformationTest.php
@@ -0,0 +1,20 @@
+<?php
+
+namespace SilverStripe\Forms\Tests;
+
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Forms\DisabledTransformation;
+use SilverStripe\Forms\TextField;
+
+class DisabledTransformationTest extends SapphireTest
+{
+    public function testTransform()
+    {
+        $field = new TextField('Test');
+
+        $transformation = new DisabledTransformation();
+        $newField = $transformation->transform($field);
+
+        $this->assertTrue($newField->isDisabled(), 'Transformation failed to transform field to be disabled');
+    }
+}
diff --git a/tests/php/Forms/PrintableTransformationTest.php b/tests/php/Forms/PrintableTransformationTest.php
new file mode 100644
index 000000000..8a54d8d49
--- /dev/null
+++ b/tests/php/Forms/PrintableTransformationTest.php
@@ -0,0 +1,25 @@
+<?php
+
+namespace SilverStripe\Forms\Tests;
+
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Forms\PrintableTransformation;
+use SilverStripe\Forms\PrintableTransformation_TabSet;
+use SilverStripe\Forms\Tab;
+use SilverStripe\Forms\TabSet;
+
+class PrintableTransformationTest extends SapphireTest
+{
+    public function testTransformTabSet()
+    {
+        $tab1 = new Tab('Main');
+        $tab2 = new Tab('Settings');
+        $tabSet = new TabSet('Root', 'Root', $tab1, $tab2);
+
+        $transformation = new PrintableTransformation();
+        $result = $transformation->transformTabSet($tabSet);
+
+        $this->assertInstanceOf(PrintableTransformation_TabSet::class, $result);
+        $this->assertSame('Root', $result->Title());
+    }
+}
diff --git a/tests/php/Forms/PrintableTransformation_TabSetTest.php b/tests/php/Forms/PrintableTransformation_TabSetTest.php
new file mode 100644
index 000000000..11ca4efd8
--- /dev/null
+++ b/tests/php/Forms/PrintableTransformation_TabSetTest.php
@@ -0,0 +1,36 @@
+<?php
+
+namespace SilverStripe\Forms\Tests;
+
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Forms\PrintableTransformation_TabSet;
+use SilverStripe\Forms\Tab;
+use SilverStripe\Forms\TabSet;
+
+class PrintableTransformation_TabSetTest extends SapphireTest
+{
+    public function testFieldHolder()
+    {
+        $tabs = [
+            new Tab('Main'),
+            new Tab('Secondary'),
+            $optionsTabSet = new TabSet(
+                'Options',
+                'Options',
+                new Tab('Colours'),
+                new Tab('Options')
+            ),
+        ];
+
+        $transformationTabSet = new PrintableTransformation_TabSet($tabs);
+        $result = $transformationTabSet->FieldHolder();
+
+        $this->assertContains('<h1>Main</h1>', $result);
+        $this->assertContains('<h1>Secondary</h1>', $result);
+
+        $transformationTabSet->setTabSet($optionsTabSet);
+        $result = $transformationTabSet->FieldHolder();
+
+        $this->assertContains('<h2>Options</h2>', $result);
+    }
+}

From 0f2eebe5d41698e3d8de74e4b2cf38ea89bf7d1e Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Fri, 9 Nov 2018 13:59:14 +0200
Subject: [PATCH 17/19] NEW Change to variadic calls in ListDecorator and add
 unit tests

---
 src/ORM/ListDecorator.php           |  13 +-
 tests/php/ORM/ListDecoratorTest.php | 278 ++++++++++++++++++++++++++++
 2 files changed, 283 insertions(+), 8 deletions(-)
 create mode 100644 tests/php/ORM/ListDecoratorTest.php

diff --git a/src/ORM/ListDecorator.php b/src/ORM/ListDecorator.php
index b141d548e..8620168ff 100644
--- a/src/ORM/ListDecorator.php
+++ b/src/ORM/ListDecorator.php
@@ -173,8 +173,7 @@ abstract class ListDecorator extends ViewableData implements SS_List, Sortable,
      */
     public function sort()
     {
-        $args = func_get_args();
-        return call_user_func_array(array($this->list, 'sort'), $args);
+        return $this->list->sort(...func_get_args());
     }
 
     public function canFilterBy($by)
@@ -192,8 +191,7 @@ abstract class ListDecorator extends ViewableData implements SS_List, Sortable,
      */
     public function filter()
     {
-        $args = func_get_args();
-        return call_user_func_array(array($this->list, 'filter'), $args);
+        return $this->list->filter(...func_get_args());
     }
 
     /**
@@ -220,7 +218,7 @@ abstract class ListDecorator extends ViewableData implements SS_List, Sortable,
      */
     public function filterAny()
     {
-        return call_user_func_array(array($this->list, __FUNCTION__), func_get_args());
+        return $this->list->filterAny(...func_get_args());
     }
 
     /**
@@ -242,7 +240,7 @@ abstract class ListDecorator extends ViewableData implements SS_List, Sortable,
         }
         $output = ArrayList::create();
         foreach ($this->list as $item) {
-            if (call_user_func($callback, $item, $this->list)) {
+            if ($callback($item, $this->list)) {
                 $output->push($item);
             }
         }
@@ -286,8 +284,7 @@ abstract class ListDecorator extends ViewableData implements SS_List, Sortable,
      */
     public function exclude()
     {
-        $args = func_get_args();
-        return call_user_func_array(array($this->list, 'exclude'), $args);
+        return $this->list->exclude(...func_get_args());
     }
 
     public function debug()
diff --git a/tests/php/ORM/ListDecoratorTest.php b/tests/php/ORM/ListDecoratorTest.php
new file mode 100644
index 000000000..6fe45fdd2
--- /dev/null
+++ b/tests/php/ORM/ListDecoratorTest.php
@@ -0,0 +1,278 @@
+<?php
+
+namespace SilverStripe\ORM\Tests;
+
+use PHPUnit_Framework_MockObject_MockObject;
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\ORM\ArrayList;
+use SilverStripe\ORM\ListDecorator;
+use SilverStripe\ORM\SS_List;
+
+/**
+ * This test class is testing that ListDecorator correctly proxies its calls through to the underlying SS_List
+ */
+class ListDecoratorTest extends SapphireTest
+{
+    /**
+     * @var ArrayList|PHPUnit_Framework_MockObject_MockObject
+     */
+    protected $list;
+
+    /**
+     * @var ListDecorator|PHPUnit_Framework_MockObject_MockObject
+     */
+    protected $decorator;
+
+    protected function setUp()
+    {
+        parent::setUp();
+
+        $this->list = $this->createMock(ArrayList::class);
+        $this->decorator = $this->getMockForAbstractClass(ListDecorator::class, [$this->list]);
+    }
+
+    public function testGetIterator()
+    {
+        $this->list->expects($this->once())->method('getIterator')->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->getIterator());
+    }
+
+    public function testCanSortBy()
+    {
+        $this->list->expects($this->once())->method('canSortBy')->with('foo')->willReturn(true);
+        $this->assertTrue($this->decorator->canSortBy('foo'));
+    }
+
+    public function testRemove()
+    {
+        $this->list->expects($this->once())->method('remove')->with('foo');
+        $this->decorator->remove('foo');
+    }
+
+    /**
+     * @param array $input
+     * @dataProvider filterProvider
+     */
+    public function testExclude($input)
+    {
+        $this->list->expects($this->once())->method('exclude')->with($input)->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->exclude($input));
+    }
+
+    /**
+     * @param array $input
+     * @dataProvider filterProvider
+     */
+    public function testFilter($input)
+    {
+        $this->list->expects($this->once())->method('filter')->with($input)->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->filter($input));
+    }
+
+    /**
+     * @param array $input
+     * @dataProvider filterProvider
+     */
+    public function testFilterAny($input)
+    {
+        $this->list->expects($this->once())->method('filterAny')->with($input)->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->filterAny($input));
+    }
+
+    /**
+     * @param array $input
+     * @dataProvider filterProvider
+     */
+    public function testSort($input)
+    {
+        $this->list->expects($this->once())->method('sort')->with($input)->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->sort($input));
+    }
+
+    /**
+     * @return array[]
+     */
+    public function filterProvider()
+    {
+        return [
+            ['Name', 'Bob'],
+            ['Name', ['aziz', 'Bob']],
+            [['Name' =>'bob', 'Age' => 21]],
+            [['Name' =>'bob', 'Age' => [21, 43]]],
+        ];
+    }
+
+    public function testCanFilterBy()
+    {
+        $this->list->expects($this->once())->method('canFilterBy')->with('Title')->willReturn(false);
+        $this->assertFalse($this->decorator->canFilterBy('Title'));
+    }
+
+    /**
+     * @expectedException \LogicException
+     * @expectedExceptionMessage SS_Filterable::filterByCallback() passed callback must be callable, 'boolean' given
+     */
+    public function testFilterByCallbackThrowsExceptionWhenGivenNonCallable()
+    {
+        $this->decorator->filterByCallback(true);
+    }
+
+    public function testFilterByCallback()
+    {
+        $input = new ArrayList([
+            ['Name' => 'Leslie'],
+            ['Name' => 'Maxime'],
+            ['Name' => 'Sal'],
+        ]);
+
+        $callback = function ($item, SS_List $list) {
+            return $item->Name === 'Maxime';
+        };
+
+        $this->decorator->setList($input);
+        $result = $this->decorator->filterByCallback($callback);
+
+        $this->assertCount(1, $result);
+        $this->assertSame('Maxime', $result->first()->Name);
+    }
+
+    public function testFind()
+    {
+        $this->list->expects($this->once())->method('find')->with('foo', 'bar')->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->find('foo', 'bar'));
+    }
+
+    public function testDebug()
+    {
+        $this->list->expects($this->once())->method('debug')->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->debug());
+    }
+
+    public function testCount()
+    {
+        $this->list->expects($this->once())->method('count')->willReturn(5);
+        $this->assertSame(5, $this->decorator->Count());
+    }
+
+    public function testEach()
+    {
+        $callable = function () {
+            // noop
+        };
+        $this->list->expects($this->once())->method('each')->with($callable)->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->each($callable));
+    }
+
+    public function testOffsetExists()
+    {
+        $this->list->expects($this->once())->method('offsetExists')->with('foo')->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->offsetExists('foo'));
+    }
+
+    public function testGetList()
+    {
+        $this->assertSame($this->list, $this->decorator->getList());
+    }
+
+    public function testColumnUnique()
+    {
+        $this->list->expects($this->once())->method('columnUnique')->with('ID')->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->columnUnique('ID'));
+    }
+
+    public function testMap()
+    {
+        $this->list->expects($this->once())->method('map')->with('ID', 'Title')->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->map('ID', 'Title'));
+    }
+
+    public function testReverse()
+    {
+        $this->list->expects($this->once())->method('reverse')->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->reverse());
+    }
+
+    public function testOffsetGet()
+    {
+        $this->list->expects($this->once())->method('offsetGet')->with(2)->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->offsetGet(2));
+    }
+
+    public function testExists()
+    {
+        $this->list->expects($this->once())->method('exists')->willReturn(false);
+        $this->assertFalse($this->decorator->exists());
+    }
+
+    public function testByID()
+    {
+        $this->list->expects($this->once())->method('byID')->with(123)->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->byID(123));
+    }
+
+    public function testByIDs()
+    {
+        $this->list->expects($this->once())->method('byIDs')->with([1, 2])->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->byIDs([1, 2]));
+    }
+
+    public function testToArray()
+    {
+        $this->list->expects($this->once())->method('toArray')->willReturn(['foo', 'bar']);
+        $this->assertSame(['foo', 'bar'], $this->decorator->toArray());
+    }
+
+    public function testToNestedArray()
+    {
+        $this->list->expects($this->once())->method('toNestedArray')->willReturn(['foo', 'bar']);
+        $this->assertSame(['foo', 'bar'], $this->decorator->toNestedArray());
+    }
+
+    public function testOffsetSet()
+    {
+        $this->list->expects($this->once())->method('offsetSet')->with('foo', 'bar');
+        $this->decorator->offsetSet('foo', 'bar');
+    }
+
+    public function testOffsetUnset()
+    {
+        $this->list->expects($this->once())->method('offsetUnset')->with('foo');
+        $this->decorator->offsetUnset('foo');
+    }
+
+    public function testLimit()
+    {
+        $this->list->expects($this->once())->method('limit')->with(5, 3)->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->limit(5, 3));
+    }
+
+    public function testTotalItems()
+    {
+        $this->list->expects($this->once())->method('count')->willReturn(5);
+        $this->assertSame(5, $this->decorator->TotalItems());
+    }
+
+    public function testAdd()
+    {
+        $this->list->expects($this->once())->method('add')->with('foo')->willReturn('mock');
+        $this->decorator->add('foo');
+    }
+
+    public function testFirst()
+    {
+        $this->list->expects($this->once())->method('first')->willReturn(1);
+        $this->assertSame(1, $this->decorator->first());
+    }
+
+    public function testLast()
+    {
+        $this->list->expects($this->once())->method('last')->willReturn(10);
+        $this->assertSame(10, $this->decorator->last());
+    }
+
+    public function testColumn()
+    {
+        $this->list->expects($this->once())->method('column')->with('DOB')->willReturn('mock');
+        $this->assertSame('mock', $this->decorator->column('DOB'));
+    }
+}

From eba92d77df1c9fa678dab97e548bb9ba65dcb696 Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 10 Nov 2018 10:04:17 +0200
Subject: [PATCH 18/19] Rename CheckboxFieldReadonlyTest for future PSR-2
 compatibility

---
 ...kboxField_ReadonlyTest.php => CheckboxFieldReadonlyTest.php} | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 rename tests/php/Forms/{CheckboxField_ReadonlyTest.php => CheckboxFieldReadonlyTest.php} (88%)

diff --git a/tests/php/Forms/CheckboxField_ReadonlyTest.php b/tests/php/Forms/CheckboxFieldReadonlyTest.php
similarity index 88%
rename from tests/php/Forms/CheckboxField_ReadonlyTest.php
rename to tests/php/Forms/CheckboxFieldReadonlyTest.php
index 4996deda8..e82156325 100644
--- a/tests/php/Forms/CheckboxField_ReadonlyTest.php
+++ b/tests/php/Forms/CheckboxFieldReadonlyTest.php
@@ -5,7 +5,7 @@ namespace SilverStripe\Forms\Tests;
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\Forms\CheckboxField_Readonly;
 
-class CheckboxField_ReadonlyTest extends SapphireTest
+class CheckboxFieldReadonlyTest extends SapphireTest
 {
     public function testPerformReadonlyTransformation()
     {

From bab84f31dc50c3ec251fa108793edad4dd9b78b6 Mon Sep 17 00:00:00 2001
From: Robbie Averill <robbie@silverstripe.com>
Date: Sat, 10 Nov 2018 14:55:11 +0200
Subject: [PATCH 19/19] Remove underscores from variable test class names

---
 ...encyField_DisabledTest.php => CurrencyFieldDisabledTest.php} | 2 +-
 ...encyField_ReadonlyTest.php => CurrencyFieldReadonlyTest.php} | 2 +-
 .../{DateField_DisabledTest.php => DateFieldDisabledTest.php}   | 2 +-
 ...ion_TabSetTest.php => PrintableTransformationTabSetTest.php} | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)
 rename tests/php/Forms/{CurrencyField_DisabledTest.php => CurrencyFieldDisabledTest.php} (95%)
 rename tests/php/Forms/{CurrencyField_ReadonlyTest.php => CurrencyFieldReadonlyTest.php} (97%)
 rename tests/php/Forms/{DateField_DisabledTest.php => DateFieldDisabledTest.php} (98%)
 rename tests/php/Forms/{PrintableTransformation_TabSetTest.php => PrintableTransformationTabSetTest.php} (93%)

diff --git a/tests/php/Forms/CurrencyField_DisabledTest.php b/tests/php/Forms/CurrencyFieldDisabledTest.php
similarity index 95%
rename from tests/php/Forms/CurrencyField_DisabledTest.php
rename to tests/php/Forms/CurrencyFieldDisabledTest.php
index ed832521c..a36efec58 100644
--- a/tests/php/Forms/CurrencyField_DisabledTest.php
+++ b/tests/php/Forms/CurrencyFieldDisabledTest.php
@@ -6,7 +6,7 @@ use SilverStripe\Dev\SapphireTest;
 use SilverStripe\Forms\CurrencyField_Disabled;
 use SilverStripe\ORM\FieldType\DBCurrency;
 
-class CurrencyField_DisabledTest extends SapphireTest
+class CurrencyFieldDisabledTest extends SapphireTest
 {
     public function testFieldWithValue()
     {
diff --git a/tests/php/Forms/CurrencyField_ReadonlyTest.php b/tests/php/Forms/CurrencyFieldReadonlyTest.php
similarity index 97%
rename from tests/php/Forms/CurrencyField_ReadonlyTest.php
rename to tests/php/Forms/CurrencyFieldReadonlyTest.php
index 289327cd1..d403126e7 100644
--- a/tests/php/Forms/CurrencyField_ReadonlyTest.php
+++ b/tests/php/Forms/CurrencyFieldReadonlyTest.php
@@ -6,7 +6,7 @@ use SilverStripe\Dev\SapphireTest;
 use SilverStripe\Forms\CurrencyField_Readonly;
 use SilverStripe\ORM\FieldType\DBCurrency;
 
-class CurrencyField_ReadonlyTest extends SapphireTest
+class CurrencyFieldReadonlyTest extends SapphireTest
 {
     public function testPerformReadonlyTransformation()
     {
diff --git a/tests/php/Forms/DateField_DisabledTest.php b/tests/php/Forms/DateFieldDisabledTest.php
similarity index 98%
rename from tests/php/Forms/DateField_DisabledTest.php
rename to tests/php/Forms/DateFieldDisabledTest.php
index 0108b751d..6fffdcd13 100644
--- a/tests/php/Forms/DateField_DisabledTest.php
+++ b/tests/php/Forms/DateFieldDisabledTest.php
@@ -10,7 +10,7 @@ use SilverStripe\ORM\FieldType\DBDatetime;
 /**
  * @skipUpgrade
  */
-class DateField_DisabledTest extends SapphireTest
+class DateFieldDisabledTest extends SapphireTest
 {
     protected function setUp()
     {
diff --git a/tests/php/Forms/PrintableTransformation_TabSetTest.php b/tests/php/Forms/PrintableTransformationTabSetTest.php
similarity index 93%
rename from tests/php/Forms/PrintableTransformation_TabSetTest.php
rename to tests/php/Forms/PrintableTransformationTabSetTest.php
index 11ca4efd8..12691ddde 100644
--- a/tests/php/Forms/PrintableTransformation_TabSetTest.php
+++ b/tests/php/Forms/PrintableTransformationTabSetTest.php
@@ -7,7 +7,7 @@ use SilverStripe\Forms\PrintableTransformation_TabSet;
 use SilverStripe\Forms\Tab;
 use SilverStripe\Forms\TabSet;
 
-class PrintableTransformation_TabSetTest extends SapphireTest
+class PrintableTransformationTabSetTest extends SapphireTest
 {
     public function testFieldHolder()
     {