Fix ss4 invalid props (#21)

* fix(ComponentTest): Prevent invalid / reserved props being passed in and add test - Github Issue #15 * fix(ComponentData): Remove unused properties and improve documentation
symbiote · Aug 2, 2018 · 3e52d15 · 3e52d15
1 parent de157b7
commit 3e52d15
Show file tree

Hide file tree

Showing 6 changed files with 113 additions and 12 deletions.
diff --git a/src/SilbinaryWolf/Components/ComponentData.php b/src/SilbinaryWolf/Components/ComponentData.php
@@ -15,21 +15,31 @@ class ComponentData extends ViewableData
      */
     protected $____name;
 
-    /**
-     * NOTE(Jake): 2018-04-06
-     *
-     * We use a custom class instead of ArrayData to avoid
-     * property clashing.
-     *
-     * (ie. passing an property 'class' to ArrayData won't work as
-     * it'll just utilize the class name "ArrayData")
-     */
     public function __construct($name, array $props)
     {
         parent::__construct();
+
+        // NOTE(Jake): 2018-08-02
+        //
+        // Don't allow use of invalid properties.
+        // ie. You can't pass "failover" as it's used by ViewableData
+        //
+        // The invalid ViewableData properties are at time of writing:
+        // - $failover
+        // - $customisedObject
+        // - $extension_instances
+        // - $beforeExtendCallbacks
+        //
+        foreach (get_object_vars($this) as $prop => $value) {
+            if (isset($props[$prop])) {
+                throw new ComponentReservedPropertyException($name, $prop);
+            }
+        }
+
+        //
         $this->____name = $name;
-        foreach ($props as $name => $value) {
-            $this->{$name} = $value;
+        foreach ($props as $prop => $value) {
+            $this->{$prop} = $value;
         }
     }
 

diff --git a/src/SilbinaryWolf/Components/ComponentException.php b/src/SilbinaryWolf/Components/ComponentException.php
@@ -0,0 +1,9 @@
+<?php
+
+namespace SilbinaryWolf\Components;
+
+use Exception;
+
+class ComponentException extends Exception
+{
+}
diff --git a/src/SilbinaryWolf/Components/ComponentReservedPropertyException.php b/src/SilbinaryWolf/Components/ComponentReservedPropertyException.php
@@ -0,0 +1,24 @@
+<?php
+
+namespace SilbinaryWolf\Components;
+
+class ComponentReservedPropertyException extends ComponentException
+{
+    /**
+     * @var string
+     */
+    private $propertyName;
+
+    /**
+     * @var string
+     */
+    private $componentName;
+
+    public function __construct($componentName, $propertyName)
+    {
+        $this->propertyName = $propertyName;
+        $this->componentName = $componentName;
+        $message = 'You cannot use the property "'.$propertyName.'" on "'.$componentName.'" as it\'s already used by ViewableData.';
+        parent::__construct($message);
+    }
+}
diff --git a/tests/ComponentTest.php b/tests/ComponentTest.php
@@ -2,6 +2,7 @@
 
 namespace SilbinaryWolf\Components\Tests;
 
+use Exception;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\View\SSViewer;
@@ -10,9 +11,10 @@
 use SilverStripe\View\ArrayData;
 use SilverStripe\Forms\TextField;
 use SilverStripe\View\ViewableData;
-use SilbinaryWolf\Components\ComponentService;
 use SilverStripe\View\SSViewer_Scope;
 use SilverStripe\Core\Injector\Injector;
+use SilbinaryWolf\Components\ComponentService;
+use SilbinaryWolf\Components\ComponentReservedPropertyException;
 
 class ComponentTest extends SapphireTest
 {
@@ -355,6 +357,60 @@ class="btn btn-secondary"
         $this->assertEqualIgnoringWhitespace($expectedHTML, $resultHTML, 'Unexpected output');
     }
 
+    /**
+     * If you don't pass a $class variable in, $class will end
+     * up defaulting to 'SilbinaryWolf\Components\ComponentData' without
+     * additional logic that prevents that (found in ComponentData class)
+     *
+     * This is most likely not necessary in SilverStripe 4.X, but it was
+     * a necessary check in SilverStripe 3.X
+     */
+    public function testComponentUsingClassButNotPassedIn()
+    {
+        $template = <<<SSTemplate
+<:ComponentUsingClassButNotPassedIn />
+SSTemplate;
+        $expectedHTML = <<<HTML
+<div class=""></div>
+HTML;
+        $resultHTML = SSViewer::fromString($template)->process(null);
+        $this->assertEqualIgnoringWhitespace($expectedHTML, $resultHTML, 'Unexpected output');
+    }
+
+    /**
+     * If you don't pass a $class variable in, $class will end
+     * up defaulting to 'SilbinaryWolf\Components\ComponentData' without
+     * additional logic that prevents that (found in ComponentData class)
+     *
+     * This is most likely not necessary in SilverStripe 4.X, but it was
+     * a necessary check in SilverStripe 3.X
+     *
+     */
+    public function testCatchReservedProperty()
+    {
+        $template = <<<SSTemplate
+<:EmptyComponent
+    failover="This is reserved by Viewable Data!"
+/>
+SSTemplate;
+        try {
+            $resultHTML = SSViewer::fromString($template)->process(null);
+        } catch (ComponentReservedPropertyException $e) {
+            // Success path!
+            $expectedMessage = 'You cannot use the property "failover" on "EmptyComponent" as it\'s already used by ViewableData.';
+            $this->assertEquals(
+                $expectedMessage,
+                $e->getMessage(),
+                'Unexpected exception message given. Was this changed? If so, please update the expected value above.'
+            );
+            return;
+        } catch (Exception $e) {
+            $this->fail('Incorrect Exception caught. Expected '.ComponentReservedPropertyException::class.' to be thrown.');
+            return;
+        }
+        $this->fail('No exception thrown. Expected '.ComponentReservedPropertyException::class.' to be thrown.');
+    }
+
     /**
      * Taken from "framework\tests\view\SSViewerTest.php"
      */

diff --git a/tests/templates/components/ComponentUsingClassButNotPassedIn.ss b/tests/templates/components/ComponentUsingClassButNotPassedIn.ss
@@ -0,0 +1 @@
+<div class="$class"></div>
diff --git a/tests/templates/components/EmptyComponent.ss b/tests/templates/components/EmptyComponent.ss
@@ -0,0 +1 @@
+<div></div>