fix(DataChangeRecord): Workaround nested array limitation with DataDi…

…fferencer (#26) * fix(DataChangeRecord): Workaround nested array limitation with DataDifferencer by only json_decode'ing 1 level down. This resolve bugs when you're storing JSON data in a 'Text' DB field via MultiValueField. * fix(DataChangeTest): Fix tests and logic surrounding getting the class from the table name * feat(DataChangeCMSTest): Add test for json_decode() / DataDifferencer bug, Update DataChangeAdmin to use class name identifier (::class)
symbiote · Jun 25, 2018 · 35d7d8c · 35d7d8c
1 parent 98b6a8f
commit 35d7d8c
Show file tree

Hide file tree

Showing 14 changed files with 377 additions and 84 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -14,6 +14,7 @@ matrix:
       env:
         - DB=MYSQL
         - PHPCS_TEST=1
+        - PHPUNIT_TEST=1
     - php: 7.0
       env:
         - DB=PGSQL
@@ -23,9 +24,6 @@ matrix:
         - PDO=1
 
 before_script:
-  #
-  # Silverstripe 4
-  #
   - phpenv rehash
   - phpenv config-rm xdebug.ini
   - composer validate
@@ -34,8 +32,4 @@ before_script:
   - composer install --prefer-dist --no-interaction --no-progress --no-suggest --optimize-autoloader --verbose --profile
 
 script:
-
-  #
-  # Silverstripe 4
-  #
   - if [[ $PHPUNIT_TEST ]]; then vendor/bin/phpunit; fi
diff --git a/docs/en/quick-start.md b/docs/en/quick-start.md
@@ -8,17 +8,17 @@ MyDataObject:
     - Symbiote\DataChange\Extension\ChangeRecordable
 ```
 
-If you are applying the extension to the SiteTree, use the SiteTreeChangeRecordable 
+If you are applying the extension to the SiteTree, use the SiteTreeChangeRecordable
 extension to record publish/unpublish actions.
 
-To track changes to many\_many relationships, you must use a custom 
-ManyManyRelationship class, as well as indicate which relationships need 
+To track changes to many\_many relationships, you must use a custom
+ManyManyRelationship class, as well as indicate which relationships need
 tracking. This can be directly configured via the Injector
 
 For example
 
 ```
-Injector:
+SilverStripe\Core\Injector\Injector:
   SilverStripe\ORM\ManyManyList:
     class: Symbiote\DataChange\Model\TrackedManyManyList
     properties:
@@ -38,7 +38,7 @@ private static $many_many = array(
 
 ## Capturing URL parameters
 
-Set the `save_request_vars` option to 1, and GET and POST vars will be recorded too. 
+Set the `save_request_vars` option to 1, and GET and POST vars will be recorded too.
 
 ```
 Symbiote\DataChange\Model\DataChangeRecord:
@@ -68,7 +68,7 @@ Symbiote\DataChange\Model\DataChangeRecord:
 
 ```
 
-Also, you may wish to blacklist some request variables from being stored 
+Also, you may wish to blacklist some request variables from being stored
 
 ```
 Symbiote\DataChange\Model\DataChangeRecord:
@@ -102,6 +102,6 @@ TeamMember:
 
 Over time, the data recorded will become overwhelming in size. May not be a problem for you, but if it is
 you can regularly prune it to retain just (N) months of data at a time. Simply create the `PruneChangesBeforeJob`
-from the QueuedJob admin section of the CMS, using a constructor param of something like "-6 months". 
+from the QueuedJob admin section of the CMS, using a constructor param of something like "-6 months".
 
-The job will restart itself to run each night, to consistently remove anything older than six months.    
+The job will restart itself to run each night, to consistently remove anything older than six months.
diff --git a/src/Admin/DataChangeAdmin.php b/src/Admin/DataChangeAdmin.php
@@ -3,16 +3,17 @@
 namespace Symbiote\DataChange\Admin;
 
 use SilverStripe\Admin\ModelAdmin;
+use Symbiote\DataChange\Model\DataChangeRecord;
 
 /**
  * @author [email protected]
  * @license BSD License http://silverstripe.org/bsd-license/
  */
 class DataChangeAdmin extends ModelAdmin
 {
-    private static $managed_models = array(
-        'Symbiote\DataChange\Model\DataChangeRecord',
-    );
+    private static $managed_models = [
+        DataChangeRecord::class,
+    ];
 
     private static $url_segment = 'datachanges';
     private static $menu_title = 'Data Changes';

diff --git a/src/Extension/ChangeRecordable.php b/src/Extension/ChangeRecordable.php
@@ -2,8 +2,8 @@
 
 namespace Symbiote\DataChange\Extension;
 
+use Symbiote\DataChange\Service\DataChangeTrackService;
 use Symbiote\DataChange\Model\DataChangeRecord;
-
 use SilverStripe\ORM\DataExtension;
 use SilverStripe\Core\Config\Config;
 
@@ -21,10 +21,11 @@ class ChangeRecordable extends DataExtension
      * @var DataChangeTrackService
      */
     public $dataChangeTrackService;
-    
+
     private static $ignored_fields = array();
-    
+
     protected $isNewObject = false;
+
     protected $changeType = 'Change';
 
     public function __construct()
@@ -66,7 +67,7 @@ public function getIgnoredFields()
             return array_combine($ignored[$class], $ignored[$class]);
         }
     }
-    
+
     public function onBeforeVersionedPublish($from, $to)
     {
         if ($this->owner->isInDB()) {

diff --git a/src/Model/DataChangeRecord.php b/src/Model/DataChangeRecord.php
@@ -100,8 +100,8 @@ public function getCMSFields($params = null)
         );
 
         if (strlen($this->Before) && strlen($this->ChangeRecordClass) && class_exists($this->ChangeRecordClass)) {
-            $before = Injector::inst()->create($this->ChangeRecordClass, json_decode($this->Before, true), true);
-            $after  = Injector::inst()->create($this->ChangeRecordClass, json_decode($this->After, true), true);
+            $before = Injector::inst()->create($this->ChangeRecordClass, $this->prepareForDataDifferencer($this->Before), true);
+            $after  = Injector::inst()->create($this->ChangeRecordClass, $this->prepareForDataDifferencer($this->After), true);
             $diff   = DataDifferencer::create($before, $after);
 
             // The solr search service injector dependency causes issues with comparison, since it has public variables that are stored in an array.
@@ -294,4 +294,19 @@ public function getMemberDetails()
             return $name;
         }
     }
+
+    private function prepareForDataDifferencer($jsonData)
+    {
+        // NOTE(Jake): 2018-06-21
+        //
+        // Data Differencer cannot handle arrays within an array,
+        //
+        // So JSON data that comes from MultiValueField / Text DB fields
+        // causes errors to be thrown.
+        //
+        // So solve this, we simply only decode to a depth of 1. (rather than the 512 default)
+        //
+        $resultJsonData = json_decode($jsonData, true, 1);
+        return $resultJsonData;
+    }
 }
diff --git a/src/Model/TrackedManyManyList.php b/src/Model/TrackedManyManyList.php
@@ -13,19 +13,21 @@
 class TrackedManyManyList extends ManyManyList
 {
     public $trackedRelationships = array();
-    
+
     public function add($item, $extraFields = array())
     {
         $this->recordManyManyChange(__FUNCTION__, $item);
-        return parent::add($item, $extraFields);
+        $result = parent::add($item, $extraFields);
+        return $result;
     }
-    
+
     public function remove($item)
     {
         $this->recordManyManyChange(__FUNCTION__, $item);
-        return parent::remove($item);
+        $result = parent::remove($item);
+        return $result;
     }
-    
+
     protected function recordManyManyChange($type, $item)
     {
         $joinName = $this->getJoinTable();
@@ -34,27 +36,50 @@ protected function recordManyManyChange($type, $item)
         }
         $parts = explode('_', $joinName);
         if (isset($parts[0]) && count($parts) > 1) {
-
             // table name could be sometihng like Symbiote_DataChange_Tests_TestObject_Kids
             // which is ClassName_RelName, with
-            array_pop($parts);
-
-            $addingToClass = implode('\\', $parts);
-            $addingTo = $this->getForeignID();
-
-            if (class_exists($addingToClass)) {
-                $onItem = $addingToClass::get()->byID($addingTo);
-                if ($onItem) {
-                    if ($item && !($item instanceof DataObject)) {
-                        $class = $this->dataClass;
-                        $item = $class::get()->byID($item);
-                    }
-                    $join = $type == 'add' ? ' to ' : ' from ';
-                    $type = ucfirst($type) . ' "' . $item->Title . '"' . $join . $parts[1];
-                    $onItem->RelatedItem = $item->ClassName . ' #' . $item->ID;
-                    singleton('DataChangeTrackService')->track($onItem, $type);
-                }
+            $tableName = $parts;
+            $relationName = array_pop($tableName);
+            $tableName = implode('_', $tableName);
+
+            $addingToClass = $this->tableClass($tableName);
+            if (!$addingToClass) {
+                return;
+            }
+            if (!class_exists($addingToClass)) {
+                return;
             }
+            $onItem = $addingToClass::get()->byID($this->getForeignID());
+            if (!$onItem) {
+                return;
+            }
+            if ($item && !($item instanceof DataObject)) {
+                $class = $this->dataClass();
+                $item = $class::get()->byID($item);
+            }
+            $join = $type === 'add' ? ' to ' : ' from ';
+            $type = ucfirst($type) . ' "' . $item->Title . '"' . $join . $relationName;
+            $onItem->RelatedItem = $item->ClassName . ' #' . $item->ID;
+            singleton('DataChangeTrackService')->track($onItem, $type);
+        }
+    }
+
+    /**
+     * Find the class for the given table.
+     *
+     * Stripped down version from framework that does not attempt to strip _Live and _versions postfixes as
+     * that throws errors in its preg_match(). (At least it did as of 2018-06-22 on SilverStripe 4.1.1)
+     *
+     * @param string $table
+     * @return string|null The FQN of the class, or null if not found
+     */
+    private function tableClass($table)
+    {
+        $tables = DataObject::getSchema()->getTableNames();
+        $class = array_search($table, $tables, true);
+        if ($class) {
+            return $class;
         }
+        return null;
     }
 }
diff --git a/src/Service/DataChangeTrackService.php b/src/Service/DataChangeTrackService.php
@@ -13,7 +13,7 @@ class DataChangeTrackService
 {
 
     protected $dcr_cache = array();
-    
+
     public $disabled = false;
 
     public function track(DataObject $object, $type = 'Change')
@@ -26,7 +26,7 @@ public function track(DataObject $object, $type = 'Change')
         if (!isset($this->dcr_cache["{$object->ID}-{$object->Classname}-$type"])) {
             $this->dcr_cache["{$object->ID}-{$object->Classname}"] = DataChangeRecord::create();
         }
-        
+
         $this->dcr_cache["{$object->ID}-{$object->Classname}"]->track($object, $type);
     }
 

diff --git a/tests/DataChangeCMSTest.php b/tests/DataChangeCMSTest.php
@@ -0,0 +1,78 @@
+<?php
+
+namespace Symbiote\DataChange\Tests;
+
+use SilverStripe\Dev\FunctionalTest;
+use SilverStripe\Control\Controller;
+use Symbiote\DataChange\Admin\DataChangeAdmin;
+
+class DataChangeCMSTest extends FunctionalTest
+{
+    protected $usesDatabase = true;
+
+    protected static $extra_dataobjects = [
+        TestTextJSONFieldObject::class,
+    ];
+
+    public function testCMSFieldsWithJSONData()
+    {
+        // Create test data
+        $record = new TestTextJSONFieldObject();
+        $record->TextFieldWithJSON = json_encode([
+            'The Pixies' => [
+                'Bossanova' => [
+                    'The Happening' => [
+                        'My head was feeling scared',
+                        'but my heart was feeling free',
+                    ]
+                ],
+            ]
+        ]);
+        $record->write();
+        $record->TextFieldWithJSON = json_encode([
+            'Radiohead' => [
+                'A Moonshaped Pool' => [
+                    'Present Tense' => [
+                        'Keep it light and',
+                        'Keep it moving',
+                        'I am doing',
+                        'No harm',
+                    ]
+                ],
+            ]
+        ]);
+        $record->write();
+
+        // Get the data change tracker record that was written in 'TestTextJSONFieldObject's onAfterWrite()
+        $dataChangeTrackRecordIds = $record->getDataChangesList()->column('ID');
+        $this->assertEquals(2, count($dataChangeTrackRecordIds));
+
+        // View in the CMS.
+        $this->logInWithPermission('ADMIN');
+        $dataChangeTrackEditID = $dataChangeTrackRecordIds[0];
+        $editLink = 'admin/datachanges/Symbiote-DataChange-Model-DataChangeRecord/EditForm/field/Symbiote-DataChange-Model-DataChangeRecord/item/'.$dataChangeTrackEditID.'/edit';
+
+        // NOTE(Jake): 2018-06-25
+        //
+        // If the test fails, you will get something like:
+        // - nl2br() expects parameter 1 to be string, array given
+        //
+        // This is because the DataDifferencer can't work wtih a 'Text' field that returns an array.
+        // ie. `TestTextJSONFieldObject` custom getter "getTextFieldWithJSON"
+        //
+        $response = $this->get($editLink);
+        $this->assertEquals(200, $response->getStatusCode());
+
+        $body = $response->getBody();
+        $this->assertTrue(
+            true,
+            strpos($body, 'Get Vars') !== false,
+            'Cannot find \'Get Vars\' field to prove that we\'re actually on the editing DataChangeRecord page.'
+        );
+        $this->assertTrue(
+            true,
+            strpos($body, 'Post Vars') !== false,
+            'Cannot find \'Post Vars\' field to prove that we\'re actually on the editing DataChangeRecord page.'
+        );
+    }
+}