Merge pull request #782 from tractorcow/3.0-index-generation-fixes

FIXED: Generation of table indexes

model/Database.php

@@ -421,14 +421,6 @@ abstract class SS_Database {
 			}
 		}
 		
-		//We need to include the name of the fulltext index here so we can trigger a rebuild
-		//if either the name or the columns have changed.
-		if(is_array($spec) && isset($spec['type'])){
-			if($spec['type']=='fulltext'){
-				$array_spec="({$spec['name']},{$spec['value']})";
-			}
-		}
-		
 		if($newTable || !isset($this->indexList[$table][$index_alt])) {
 			$this->transCreateIndex($table, $index, $spec);
 			$this->alterationMessage("Index $table.$index: created as " . DB::getConn()->convertIndexSpec($spec),"created");

model/Versioned.php

@@ -70,7 +70,7 @@ class Versioned extends DataExtension {
 	 * @var array $indexes_for_versions_table
 	 */
 	static $indexes_for_versions_table = array(
-		'RecordID_Version' => '(RecordID,Version)',
+		'RecordID_Version' => '("RecordID","Version")',
 		'RecordID' => true,
 		'Version' => true,
 		'AuthorID' => true,

search/FulltextSearchable.php

@@ -43,8 +43,8 @@ class FulltextSearchable extends DataExtension {
 	 */
 	static function enable($searchableClasses = array('SiteTree', 'File')) {
 		$defaultColumns = array(
-			'SiteTree' => 'Title,MenuTitle,Content,MetaTitle,MetaDescription,MetaKeywords',
+			'SiteTree' => '"Title","MenuTitle","Content","MetaTitle","MetaDescription","MetaKeywords"',
-			'File' => 'Filename,Title,Content'
+			'File' => '"Filename","Title","Content"'
 		);
 
 		if(!is_array($searchableClasses)) $searchableClasses = array($searchableClasses);
@@ -69,7 +69,7 @@ class FulltextSearchable extends DataExtension {
 	 *  that can be searched on. Used for generation of the database index defintions.
 	 */
 	function __construct($searchFields = array()) {
-		if(is_array($searchFields)) $this->searchFields = implode(',', $searchFields);
+		if(is_array($searchFields)) $this->searchFields = '"'.implode('","', $searchFields).'"';
 		else $this->searchFields = $searchFields;
 		
 		parent::__construct();

tests/model/DataObjectSchemaGenerationTest.php

@@ -3,28 +3,117 @@
 class DataObjectSchemaGenerationTest extends SapphireTest {
 	protected $extraDataObjects = array(
 		'DataObjectSchemaGenerationTest_DO',
+		'DataObjectSchemaGenerationTest_IndexDO'
 	);
 	
+	function setUpOnce() {
+		
+		// enable fulltext option on this table
+		Config::inst()->update('DataObjectSchemaGenerationTest_IndexDO', 'create_table_options', array('MySQLDatabase' => 'ENGINE=MyISAM'));
+		
+		parent::setUpOnce();
+	}
+
 	/**
 	 * Check that once a schema has been generated, then it doesn't need any more updating
 	 */
 	function testFieldsDontRerequestChanges() {
 		$db = DB::getConn();
 		DB::quiet();
-		
 
 		// Table will have been initially created by the $extraDataObjects setting
 		
-		
 		// Verify that it doesn't need to be recreated
 		$db->beginSchemaUpdate();
 		$obj = new DataObjectSchemaGenerationTest_DO();
 		$obj->requireTable();
 		$needsUpdating = $db->doesSchemaNeedUpdating();
 		$db->cancelSchemaUpdate();
-		
 		$this->assertFalse($needsUpdating);
 	}
+
+	/**
+	 * Check that updates to a class fields are reflected in the database
+	 */
+	function testFieldsRequestChanges() {
+		$db = DB::getConn();
+		DB::quiet();
+
+		// Table will have been initially created by the $extraDataObjects setting
+		
+		// Let's insert a new field here
+		$oldDB = DataObjectSchemaGenerationTest_DO::$db;
+		DataObjectSchemaGenerationTest_DO::$db['SecretField'] = 'Varchar(100)';
+		
+		// Verify that the above extra field triggered a schema update
+		$db->beginSchemaUpdate();
+		$obj = new DataObjectSchemaGenerationTest_DO();
+		$obj->requireTable();
+		$needsUpdating = $db->doesSchemaNeedUpdating();
+		$db->cancelSchemaUpdate();
+		$this->assertTrue($needsUpdating);
+		
+		// Restore db configuration
+		DataObjectSchemaGenerationTest_DO::$db = $oldDB;
+	}
+	
+	/**
+	 * Check that indexes on a newly generated class do not subsequently request modification 
+	 */
+	function testIndexesDontRerequestChanges() {
+		$db = DB::getConn();
+		DB::quiet();
+		
+		// Table will have been initially created by the $extraDataObjects setting
+		
+		// Verify that it doesn't need to be recreated
+		$db->beginSchemaUpdate();
+		$obj = new DataObjectSchemaGenerationTest_IndexDO();
+		$obj->requireTable();
+		$needsUpdating = $db->doesSchemaNeedUpdating();
+		$db->cancelSchemaUpdate();
+		$this->assertFalse($needsUpdating);
+		
+		// Test with alternate index format, although these indexes are the same
+		$oldIndexes = DataObjectSchemaGenerationTest_IndexDO::$indexes;
+		DataObjectSchemaGenerationTest_IndexDO::$indexes = DataObjectSchemaGenerationTest_IndexDO::$indexes_alt;
+				
+		// Verify that it still doesn't need to be recreated
+		$db->beginSchemaUpdate();
+		$obj2 = new DataObjectSchemaGenerationTest_IndexDO();
+		$obj2->requireTable();
+		$needsUpdating = $db->doesSchemaNeedUpdating();
+		$db->cancelSchemaUpdate();
+		$this->assertFalse($needsUpdating);
+		
+		// Restore old index format
+		DataObjectSchemaGenerationTest_IndexDO::$indexes = $oldIndexes;
+	}
+	
+	/**
+	 * Check that updates to a dataobject's indexes are reflected in DDL
+	 */
+	function testIndexesRerequestChanges() {
+		$db = DB::getConn();
+		DB::quiet();
+		
+		// Table will have been initially created by the $extraDataObjects setting
+		
+		// Update the SearchFields index here
+		$oldIndexes = DataObjectSchemaGenerationTest_IndexDO::$indexes;
+		DataObjectSchemaGenerationTest_IndexDO::$indexes['SearchFields']['value'] = '"Title"';
+		
+		// Verify that the above index change triggered a schema update
+		$db->beginSchemaUpdate();
+		$obj = new DataObjectSchemaGenerationTest_IndexDO();
+		$obj->requireTable();
+		$needsUpdating = $db->doesSchemaNeedUpdating();
+		$db->cancelSchemaUpdate();
+		$this->assertTrue($needsUpdating);
+		
+		// Restore old indexes
+		DataObjectSchemaGenerationTest_IndexDO::$indexes = $oldIndexes;
+	}
 }
 
 class DataObjectSchemaGenerationTest_DO extends DataObject implements TestOnly {
@@ -32,5 +121,30 @@ class DataObjectSchemaGenerationTest_DO extends DataObject implements TestOnly {
 		'Enum1' => 'Enum("A, B, C, D","")',
 		'Enum2' => 'Enum("A, B, C, D","A")',
 	);
-
 }
+
+
+class DataObjectSchemaGenerationTest_IndexDO extends DataObjectSchemaGenerationTest_DO implements TestOnly {
+	static $db = array(
+		'Title' => 'Varchar(255)',
+		'Content' => 'Text'
+	);
+
+	static $indexes = array(
+		'NameIndex' => 'unique ("Title")',
+		'SearchFields' => array(
+			'type' => 'fulltext',
+			'name' => 'SearchFields',
+			'value' => '"Title","Content"'
+		)
+	);
+	
+	static $indexes_alt = array(
+		'NameIndex' => array(
+			'type' => 'unique',
+			'name' => 'NameIndex',
+			'value' => '"Title"'
+		),
+		'SearchFields' => 'fulltext ("Title","Content")'
+	);
+}

tests/search/FulltextSearchableTest.php

@@ -12,12 +12,12 @@ class FulltextSearchableTest extends SapphireTest {
 		$this->orig['File_searchable'] = Object::has_extension('File', 'FulltextSearchable');
 		
 		// TODO This shouldn't need all arguments included
-		Object::remove_extension('File', 'FulltextSearchable(\'Filename,Title,Content\')');
+		Object::remove_extension('File', 'FulltextSearchable(\'"Filename","Title","Content"\')');
 	}
 	
 	function tearDown() {
 		// TODO This shouldn't need all arguments included
-		if($this->orig['File_searchable']) Object::add_extension('File', 'FulltextSearchable(\'Filename,Title,Content\')');
+		if($this->orig['File_searchable']) Object::add_extension('File', 'FulltextSearchable(\'"Filename","Title","Content"\')');
 		
 		parent::tearDown();
 	}
@@ -32,7 +32,7 @@ class FulltextSearchableTest extends SapphireTest {
 		$this->assertTrue(Object::has_extension('File', 'FulltextSearchable'));
 
 		// TODO This shouldn't need all arguments included
-		Object::remove_extension('File', 'FulltextSearchable(\'Filename,Title,Content\')');
+		Object::remove_extension('File', 'FulltextSearchable(\'"Filename","Title","Content"\')');
 		
 		$this->assertFalse(Object::has_extension('File', 'FulltextSearchable'));
 	}