FIX: Make all enums non-destructive, not just ClassName

This change also renders a portion of DBSchemaManager irrelevant, that
destructively “fixes” old values. This is in keeping with the
non-destructive principle of dev/build, and some suggestions to move
away from enum fields altogether.

Fixes https://github.com/silverstripe/silverstripe-framework/issues/1387

docs/en/04_Changelogs/4.4.0.md

@@ -6,7 +6,7 @@
  
 ## Upgrading {#upgrading}
 
-tbc
+ - dev/build is now non-destructive for all Enums, not just ClassNames. This means your data won't be lost if you're switching between versions, but watch out for code that breaks when it sees an unrecognised value!
 
 ## Changes to internal APIs
 

src/ORM/Connect/DBSchemaManager.php

@@ -713,37 +713,6 @@ MESSAGE
             $this->transCreateField($table, $field, $spec_orig);
             $this->alterationMessage("Field $table.$field: created as $spec_orig", "created");
         } elseif ($fieldValue != $specValue) {
-            // If enums/sets are being modified, then we need to fix existing data in the table.
-            // Update any records where the enum is set to a legacy value to be set to the default.
-            foreach (array('enum', 'set') as $enumtype) {
-                if (preg_match("/^$enumtype/i", $specValue)) {
-                    $newStr = preg_replace("/(^$enumtype\\s*\\(')|('\\).*)/i", "", $spec_orig);
-                    $new = preg_split("/'\\s*,\\s*'/", $newStr);
-
-                    $oldStr = preg_replace("/(^$enumtype\\s*\\(')|('\\).*)/i", "", $fieldValue);
-                    $old = preg_split("/'\\s*,\\s*'/", $oldStr);
-
-                    $holder = array();
-                    foreach ($old as $check) {
-                        if (!in_array($check, $new)) {
-                            $holder[] = $check;
-                        }
-                    }
-                    if (count($holder)) {
-                        $default = explode('default ', $spec_orig);
-                        $default = $default[1];
-                        $query = "UPDATE \"$table\" SET $field=$default WHERE $field IN (";
-                        for ($i = 0; $i + 1 < count($holder); $i++) {
-                            $query .= "'{$holder[$i]}', ";
-                        }
-                        $query .= "'{$holder[$i]}')";
-                        $this->query($query);
-                        $amount = $this->database->affectedRows();
-                        $this->alterationMessage("Changed $amount rows to default value of field $field"
-                                . " (Value: $default)");
-                    }
-                }
-            }
             $this->transAlterField($table, $field, $spec_orig);
             $this->alterationMessage(
                 "Field $table.$field: changed to $specValue <i class=\"build-info-before\">(from {$fieldValue})</i>",

src/ORM/DataObject.php

@@ -19,6 +19,7 @@ use SilverStripe\i18n\i18n;
 use SilverStripe\i18n\i18nEntityProvider;
 use SilverStripe\ORM\Connect\MySQLSchemaManager;
 use SilverStripe\ORM\FieldType\DBClassName;
+use SilverStripe\ORM\FieldType\DBEnum;
 use SilverStripe\ORM\FieldType\DBComposite;
 use SilverStripe\ORM\FieldType\DBDatetime;
 use SilverStripe\ORM\FieldType\DBField;
@@ -3197,7 +3198,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
     public static function reset()
     {
         // @todo Decouple these
-        DBClassName::clear_classname_cache();
+        DBEnum::flushCache();
         ClassInfo::reset_db_cache();
         static::getSchema()->reset();
         self::$_cache_get_one = array();

src/ORM/FieldType/DBClassName.php

@@ -6,6 +6,7 @@ use SilverStripe\Core\ClassInfo;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\ORM\DB;
+use SilverStripe\Dev\Deprecation;
 
 /**
  * Represents a classname selector, which respects obsolete clasess.
@@ -29,14 +30,6 @@ class DBClassName extends DBEnum
      */
     protected $record = null;
 
-    /**
-     * Classname spec cache for obsolete classes. The top level keys are the table, each of which contains
-     * nested arrays with keys mapped to field names. The values of the lowest level array are the classnames
-     *
-     * @var array
-     */
-    protected static $classname_cache = array();
-
     private static $index = true;
 
     /**
@@ -45,7 +38,8 @@ class DBClassName extends DBEnum
      */
     public static function clear_classname_cache()
     {
-        self::$classname_cache = array();
+        Deprecation::notice('4.3', 'Call DBEnum::flushCache() instead');
+        DBEnum::flushCache();
     }
 
     /**
@@ -149,47 +143,6 @@ class DBClassName extends DBEnum
         return array_values($classNames);
     }
 
-    /**
-     * Get the list of classnames, including obsolete classes.
-     *
-     * If table or name are not set, or if it is not a valid field on the given table,
-     * then only known classnames are returned.
-     *
-     * Values cached in this method can be cleared via `DBClassName::clear_classname_cache();`
-     *
-     * @return array
-     */
-    public function getEnumObsolete()
-    {
-        // Without a table or field specified, we can only retrieve known classes
-        $table = $this->getTable();
-        $name = $this->getName();
-        if (empty($table) || empty($name)) {
-            return $this->getEnum();
-        }
-
-        // Ensure the table level cache exists
-        if (empty(self::$classname_cache[$table])) {
-            self::$classname_cache[$table] = array();
-        }
-
-        // Check existing cache
-        if (!empty(self::$classname_cache[$table][$name])) {
-            return self::$classname_cache[$table][$name];
-        }
-
-        // Get all class names
-        $classNames = $this->getEnum();
-        if (DB::get_schema()->hasField($table, $name)) {
-            $existing = DB::query("SELECT DISTINCT \"{$name}\" FROM \"{$table}\"")->column();
-            $classNames = array_unique(array_merge($classNames, $existing));
-        }
-
-        // Cache and return
-        self::$classname_cache[$table][$name] = $classNames;
-        return $classNames;
-    }
-
     public function setValue($value, $record = null, $markChanged = true)
     {
         parent::setValue($value, $record, $markChanged);

src/ORM/FieldType/DBEnum.php

@@ -20,7 +20,7 @@ class DBEnum extends DBString
      *
      * @var array
      */
-    protected $enum = array();
+    protected $enum = [];
 
     /**
      * Default value
@@ -31,6 +31,22 @@ class DBEnum extends DBString
 
     private static $default_search_filter_class = 'ExactMatchFilter';
 
+    /**
+     * Internal cache for obsolete enum values. The top level keys are the table, each of which contains
+     * nested arrays with keys mapped to field names. The values of the lowest level array are the enum values
+     *
+     * @var array
+     */
+    protected static $enum_cache = [];
+
+    /**
+     * Clear all cached enum values.
+     */
+    public static function flushCache()
+    {
+        self::$enum_cache = [];
+    }
+
     /**
      * Create a new Enum field, which is a value within a defined set, with an optional default.
      *
@@ -88,7 +104,7 @@ class DBEnum extends DBString
 
         $parts = array(
             'datatype' => 'enum',
-            'enums' => $this->getEnum(),
+            'enums' => $this->getEnumObsolete(),
             'character set' => $charset,
             'collate' => $collation,
             'default' => $this->getDefault(),
@@ -173,6 +189,48 @@ class DBEnum extends DBString
         return $this->enum;
     }
 
+
+    /**
+     * Get the list of enum values, including obsolete values still present in the database
+     *
+     * If table or name are not set, or if it is not a valid field on the given table,
+     * then only known enum values are returned.
+     *
+     * Values cached in this method can be cleared via `DBEnum::flushCache();`
+     *
+     * @return array
+     */
+    public function getEnumObsolete()
+    {
+        // Without a table or field specified, we can only retrieve known enum values
+        $table = $this->getTable();
+        $name = $this->getName();
+        if (empty($table) || empty($name)) {
+            return $this->getEnum();
+        }
+
+        // Ensure the table level cache exists
+        if (empty(self::$enum_cache[$table])) {
+            self::$enum_cache[$table] = array();
+        }
+
+        // Check existing cache
+        if (!empty(self::$enum_cache[$table][$name])) {
+            return self::$enum_cache[$table][$name];
+        }
+
+        // Get all enum values
+        $enumValues = $this->getEnum();
+        if (DB::get_schema()->hasField($table, $name)) {
+            $existing = DB::query("SELECT DISTINCT \"{$name}\" FROM \"{$table}\"")->column();
+            $enumValues = array_unique(array_merge($enumValues, $existing));
+        }
+
+        // Cache and return
+        self::$enum_cache[$table][$name] = $enumValues;
+        return $enumValues;
+    }
+
     /**
      * Set enum options
      *

tests/php/ORM/DBEnumTest.php

@@ -5,9 +5,17 @@ namespace SilverStripe\ORM\Tests;
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\ORM\FieldType\DBEnum;
 use SilverStripe\ORM\FieldType\DBField;
+use SilverStripe\ORM\DB;
 
 class DBEnumTest extends SapphireTest
 {
+
+    protected $extraDataObjects = [
+        FieldType\DBEnumTestObject::class,
+    ];
+
+    protected $usesDatabase = true;
+
     public function testDefault()
     {
         /** @var DBEnum $enum1 */
@@ -28,4 +36,66 @@ class DBEnumTest extends SapphireTest
         $this->assertEquals('B', $enum4->getDefaultValue());
         $this->assertEquals('B', $enum4->getDefault());
     }
+
+    public function testObsoleteValues()
+    {
+        $obj = new FieldType\DBEnumTestObject();
+        $colourField = $obj->obj('Colour');
+        $colourField->setTable('FieldType_DBEnumTestObject');
+
+        // Test values prior to any database content
+        $this->assertEquals(
+            ['Red', 'Blue', 'Green'],
+            $colourField->getEnumObsolete()
+        );
+
+        // Test values with a record
+        $obj->Colour = 'Red';
+        $obj->write();
+        DBEnum::flushCache();
+
+        $this->assertEquals(
+            ['Red', 'Blue', 'Green'],
+            $colourField->getEnumObsolete()
+        );
+
+        // If the value is removed from the enum, obsolete content is still retained
+        $colourField->setEnum(['Blue', 'Green', 'Purple']);
+        DBEnum::flushCache();
+
+        $this->assertEquals(
+            ['Blue', 'Green', 'Purple', 'Red'], // Red on the end now, because it's obsolete
+            $colourField->getEnumObsolete()
+        );
+
+        // Check that old and new data is preserved after a schema update
+        DB::get_schema()->schemaUpdate(function () use ($colourField) {
+            $colourField->requireField();
+        });
+
+        $obj2 = new FieldType\DBEnumTestObject();
+        $obj2->Colour = 'Purple';
+        $obj2->write();
+
+        $this->assertEquals(
+            ['Purple', 'Red'],
+            FieldType\DBEnumTestObject::get()->sort('Colour')->column('Colour')
+        );
+
+        // Ensure that enum columns are retained
+        $colourField->setEnum(['Blue', 'Green']);
+        $this->assertEquals(
+            ['Blue', 'Green', 'Purple', 'Red'],
+            $colourField->getEnumObsolete()
+        );
+
+        // If obsolete records are deleted, the extra values go away
+        $obj->delete();
+        $obj2->delete();
+        DBEnum::flushCache();
+        $this->assertEquals(
+            ['Blue', 'Green'],
+            $colourField->getEnumObsolete()
+        );
+    }
 }

tests/php/ORM/DataObjectSchemaGenerationTest.php

@@ -5,7 +5,7 @@ namespace SilverStripe\ORM\Tests;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\ORM\Connect\MySQLSchemaManager;
 use SilverStripe\ORM\DB;
-use SilverStripe\ORM\FieldType\DBClassName;
+use SilverStripe\ORM\FieldType\DBEnum;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\ORM\Tests\DataObjectSchemaGenerationTest\SortedObject;
@@ -208,7 +208,7 @@ class DataObjectSchemaGenerationTest extends SapphireTest
         $schema = DataObject::getSchema();
 
         // Test with blank entries
-        DBClassName::clear_classname_cache();
+        DBEnum::flushCache();
         $do1 = new TestObject();
         $fields = $schema->databaseFields(TestObject::class, false);
         $this->assertEquals("DBClassName", $fields['ClassName']);
@@ -224,7 +224,7 @@ class DataObjectSchemaGenerationTest extends SapphireTest
         // Test with instance of subclass
         $item1 = new TestIndexObject();
         $item1->write();
-        DBClassName::clear_classname_cache();
+        DBEnum::flushCache();
         $this->assertEquals(
             [
                 TestObject::class,
@@ -237,7 +237,7 @@ class DataObjectSchemaGenerationTest extends SapphireTest
         // Test with instance of main class
         $item2 = new TestObject();
         $item2->write();
-        DBClassName::clear_classname_cache();
+        DBEnum::flushCache();
         $this->assertEquals(
             [
                 TestObject::class,
@@ -252,7 +252,7 @@ class DataObjectSchemaGenerationTest extends SapphireTest
         $item1->write();
         $item2 = new TestObject();
         $item2->write();
-        DBClassName::clear_classname_cache();
+        DBEnum::flushCache();
         $this->assertEquals(
             [
                 TestObject::class,

tests/php/ORM/FieldType/DBEnumTestObject.php

@@ -0,0 +1,15 @@
+<?php
+
+namespace SilverStripe\ORM\Tests\FieldType;
+
+use SilverStripe\ORM\DataObject;
+
+class DBEnumTestObject extends DataObject
+{
+
+    private static $table_name = 'FieldType_DBEnumTestObject';
+
+    private static $db = [
+        'Colour' => 'Enum("Red,Blue,Green")',
+    ];
+}

tests/php/Security/SecurityTest.php

@@ -14,7 +14,7 @@ use SilverStripe\Dev\FunctionalTest;
 use SilverStripe\i18n\i18n;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\ORM\DB;
-use SilverStripe\ORM\FieldType\DBClassName;
+use SilverStripe\ORM\FieldType\DBEnum;
 use SilverStripe\ORM\FieldType\DBDatetime;
 use SilverStripe\ORM\FieldType\DBField;
 use SilverStripe\ORM\ValidationResult;
@@ -671,7 +671,7 @@ class SecurityTest extends FunctionalTest
     public function testDatabaseIsReadyWithInsufficientMemberColumns()
     {
         Security::clear_database_is_ready();
-        DBClassName::clear_classname_cache();
+        DBEnum::flushCache();
 
         // Assumption: The database has been built correctly by the test runner,
         // and has all columns present in the ORM