Merge branch '1.11' into 1.12-release

silverstripe · Nov 21, 2022 · 8c0f820 · 8c0f820
2 parents 43a637a + aef67c5
commit 8c0f820
Show file tree

Hide file tree

Showing 2 changed files with 90 additions and 2 deletions.
diff --git a/src/Forms/DiffField.php b/src/Forms/DiffField.php
@@ -3,7 +3,9 @@
 namespace SilverStripe\VersionedAdmin\Forms;
 
 use SilverStripe\Forms\FormField;
+use SilverStripe\Forms\HTMLEditor\HTMLEditorField_Readonly;
 use SilverStripe\Forms\HTMLReadonlyField;
+use SilverStripe\Forms\ReadonlyField;
 use SilverStripe\View\Parsers\Diff;
 
 /**
@@ -49,8 +51,27 @@ public function Value()
         if (is_object($oldValue) || is_object($newValue)) {
             return sprintf('(%s)', _t(__CLASS__ . '.NO_DIFF_AVAILABLE', 'No diff available'));
         }
-
-        return Diff::compareHTML($oldValue, $newValue);
+        // Escape content from everything except HTMLEditorFields
+        $escape = true;
+        if ($this->getComparisonField() instanceof HTMLEditorField_Readonly) {
+            $escape = false;
+        }
+        // Ensure that the emtpy placeholder value is not escaped
+        // The empty placeholder value is usually going to be `<i>('none')</i>`
+        $emptyPlaceholder = ReadonlyField::create('na')->Value();
+        if ($oldValue === $newValue && $newValue === $emptyPlaceholder) {
+            // if both the old and new valus are empty, let the surronding <i> tags render as HTML (escape = false)
+            $escape = false;
+        } else {
+            // if either the new or old values are the empty placeholder, but the corresponding valued
+            // diffed against is not, then strip the surronding <i> tags and do not render as HTML (escape = true)
+            if ($oldValue === $emptyPlaceholder) {
+                $oldValue = strip_tags($emptyPlaceholder);
+            } elseif ($newValue === $emptyPlaceholder) {
+                $newValue = strip_tags($emptyPlaceholder);
+            }
+        }
+        return Diff::compareHTML($oldValue, $newValue, $escape);
     }
 
     /**

diff --git a/tests/Forms/DiffFieldTest.php b/tests/Forms/DiffFieldTest.php
@@ -3,6 +3,8 @@
 namespace SilverStripe\VersionedAdmin\Tests\Forms;
 
 use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Forms\HTMLEditor\HTMLEditorField_Readonly;
+use SilverStripe\Forms\ReadonlyField;
 use SilverStripe\Forms\TextField;
 use SilverStripe\Forms\TreeMultiselectField;
 use SilverStripe\ORM\ManyManyList;
@@ -35,4 +37,69 @@ public function testObjectValuesAreNotDiffed()
 
         $this->assertEquals('(No diff available)', $diffField->Value());
     }
+
+    /**
+     * @dataProvider provideEscaping
+     */
+    public function testEscaping(
+        string $className,
+        string $oldValue,
+        string $newValue,
+        string $expected,
+        string $message
+    ) {
+        // get $emptyPlaceholder here instead of provideEscaping to prevent
+        // BadMethodCallException: No injector manifests available
+        // error in dataProvider method
+        $emptyPlaceholder = ReadonlyField::create('na')->Value();
+        $emptyPlaceholderNoTags = strip_tags($emptyPlaceholder);
+        $expected = str_replace('$emptyPlaceholderNoTags', $emptyPlaceholderNoTags, $expected);
+        $expected = str_replace('$emptyPlaceholder', $emptyPlaceholder, $expected);
+        $newField = new $className('Test', 'Test', $oldValue);
+        $diffField = DiffField::create('DiffTest');
+        $diffField->setComparisonField($newField);
+        $diffField->setValue($newValue);
+        $this->assertSame($expected, $diffField->Value(), $message);
+    }
+
+    public function provideEscaping()
+    {
+        return [
+            [
+                ReadonlyField::class,
+                'Something',
+                'Something <strong>bold</strong>',
+                'Something <del>&lt;strong&gt; bold &lt;/strong&gt;</del>',
+                'Non HTML field is escaped'
+            ],
+            [
+                HTMLEditorField_Readonly::class,
+                'Something',
+                'Something <strong>bold</strong>',
+                'Something <del><strong>bold</strong></del>',
+                'Non HTML field is not escaped'
+            ],
+            [
+                ReadonlyField::class,
+                '',
+                '',
+                '$emptyPlaceholder',
+                'No value is not escaped'
+            ],
+            [
+                ReadonlyField::class,
+                '',
+                'Something',
+                '<ins>$emptyPlaceholderNoTags</ins> <del>Something</del>',
+                'No value is escaped without tags removed when value added'
+            ],
+            [
+                ReadonlyField::class,
+                'Something',
+                '',
+                '<ins>Something</ins> <del>$emptyPlaceholderNoTags</del>',
+                'No value is escaped without tags removed when value removed'
+            ],
+        ];
+    }
 }