diff --git a/model/DataObject.php b/model/DataObject.php index 8a3180114..f1ffa71a2 100644 --- a/model/DataObject.php +++ b/model/DataObject.php @@ -1793,18 +1793,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity return $this->hasOneComponent($component); } - $items = (array)Config::inst()->get($this->class, 'has_one', Config::INHERITED); - - // Validate the data - foreach($items as $k => $v) { - if(!is_string($k) || is_numeric($k) || !is_string($v)) { - throw new LogicException("$this->class::\$has_one has a bad entry: " - . var_export($k, true). " => " . var_export($v, true) . ". Each map key should be a - relationship name, and the map value should be the data class to join to."); - } - } - - return $items; + return (array)Config::inst()->get($this->class, 'has_one', Config::INHERITED); } /** @@ -1911,15 +1900,6 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity return $dbItems[$fieldName]; } } else { - // Validate the data - foreach($dbItems as $k => $v) { - if(!is_string($k) || is_numeric($k) || !is_string($v)) { - throw new LogicException("$class::\$db has a bad entry: " - . var_export($k, true). " => " . var_export($v, true) . ". Each map key should be a - property name, and the map value should be the property type."); - } - } - $items = isset($items) ? array_merge((array) $items, $dbItems) : $dbItems; } } @@ -2025,16 +2005,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity return $this->manyManyExtraFieldsForComponent($component); } - $items = (array)Config::inst()->get($this->class, 'many_many_extraFields', Config::INHERITED); - foreach($items as $k => $v) { - if(!is_array($v)) { - throw new LogicException("$this->class::\$many_many_extraFields has a bad entry: " - . var_export($k, true) . " => " . var_export($v, true) - . ". Each many_many_extraFields entry should map to a field specification array."); - } - } - - return !empty($items) ? $items : null; + return Config::inst()->get($this->class, 'many_many_extraFields', Config::INHERITED); } /** @@ -2118,24 +2089,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity } $manyManys = (array)Config::inst()->get($this->class, 'many_many', Config::INHERITED); - // Validate the data - foreach($manyManys as $k => $v) { - if(!is_string($k) || is_numeric($k) || !is_string($v)) { - throw new LogicException("$this->class::\$many_many has a bad entry: " - . var_export($k, true). " => " . var_export($v, true) . ". Each map key should be a - relationship name, and the map value should be the data class to join to."); - } - } - $belongsManyManys = (array)Config::inst()->get($this->class, 'belongs_many_many', Config::INHERITED); - // Validate the data - foreach($belongsManyManys as $k => $v) { - if(!is_string($k) || is_numeric($k) || !is_string($v)) { - throw new LogicException("$this->class::\$belongs_many_many has a bad entry: " - . var_export($k, true). " => " . var_export($v, true) . ". Each map key should be a - relationship name, and the map value should be the data class to join to."); - } - } $items = array_merge($manyManys, $belongsManyManys); return $items; @@ -3355,6 +3309,9 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity $indexes = $this->databaseIndexes(); + // Validate relationship configuration + $this->validateModelDefinitions(); + if($fields) { $hasAutoIncPK = ($this->class == ClassInfo::baseDataClass($this->class)); DB::require_table($this->class, $fields, $indexes, $hasAutoIncPK, $this->stat('create_table_options'), @@ -3391,6 +3348,42 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity $this->extend('augmentDatabase', $dummy); } + /** + * Validate that the configured relations for this class use the correct syntaxes + * @throws LogicException + */ + protected function validateModelDefinitions() { + $modelDefinitions = array( + 'db' => Config::inst()->get($this->class, 'db', Config::UNINHERITED), + 'has_one' => Config::inst()->get($this->class, 'has_one', Config::UNINHERITED), + 'has_many' => Config::inst()->get($this->class, 'has_many', Config::UNINHERITED), + 'belongs_to' => Config::inst()->get($this->class, 'belongs_to', Config::UNINHERITED), + 'many_many' => Config::inst()->get($this->class, 'many_many', Config::UNINHERITED), + 'belongs_many_many' => Config::inst()->get($this->class, 'belongs_many_many', Config::UNINHERITED), + 'many_many_extraFields' => Config::inst()->get($this->class, 'many_many_extraFields', Config::UNINHERITED) + ); + + foreach($modelDefinitions as $defType => $relations) { + if( ! $relations) continue; + + foreach($relations as $k => $v) { + if($defType === 'many_many_extraFields') { + if(!is_array($v)) { + throw new LogicException("$this->class::\$many_many_extraFields has a bad entry: " + . var_export($k, true) . " => " . var_export($v, true) + . ". Each many_many_extraFields entry should map to a field specification array."); + } + } else { + if(!is_string($k) || is_numeric($k) || !is_string($v)) { + throw new LogicException("$this->class::$defType has a bad entry: " + . var_export($k, true). " => " . var_export($v, true) . ". Each map key should be a + relationship name, and the map value should be the data class to join to."); + } + } + } + } + } + /** * Add default records to database. This function is called whenever the * database is built, after the database tables have all been created. Overload diff --git a/tests/model/DataObjectTest.php b/tests/model/DataObjectTest.php index dad3e1cc0..4401fe995 100644 --- a/tests/model/DataObjectTest.php +++ b/tests/model/DataObjectTest.php @@ -1062,6 +1062,86 @@ class DataObjectTest extends SapphireTest { ); } + protected function makeAccessible($object, $method) { + $reflectionMethod = new ReflectionMethod($object, $method); + $reflectionMethod->setAccessible(true); + return $reflectionMethod; + } + + public function testValidateModelDefinitionsFailsWithArray() { + Config::nest(); + + $object = new DataObjectTest_Team; + $method = $this->makeAccessible($object, 'validateModelDefinitions'); + + Config::inst()->update('DataObjectTest_Team', 'has_one', array('NotValid' => array('NoArraysAllowed'))); + $this->setExpectedException('LogicException'); + + try { + $method->invoke($object); + } catch(Exception $e) { + Config::unnest(); // Catch the exception so we can unnest config before failing the test + throw $e; + } + } + + public function testValidateModelDefinitionsFailsWithIntKey() { + Config::nest(); + + $object = new DataObjectTest_Team; + $method = $this->makeAccessible($object, 'validateModelDefinitions'); + + Config::inst()->update('DataObjectTest_Team', 'has_many', array(12 => 'DataObjectTest_Player')); + $this->setExpectedException('LogicException'); + + try { + $method->invoke($object); + } catch(Exception $e) { + Config::unnest(); // Catch the exception so we can unnest config before failing the test + throw $e; + } + } + + public function testValidateModelDefinitionsFailsWithIntValue() { + Config::nest(); + + $object = new DataObjectTest_Team; + $method = $this->makeAccessible($object, 'validateModelDefinitions'); + + Config::inst()->update('DataObjectTest_Team', 'many_many', array('Players' => 12)); + $this->setExpectedException('LogicException'); + + try { + $method->invoke($object); + } catch(Exception $e) { + Config::unnest(); // Catch the exception so we can unnest config before failing the test + throw $e; + } + } + + /** + * many_many_extraFields is allowed to have an array value, so shouldn't throw an exception + */ + public function testValidateModelDefinitionsPassesWithExtraFields() { + Config::nest(); + + $object = new DataObjectTest_Team; + $method = $this->makeAccessible($object, 'validateModelDefinitions'); + + Config::inst()->update('DataObjectTest_Team', 'many_many_extraFields', + array('Relations' => array('Price' => 'Int'))); + + try { + $method->invoke($object); + } catch(Exception $e) { + Config::unnest(); + $this->fail('Exception should not be thrown'); + throw $e; + } + + Config::unnest(); + } + public function testNewClassInstance() { $dataObject = $this->objFromFixture('DataObjectTest_Team', 'team1'); $changedDO = $dataObject->newClassInstance('DataObjectTest_SubTeam');