Skip to content

Commit

Permalink
FIX #2174: SearchFilter needs casting helper for DataObject base fields
Browse files Browse the repository at this point in the history
Commit 964b3f2 fixed an issue where dbObject was returning casting helpers for
fields that were not actually DB objects, but had something in $casting config.

However, because dbObject was no longer calling DataObject->castingHelper, this
exposed a bug that the underlying function db($fieldName) was not returning
field specs for the base fields that are created by SS automatically on all
DataObjects (i.e. Created, LastEdited, etc).

This commit fixes the underlying issue that DataObject->db($fieldName) should
return the field specs for *all* DB fields like its documentation says it will,
including those base fields that are automatically created and do not appear in
$db.
  • Loading branch information
jthomerson committed Jul 3, 2013
1 parent 429bbc5 commit 50e9eee
Show file tree
Hide file tree
Showing 4 changed files with 82 additions and 18 deletions.
41 changes: 26 additions & 15 deletions model/DataObject.php
Expand Up @@ -156,6 +156,14 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
protected static $_cache_custom_database_fields = array();
protected static $_cache_field_labels = array();

// base fields which are not defined in static $db
private static $fixed_fields = array(
'ID' => 'Int',
'ClassName' => 'Enum',
'LastEdited' => 'SS_Datetime',
'Created' => 'SS_Datetime',
);

/**
* Non-static relationship cache, indexed by component name.
*/
Expand Down Expand Up @@ -223,6 +231,8 @@ public static function database_fields($class) {
}

