-
Notifications
You must be signed in to change notification settings - Fork 824
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 Respect searchable_fields #10733
FIX Respect searchable_fields #10733
Conversation
$searchableFields = Config::inst()->get($modelClass, 'searchable_fields'); | ||
if (!empty($searchableFields)) { |
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.
Do we want to make sure here that searchable_fields
is an array if set?
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.
Have changed to use $modelClass::singleton()->searchableFields() which returns an array
if ($title && $list->canFilterBy($columnField)) { | ||
return true; | ||
$modelClass = $gridField->getModelClass(); | ||
$searchableFields = Config::inst()->get($modelClass, 'searchable_fields'); |
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.
Searchable fields should be fetched by calling searchableFields()
on a singleton, i.e. $modelClass::singleton()->searchableFields()
See SearchContext and GridFieldAddExistingAutocompleter as examples of where this is already done.
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.
Updated
foreach ($searchableFields as $searchableField) { | ||
if ($list->canFilterBy($searchableField)) { | ||
return true; | ||
} | ||
} |
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.
canFilterBy()
is fine for summary_fields()
- but we can declare searchable_fields
that aren't traditionally filterable here, and then tell SearchContext
how to search using those fields. If a field is in searchable fields that isn't in summary fields, we should just assume it's filterable and let SearchContext
sort it out.
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.
Rewrote the method
66a2bb5
to
0f40cc3
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.
Works as expected
Issue #10683
Installer run - https://github.com/emteknetnz/silverstripe-installer/actions/runs/4485413512