Skip to content

Commit

Permalink
FIX Image in summaryfields breaks search
Browse files Browse the repository at this point in the history
  • Loading branch information
Sabina Talipova committed Aug 2, 2023
1 parent 8c3ba81 commit 75f092b
Show file tree
Hide file tree
Showing 7 changed files with 130 additions and 22 deletions.
6 changes: 0 additions & 6 deletions src/Forms/GridField/GridFieldFilterHeader.php
Original file line number Diff line number Diff line change
Expand Up @@ -265,13 +265,7 @@ public function canFilterAnyColumns($gridField)
) {
// note: searchableFields() will return summary_fields if there are no searchable_fields on the model
$searchableFields = array_keys($singleton->searchableFields());
$summaryFields = array_keys($singleton->summaryFields());
sort($searchableFields);
sort($summaryFields);
// searchable_fields has been explictily defined i.e. searchableFields() is not falling back to summary_fields
if ($searchableFields !== $summaryFields) {
return true;
}
// we have fallen back to summary_fields, check they are filterable
foreach ($searchableFields as $searchableField) {
if ($list->canFilterBy($searchableField)) {
Expand Down
62 changes: 48 additions & 14 deletions src/ORM/DataObject.php
Original file line number Diff line number Diff line change
Expand Up @@ -3724,6 +3724,51 @@ public function onAfterBuild()
$this->extend('onAfterBuild');
}

private function getDatabaseBackedField(string $fieldPath): ?string
{
$component = $this;
$fieldParts = [];

foreach ($parts = explode('.', $fieldPath ?? '') as $nextPart) {
if (!$component) {
return null;
}
$fieldParts[] = $nextPart;

if ($component instanceof Relation || $component instanceof DataList) {
if ($component->hasMethod($nextPart)) {
// If the next part is a method, we don't have a database-backed field.
return null;
}
// The next part could either be a field, or another relation
$singleton = DataObject::singleton($component->dataClass());
if ($singleton->dbObject($nextPart)) {
// If the next part is a DBField, we've found the database-backed field.
break;
}
$component = $component->relation($nextPart);
array_shift($parts);
} elseif ($component instanceof DataObject && ($component->dbObject($nextPart) instanceof DBField)) {
// If the next part is a DBField, we've found the database-backed field.
break;
} elseif ($component instanceof DataObject && $component->getRelationType($nextPart) !== null) {
// If it's a last part or only one elemnt of a relation, we don't have a database-backed field.
if (count($parts) === 1) {
return null;
}
$component = $component->$nextPart();
array_shift($parts);
} elseif (ClassInfo::hasMethod($component, $nextPart)) {
// If the next part is a method, we don't have a database-backed field.
return null;
} else {
return null;
}
}

return implode('.', $fieldParts) ?: null;
}

/**
* Get the default searchable fields for this object, as defined in the
* $searchable_fields list. If searchable fields are not defined on the
Expand All @@ -3742,21 +3787,10 @@ public function searchableFields()
$summaryFields = array_keys($this->summaryFields() ?? []);
$fields = [];

// remove the custom getters as the search should not include them
$schema = static::getSchema();
if ($summaryFields) {
foreach ($summaryFields as $key => $name) {
$spec = $name;

// Extract field name in case this is a method called on a field (e.g. "Date.Nice")
if (($fieldPos = strpos($name ?? '', '.')) !== false) {
$name = substr($name ?? '', 0, $fieldPos);
}

if ($schema->fieldSpec($this, $name)) {
$fields[] = $name;
} elseif ($this->relObject($spec)) {
$fields[] = $spec;
foreach ($summaryFields as $name) {
if ($field = $this->getDatabaseBackedField($name)) {
$fields[] = $field;
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/php/Forms/GridField/GridFieldFilterHeaderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ public function testCanFilterAnyColumns()
// test that you can filterBy if searchable_fields even if it is not a legit field
// this is because we're making a blind assumption it will be filterable later in a SearchContext
Config::modify()->set(Team::class, 'searchable_fields', ['WhatIsThis']);
$this->assertTrue($filterHeader->canFilterAnyColumns($gridField));
$this->assertFalse($filterHeader->canFilterAnyColumns($gridField));

// test that you cannot filter by non-db field when it falls back to summary_fields
Config::modify()->remove(Team::class, 'searchable_fields');
Expand Down
43 changes: 43 additions & 0 deletions tests/php/ORM/DataObjectTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use SilverStripe\ORM\ValidationException;
use SilverStripe\Security\Member;
use SilverStripe\View\ViewableData;
use ReflectionMethod;
use stdClass;

class DataObjectTest extends SapphireTest
Expand Down Expand Up @@ -2671,4 +2672,46 @@ public function testDBObjectEnum()
$vals = ['25.25', '50.00', '75.00', '100.50'];
$this->assertSame(array_combine($vals ?? [], $vals ?? []), $obj->dbObject('MyEnumWithDots')->enumValues());
}

public function provideTestGetDatabaseBackedField()
{
return [
['Captain.IsRetired', 'Captain.IsRetired'],
['Captain.ShirtNumber', 'Captain.ShirtNumber'],
['Captain.FavouriteTeam', null],
['Captain.FavouriteTeam.Fans', null],
['Captain.FavouriteTeam.Fans.Count', null],
['Captain.FavouriteTeam.Title', 'Captain.FavouriteTeam.Title'],
['Captain.FavouriteTeam.Title.Plain', 'Captain.FavouriteTeam.Title'],
['Captain.FavouriteTeam.ReturnsNull', null],
['Captain.FavouriteTeam.MethodDoesNotExist', null],
['Captain.ReturnsNull', null],
['Founder.FavouriteTeam.Captain.ShirtNumber', 'Founder.FavouriteTeam.Captain.ShirtNumber'],
['Founder.FavouriteTeam.Captain.Fans', null],
['Founder.FavouriteTeam.Captain.Fans.Name.Plain', 'Founder.FavouriteTeam.Captain.Fans.Name'],
['Founder.FavouriteTeam.Captain.ReturnsNull', null],
['HasOneRelationship.FavouriteTeam.MyTitle', null],
['SubTeams.Comments.Name.Plain', 'SubTeams.Comments.Name'],
['Title', 'Title'],
['Title.Plain', 'Title'],
['DatabaseField', 'DatabaseField'],
['DatabaseField.MethodDoesNotExist', 'DatabaseField'],
['ReturnsNull', null],
['DynamicField', null],
];
}

/**
* @dataProvider provideTestGetDatabaseBackedField
*/
public function testGetDatabaseBackedField(string $fieldPath, $expected)
{
$dataObjectClass = new DataObject();
$method = new ReflectionMethod($dataObjectClass, 'getDatabaseBackedField');
$method->setAccessible(true);
$class = new Team([]);

$databaseBackedField = $method->invokeArgs($class, [$fieldPath]);
$this->assertSame($expected, $databaseBackedField);
}
}
5 changes: 5 additions & 0 deletions tests/php/ORM/DataObjectTest/Player.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,9 @@ class Player extends Member implements TestOnly
'IsRetired',
'ShirtNumber'
];