return array_merge (
// TODO: should this be using self::$fixed_fields? only difference is ID field
// and ClassName creates an Enum with all values
array (
'ClassName' => self::$classname_spec_cache[$class],
'Created' => 'SS_Datetime',
Expand Down Expand Up @@ -1651,11 +1661,16 @@ public function belongs_to($component = null, $classOnly = true) {
/**
* Return all of the database fields defined in self::$db and all the parent classes.
* Doesn't include any fields specified by self::$has_one. Use $this->has_one() to get these fields
* Also returns "base" fields like "Created", "LastEdited", et cetera.
*
* @param string $fieldName Limit the output to a specific field name
* @return array The database fields
*/
public function db($fieldName = null) {
if ($fieldName && array_key_exists($fieldName, self::$fixed_fields)) {
return self::$fixed_fields[$fieldName];
}

$classes = ClassInfo::ancestry($this);
$good = false;
$items = array();
Expand Down Expand Up @@ -1694,6 +1709,10 @@ public function db($fieldName = null) {
}
}

if (!$fieldName) {
// trying to get all fields, so add the fixed fields to return value
$items = array_merge(self::$fixed_fields, $items);
}
return $items;
}

Expand Down Expand Up @@ -2387,15 +2406,7 @@ public function hasField($field) {
* @return boolean
*/
public function hasDatabaseField($field) {
// Add base fields which are not defined in static $db
static $fixedFields = array(
'ID' => 'Int',
'ClassName' => 'Enum',
'LastEdited' => 'SS_Datetime',
'Created' => 'SS_Datetime',
);

if(isset($fixedFields[$field])) return true;
if(isset(self::$fixed_fields[$field])) return true;

return array_key_exists($field, $this->inheritedDatabaseFields());
}
Expand Down Expand Up @@ -2658,7 +2669,12 @@ public function dbObject($fieldName) {
// Special case for ID field
} else if($fieldName == 'ID') {
return new PrimaryKey($fieldName, $this);


// Special case for ClassName
} else if($fieldName == 'ClassName') {
$val = get_class($this);
return DBField::create_field('Varchar', $val, $fieldName, $this);

// General casting information for items in $db
} else if($helper = $this->db($fieldName)) {
$obj = Object::create_from_string($helper, $fieldName);
Expand All @@ -2669,11 +2685,6 @@ public function dbObject($fieldName) {
} else if(preg_match('/ID$/', $fieldName) && $this->has_one(substr($fieldName,0,-2))) {
$val = $this->$fieldName;
return DBField::create_field('ForeignKey', $val, $fieldName, $this);

// Special case for ClassName
} else if($fieldName == 'ClassName') {
$val = get_class($this);
return DBField::create_field('Varchar', $val, $fieldName, $this);
}
}

Expand Down
2 changes: 1 addition & 1 deletion search/filters/SearchFilter.php
Expand Up @@ -300,4 +300,4 @@ protected function getCaseSensitive() {
else return null;
}

}
}
42 changes: 41 additions & 1 deletion tests/model/DataListTest.php
Expand Up @@ -21,7 +21,47 @@ class DataListTest extends SapphireTest {
'DataObjectTest_TeamComment',
'DataObjectTest\NamespacedClass',
);


public function testFilterDataObjectByCreatedDate() {
// create an object to test with
$obj1 = new DataObjectTest_ValidatedObject();
$obj1->Name = 'test obj 1';
$obj1->write();
$this->assertTrue($obj1->isInDB());

// reload the object from the database and reset its Created timestamp to a known value
$obj1 = DataObjectTest_ValidatedObject::get()->filter(array('Name' => 'test obj 1'))->first();
$this->assertTrue(is_object($obj1));
$this->assertEquals('test obj 1', $obj1->Name);
$obj1->Created = '2013-01-01 00:00:00';
$obj1->write();

// reload the object again and make sure that our Created date was properly persisted
$obj1 = DataObjectTest_ValidatedObject::get()->filter(array('Name' => 'test obj 1'))->first();
$this->assertTrue(is_object($obj1));
$this->assertEquals('test obj 1', $obj1->Name);
$this->assertEquals('2013-01-01 00:00:00', $obj1->Created);

// now save a second object to the DB with an automatically-set Created value
$obj2 = new DataObjectTest_ValidatedObject();
$obj2->Name = 'test obj 2';
$obj2->write();
$this->assertTrue($obj2->isInDB());

// and a third object
$obj3 = new DataObjectTest_ValidatedObject();
$obj3->Name = 'test obj 3';
$obj3->write();
$this->assertTrue($obj3->isInDB());

// now test the filtering based on Created timestamp
$list = DataObjectTest_ValidatedObject::get()
->filter(array('Created:GreaterThan' => '2013-02-01 00:00:00'))
->toArray();
$this->assertEquals(2, count($list));

}

public function testSubtract(){
$comment1 = $this->objFromFixture('DataObjectTest_TeamComment', 'comment1');
$subtractList = DataObjectTest_TeamComment::get()->filter('ID', $comment1->ID);
Expand Down
15 changes: 14 additions & 1 deletion tests/model/DataObjectTest.php
Expand Up @@ -18,7 +18,20 @@ class DataObjectTest extends SapphireTest {
'DataObjectTest_Player',
'DataObjectTest_TeamComment'
);


public function testValidObjectsForBaseFields() {
$obj = new DataObjectTest_ValidatedObject();

foreach (array('Created', 'LastEdited', 'ClassName', 'ID') as $field) {
$helper = $obj->dbObject($field);
$this->assertTrue(
($helper instanceof DBField),
"for {$field} expected helper to be DBField, but was " .
(is_object($helper) ? get_class($helper) : "null")
);
}
}

public function testDataIntegrityWhenTwoSubclassesHaveSameField() {
// Save data into DataObjectTest_SubTeam.SubclassDatabaseField
$obj = new DataObjectTest_SubTeam();
Expand Down

7 comments on commit 50e9eee

@ARNHOE
Copy link

@ARNHOE ARNHOE commented on 50e9eee Jul 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think since this commit, Created/LastEdited/ID/ClassName are showing up in my dataobjects in my modeladmin.

@hafriedlander
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this has broken VirtualPageTest too.

This shouldn't really have been merged into 3.1 this late in the cycle. It changes how DataObject#db functions, but there are lots of calls to that method that assume the old behaviour & break on the new one. I'm probably going to at least partially revert it for now.

@jthomerson
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hafriedlander give me a few minutes to work on this ... it all goes back to #2021. I still think that the commits for that issue and this one were the correct thing to do (made things work as their docs said they should), but since the way they were implemented is what's breaking things, I think at the very least we can make a safer change to DataObject->db (for #2021) but not make the changes in this commit, and still make all the test cases pass.

@hafriedlander
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, take a look. I'm looking too, since the CMS tests breaking is holding up a pretty critical fix.

I still think that the commits for that issue and this one were the correct thing to do (made things work as their docs said they should)

Possibly, although the docs are definitely not a canonical source for correct operation - they're often overlooked when changing methods. The issue is basically, we shouldn't be making API changes at this stage unless absolutely critical, and changing db like this is an API change.

PS you didn't used to be able to overwrite the Created date like you do in your test - it was forced as part of the write process. Not sure if that changed before & I just didn't notice, or is also an artefact of this patch.

@jthomerson
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hafriedlander see #2187 which reverts all the changes I made and fixes the original problem a different and much less obtrusive way. It leaves all the additional test cases I wrote, thought, since they are still valid.

PS you didn't used to be able to overwrite the Created date like you do in your test - it was forced as part of the write process. Not sure if that changed before & I just didn't notice, or is also an artefact of this patch.

If that's the case it must have changed well before my commits because the test case still passes even after reverting my other changes. Seems like maybe we should create an issue for it so somebody can dig into it later. In that case the test case will need to be modified to still work (maybe direct DB update query) so that we can easily still test the use case that caused me to write the test in the first place. But for now it's a useful test (testing querying by Created).

@simonwelsh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f2a709d is the commit that added that behaviour. Was designed so you could write unit tests that used Created.

@hafriedlander
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f2a709d is the commit that added that behaviour. Was designed so you could write unit tests that used Created.

Cool, mystery solved.

Please sign in to comment.