-
Notifications
You must be signed in to change notification settings - Fork 822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX Image in summaryfields breaks search #10885
FIX Image in summaryfields breaks search #10885
Conversation
e466b85
to
c65b5d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tested locally yet but there are some things in the implementation and tests that don't quite make sense to me
c65b5d5
to
df0981e
Compare
df0981e
to
0c5290a
Compare
I think this approach isn't complete. I have used the following class when testing locally. Note that the use SilverStripe\Assets\Image;
use SilverStripe\ORM\DataObject;
use SilverStripe\ORM\FieldType\DBField;
class SearchableFieldsTest extends DataObject
{
private static $db = [
'Name' => 'Varchar',
'Biography' => 'HTMLText',
];
private static $has_one = [
'Company' => Company::class,
'ProfileImage' => Image::class
];
private static $many_many = [
'PastCompanies' => Company::class
];
private static $summary_fields = [
'ID',
'Name',
'Biography.Plain',
'CompanyID',
'Company.Category',
'Company.Name.Plain',
'PastCompanies.Name',
'PastCompanies.Count', // shouldn't be searchable
'ProfileImage.CMSThumbnail', // shouldn't be searchable
'MethodOne', // shouldn't be searchable
'MethodTwo', // shouldn't be searchable
'Company', // shouldn't be searchable
];
public function getMethodOne()
{
return implode(' ', $this->Company()->Employees()->column('Name'));
}
public function MethodTwo()
{
return DBField::create_field('Varchar', 'this is the value');
}
} Problems with the current implementationCorrectly included:
Still included, but shouldn't be:
Not included, but should be:
Not tested:
RecommendationInstead of Note that this implementation is based heavily on private function getDatabaseBackedField(string $fieldPath): ?string
{
$component = $this;
$fieldParts = [];
foreach (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);
} 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) {
$component = $component->$nextPart();
} 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;
} Then, use this instead of everything in the - $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) && $this->hasRelations($spec)) {
- $fields[] = $spec;
+ if ($field = $this->getDatabaseBackedField($name)) {
+ $fields[] = $field;
} |
1188c80
to
a6fbcfa
Compare
75f092b
to
85421a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is shaping up nicely. Just a few small things to clean up now.
85421a5
to
d24095a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, works well locally, and provides a much nicer default behaviour than the previous behaviour.
Description
Fields that pass validation on
hasMethod()
check, but do not have a corresponding field or relations in the DB are excluded fromsearchable_fields
.Parent issue