public function ReturnsNull()
{
return null;
}
}
11 changes: 10 additions & 1 deletion tests/php/ORM/Search/SearchContextTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,22 @@ public function testSearchableFieldsDefaultsToSummaryFields()
$obj = new SearchContextTest\NoSearchableFields();
$summaryFields = $obj->summaryFields();
$expected = [];
foreach ($summaryFields as $field => $label) {

$expectedSearchableFields = [
'Name',
'Customer.FirstName',
'HairColor',
'EyeColor',
];

foreach ($expectedSearchableFields as $field) {
$expected[$field] = [
'title' => $obj->fieldLabel($field),
'filter' => 'PartialMatchFilter',
];
}
$this->assertEquals($expected, $obj->searchableFields());
$this->assertNotEquals($summaryFields, $obj->searchableFields());
}

public function testSummaryIncludesDefaultFieldsIfNotDefined()
Expand Down
23 changes: 23 additions & 0 deletions tests/php/ORM/Search/SearchContextTest/NoSearchableFields.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use SilverStripe\Dev\TestOnly;
use SilverStripe\ORM\DataObject;
use SilverStripe\Assets\Image;

class NoSearchableFields extends DataObject implements TestOnly
{
Expand All @@ -18,12 +19,34 @@ class NoSearchableFields extends DataObject implements TestOnly

private static $has_one = [
'Customer' => Customer::class,
'Image' => Image::class,
];

private static $summary_fields = [
'Name' => 'Custom Label',
'Customer' => 'Customer',
'Customer.FirstName' => 'Customer',
'Image.CMSThumbnail' => 'Image',
'Image.BackLinks' => 'Backlinks',
'Image.BackLinks.Count' => 'Backlinks',
'HairColor',
'EyeColor',
'ReturnsNull',
'DynamicField'
];

public function MyName()
{
return 'Class ' . $this->Name;
}

public function getDynamicField()
{
return 'dynamicfield';
}

public function ReturnsNull()
{
return null;
}
}

0 comments on commit 75f092b

Please sign in to comment.