From 8455a019543e9ef77ba873065b1ea459ecdffccb Mon Sep 17 00:00:00 2001 From: Murray Wood Date: Fri, 25 Oct 2024 19:09:50 +0800 Subject: [PATCH 1/3] Experiment with reverting to the expected type rather than treat everything as a string. This solves not knowing if a properties field should be decoded or not. --- _build/schema/versionx.mysql.schema.xml | 6 ++++ .../versionx/mysql/vxdeltafield.map.inc.php | 18 ++++++++++ core/components/versionx/src/DeltaManager.php | 18 ++++++++-- .../versionx/src/Fields/Properties.php | 34 +++++++++++++++++-- core/components/versionx/src/Types/Type.php | 4 +-- core/components/versionx/src/Utils.php | 8 +++-- 6 files changed, 78 insertions(+), 10 deletions(-) diff --git a/_build/schema/versionx.mysql.schema.xml b/_build/schema/versionx.mysql.schema.xml index f85ac9f..1198110 100644 --- a/_build/schema/versionx.mysql.schema.xml +++ b/_build/schema/versionx.mysql.schema.xml @@ -97,6 +97,12 @@ + + + diff --git a/core/components/versionx/model/versionx/mysql/vxdeltafield.map.inc.php b/core/components/versionx/model/versionx/mysql/vxdeltafield.map.inc.php index ccc53ae..77d810e 100644 --- a/core/components/versionx/model/versionx/mysql/vxdeltafield.map.inc.php +++ b/core/components/versionx/model/versionx/mysql/vxdeltafield.map.inc.php @@ -34,6 +34,8 @@ 'delta' => 0, 'field' => NULL, 'field_type' => 'text', + 'before_type' => '', + 'after_type' => '', 'before' => '', 'after' => '', ), @@ -63,6 +65,22 @@ 'null' => false, 'default' => 'text', ), + 'before_type' => + array ( + 'dbtype' => 'varchar', + 'precision' => '20', + 'phptype' => 'string', + 'null' => false, + 'default' => '', + ), + 'after_type' => + array ( + 'dbtype' => 'varchar', + 'precision' => '20', + 'phptype' => 'string', + 'null' => false, + 'default' => '', + ), 'before' => array ( 'dbtype' => 'mediumtext', diff --git a/core/components/versionx/src/DeltaManager.php b/core/components/versionx/src/DeltaManager.php index d7cc70d..3419e31 100644 --- a/core/components/versionx/src/DeltaManager.php +++ b/core/components/versionx/src/DeltaManager.php @@ -8,7 +8,8 @@ use modmore\VersionX\Enums\RevertAction; use MODX\Revolution\modX; -class DeltaManager { +class DeltaManager +{ public VersionX $versionX; /** @var \modX|modX */ public $modx; @@ -117,13 +118,22 @@ public function createDelta(int $id, Type $type, bool $isSnapshot = false): ?\vx $fieldType = $type->getFieldClass($field); $fieldTypeObj = new $fieldType($value); - // - $value = Utils::flattenArray($fieldTypeObj->getValue()); + // Flatten any arrays and set the value as a string + $value = $fieldTypeObj->getValue(); + $valueType = gettype($value); + + if (is_array($value)) { + $value = serialize($value); + } else { + $value = (string)$value; + } // If a previous delta exists, get the "after" value. Otherwise, use a blank string. $prevValue = ''; + $prevValueType = ''; if ($prevDelta && isset($prevFields[$field])) { $prevValue = $prevFields[$field]->get('after'); + $prevValueType = $prevFields[$field]->get('after_type'); } try { @@ -139,6 +149,8 @@ public function createDelta(int $id, Type $type, bool $isSnapshot = false): ?\vx $deltaField = $this->modx->newObject(\vxDeltaField::class, [ 'field' => $field, 'field_type' => $fieldType, + 'before_type' => $prevValueType, + 'after_type' => $valueType !== 'string' ? $valueType : '', 'before' => $prevValue, 'after' => $value, 'diff' => $renderedDiff, // Not persisted. Kept on object until cached. diff --git a/core/components/versionx/src/Fields/Properties.php b/core/components/versionx/src/Fields/Properties.php index 6f57993..8c0650f 100644 --- a/core/components/versionx/src/Fields/Properties.php +++ b/core/components/versionx/src/Fields/Properties.php @@ -12,6 +12,7 @@ public function parse() } /** + * * @param array $arrayField * @param string $name * @param array $fields @@ -21,6 +22,10 @@ public static function splitPropertyValues(array $arrayField, string $name = '', { $arrays = []; foreach ($arrayField as $field => $value) { + if (empty($name)) { + $fields[$field] = $value; + continue; + } if (is_numeric($field)) { $fields[$name][$field] = $value; continue; @@ -47,15 +52,40 @@ public static function splitPropertyValues(array $arrayField, string $name = '', */ public static function revertPropertyValue(\vxDeltaField $field, &$data) { + if (!is_array($data)) { + return $data; + } + $pieces = explode('.', $field->get('field')); $last = end($pieces); foreach ($pieces as $piece) { - if (!is_array($data) || !array_key_exists($piece, $data)) { + if (!array_key_exists($piece, $data)) { continue; } if ($piece === $last) { - $data[$piece] = $field->get('before'); +// $json = json_decode($field->get('before'), true); +// if (json_last_error() === JSON_ERROR_NONE) { +// $data[$piece] = $json; +// } + + $beforeValue = $field->get('before'); + $beforeType = $field->get('before_type'); + + if ($beforeType === 'array') { + // Unserialize if it's meant to be an array + $unserialized = unserialize($beforeValue); + $data[$piece] = $unserialized !== false ? $unserialized : $field->get('before'); + } + else if (in_array($beforeType, ['boolean', 'bool', 'integer', 'int', 'float', 'double'])) { + // Cast as the set type + $data[$piece] = settype($beforeValue, $beforeType); + } + else { + // If we're here treat as a string + $data[$piece] = $field->get('before'); + } + } else { $data = &$data[$piece]; diff --git a/core/components/versionx/src/Types/Type.php b/core/components/versionx/src/Types/Type.php index 0913c9f..88e9bdb 100644 --- a/core/components/versionx/src/Types/Type.php +++ b/core/components/versionx/src/Types/Type.php @@ -184,7 +184,7 @@ public function afterDeltaCreate(\vxDelta $delta, \xPDOObject $object): ?\vxDelt } /** - * Runs before object being reverted is saved. + * Runs after main object revert, and before the object is saved. * @param string $action - 'all', 'delta', or 'single' * @param array $fields - the delta fields that are being saved to the object * @param \xPDOObject $object - the object being reverted @@ -287,7 +287,7 @@ protected function savePropertiesFields(\vxDeltaField $field, \xPDOObject $objec ) { // Take current properties field value and insert the "before" version at matching array keys. $data = $object->get($propField); - Properties::revertPropertyValue($field, $data, 'before'); + Properties::revertPropertyValue($field, $data); $object->set($propField, $data); $object->save(); } diff --git a/core/components/versionx/src/Utils.php b/core/components/versionx/src/Utils.php index b64a31c..05f41bc 100644 --- a/core/components/versionx/src/Utils.php +++ b/core/components/versionx/src/Utils.php @@ -2,8 +2,8 @@ namespace modmore\VersionX; -class Utils { - +class Utils +{ /** * Flattens an array recursively, and returns other values as a string * @param mixed $array @@ -12,7 +12,9 @@ class Utils { */ public static function flattenArray($array = []): string { - if (!is_array($array)) return (string)$array; + if (!is_array($array)) { + return (string)$array; + } $string = []; foreach ($array as $key => $value) { From d6fac550173deac3aab68e0feef5ff627fb00cd7 Mon Sep 17 00:00:00 2001 From: Murray Wood Date: Sat, 26 Oct 2024 09:39:55 +0800 Subject: [PATCH 2/3] Add field updates to resolver --- _build/resolvers/tables.resolver.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/_build/resolvers/tables.resolver.php b/_build/resolvers/tables.resolver.php index 7a7071a..9fd8573 100644 --- a/_build/resolvers/tables.resolver.php +++ b/_build/resolvers/tables.resolver.php @@ -30,6 +30,14 @@ $manager->createObjectContainer($obj); } + // Set to fatal errors only while updating database, to avoid false positives displayed. + $modx->setLogLevel(modX::LOG_LEVEL_FATAL); + + // VersionX 3.1.1 + // These fields are added to keep track of data types to be used when reverting + $manager->addField('vxDeltaField', 'before_type', ['after' => 'field_type']); + $manager->addField('vxDeltaField', 'after_type', ['after' => 'before_type']); + $modx->setLogLevel($loglevel); break; From 8d4a8501e4804e01df04b25680144bfee40a7f0f Mon Sep 17 00:00:00 2001 From: Murray Wood Date: Fri, 1 Nov 2024 15:13:01 +0800 Subject: [PATCH 3/3] Switch from serialize to json_encode. Restrict via a list of accepted value types. Shrink varchar size to 7. Code tweaks. --- _build/schema/versionx.mysql.schema.xml | 4 ++-- .../versionx/mysql/vxdeltafield.map.inc.php | 4 ++-- core/components/versionx/src/DeltaManager.php | 14 +++++++++----- core/components/versionx/src/Fields/Field.php | 12 ++++++++++++ .../versionx/src/Fields/Properties.php | 16 ++++++---------- 5 files changed, 31 insertions(+), 19 deletions(-) diff --git a/_build/schema/versionx.mysql.schema.xml b/_build/schema/versionx.mysql.schema.xml index 1198110..ffb71dc 100644 --- a/_build/schema/versionx.mysql.schema.xml +++ b/_build/schema/versionx.mysql.schema.xml @@ -101,8 +101,8 @@ Keep track of the before and after value types so reverting doesn't decode an intended json string, or cast an int as a string etc. Empty value means it's a string. --> - - + + diff --git a/core/components/versionx/model/versionx/mysql/vxdeltafield.map.inc.php b/core/components/versionx/model/versionx/mysql/vxdeltafield.map.inc.php index 77d810e..294886b 100644 --- a/core/components/versionx/model/versionx/mysql/vxdeltafield.map.inc.php +++ b/core/components/versionx/model/versionx/mysql/vxdeltafield.map.inc.php @@ -68,7 +68,7 @@ 'before_type' => array ( 'dbtype' => 'varchar', - 'precision' => '20', + 'precision' => '7', 'phptype' => 'string', 'null' => false, 'default' => '', @@ -76,7 +76,7 @@ 'after_type' => array ( 'dbtype' => 'varchar', - 'precision' => '20', + 'precision' => '7', 'phptype' => 'string', 'null' => false, 'default' => '', diff --git a/core/components/versionx/src/DeltaManager.php b/core/components/versionx/src/DeltaManager.php index 3419e31..4c88913 100644 --- a/core/components/versionx/src/DeltaManager.php +++ b/core/components/versionx/src/DeltaManager.php @@ -4,6 +4,7 @@ use Carbon\Carbon; use Jfcherng\Diff\DiffHelper; +use modmore\VersionX\Fields\Field; use modmore\VersionX\Types\Type; use modmore\VersionX\Enums\RevertAction; use MODX\Revolution\modX; @@ -31,7 +32,6 @@ class DeltaManager 'showHeader' => false, ]; - function __construct(VersionX $versionX) { $this->versionX = $versionX; @@ -118,12 +118,16 @@ public function createDelta(int $id, Type $type, bool $isSnapshot = false): ?\vx $fieldType = $type->getFieldClass($field); $fieldTypeObj = new $fieldType($value); - // Flatten any arrays and set the value as a string + // Get the value and the value type $value = $fieldTypeObj->getValue(); $valueType = gettype($value); + // If the value is not an accepted type, default to string + if (!in_array(strtolower($valueType), Field::ACCEPTED_VALUE_TYPES)) { + $valueType = ''; + } - if (is_array($value)) { - $value = serialize($value); + if (is_array($value) || is_object($value)) { + $value = json_encode($value); } else { $value = (string)$value; } @@ -150,7 +154,7 @@ public function createDelta(int $id, Type $type, bool $isSnapshot = false): ?\vx 'field' => $field, 'field_type' => $fieldType, 'before_type' => $prevValueType, - 'after_type' => $valueType !== 'string' ? $valueType : '', + 'after_type' => $valueType, 'before' => $prevValue, 'after' => $value, 'diff' => $renderedDiff, // Not persisted. Kept on object until cached. diff --git a/core/components/versionx/src/Fields/Field.php b/core/components/versionx/src/Fields/Field.php index 1158651..ca0da4f 100644 --- a/core/components/versionx/src/Fields/Field.php +++ b/core/components/versionx/src/Fields/Field.php @@ -12,6 +12,18 @@ abstract class Field protected array $options = []; protected string $tpl = ''; + public const ACCEPTED_VALUE_TYPES = [ + 'array', + 'object', + 'boolean', + 'bool', + 'integer', + 'int', + 'float', + 'double', + 'null' + ]; + function __construct($value, $name = '', $options = []) { if (!$this->value) { diff --git a/core/components/versionx/src/Fields/Properties.php b/core/components/versionx/src/Fields/Properties.php index 8c0650f..2017d60 100644 --- a/core/components/versionx/src/Fields/Properties.php +++ b/core/components/versionx/src/Fields/Properties.php @@ -63,21 +63,17 @@ public static function revertPropertyValue(\vxDeltaField $field, &$data) continue; } + // The last 'piece' will be the key we're after. if ($piece === $last) { -// $json = json_decode($field->get('before'), true); -// if (json_last_error() === JSON_ERROR_NONE) { -// $data[$piece] = $json; -// } - $beforeValue = $field->get('before'); $beforeType = $field->get('before_type'); - if ($beforeType === 'array') { - // Unserialize if it's meant to be an array - $unserialized = unserialize($beforeValue); - $data[$piece] = $unserialized !== false ? $unserialized : $field->get('before'); + if (in_array($beforeType, ['array', 'object'])) { + // Decode if it's meant to be an array/object + $decoded = json_decode($beforeValue, true); + $data[$piece] = $decoded ?: $field->get('before'); } - else if (in_array($beforeType, ['boolean', 'bool', 'integer', 'int', 'float', 'double'])) { + else if (in_array(strtolower($beforeType), self::ACCEPTED_VALUE_TYPES)) { // Cast as the set type $data[$piece] = settype($beforeValue, $beforeType); }