Silverstripe-framework: BadMethodCallException when scafolding search

[maxime-rainville]

Affected Version

SilverStripe 4.3

Description

Originally reported by @purplespider on SilverStripe forum

[Emergency] Uncaught BadMethodCallException: Object->__call(): the method 'scaffoldSearchField' does not exist on 'PurpleSpider\MSG\SalesCategory'
GET /admin/pages/edit/show/44

Line 54 in /vendor/silverstripe/framework/src/Core/CustomMethods.php
Source

45      * @throws BadMethodCallException
46      */
47     public function __call($method, $arguments)
48     {
49         // If the method cache was cleared by an an Object::add_extension() / Object::remove_extension()
50         // call, then we should rebuild it.
51         $class = static::class;
52         $config = $this->getExtraMethodConfig($method);
53         if (empty($config)) {
54             throw new BadMethodCallException(
55                 "Object->__call(): the method '$method' does not exist on '$class'"
56             );
57         }
58 
59         switch (true) {
60             case isset($config['callback']): {

Trace

    SilverStripe\View\ViewableData->__call(scaffoldSearchField, Array)
    DataObject.php:2234
    SilverStripe\ORM\DataObject->scaffoldSearchFields()
    DataObject.php:2175
    SilverStripe\ORM\DataObject->getDefaultSearchContext()
    GridFieldFilterHeader.php:249
    SilverStripe\Forms\GridField\GridFieldFilterHeader->getSearchContext(SilverStripe\Forms\GridField\GridField)
    GridFieldFilterHeader.php:269
    SilverStripe\Forms\GridField\GridFieldFilterHeader->getSearchFieldSchema(SilverStripe\Forms\GridField\GridField)
    GridFieldFilterHeader.php:491
    SilverStripe\Forms\GridField\GridFieldFilterHeader->getHTMLFragments(SilverStripe\Forms\GridField\GridField)
    GridField.php:434
    SilverStripe\Forms\GridField\GridField->FieldHolder()
    call_user_func_array(Array, Array)
    ViewableData.php:485
    SilverStripe\View\ViewableData->obj(FieldHolder, , 1)
    ViewableData.php:547
    SilverStripe\View\ViewableData->XML_val(FieldHolder, , 1)
    SSViewer_Scope.php:323

[chillu]

@maxime-rainville What's your take on this? I thought we pretty explicitly tried to make the search changes backwards compatible?

[maxime-rainville]

I had a look and managed to replicate the issue. I think the problem is that GridFieldFilterHeader wasn't looking the DataObject search context when building it's search form. So if the search context contained a field that wasn't searchable, you wouldn't get an error. This worked for only for regular gridifield. You would still get an error when the gridifield was served through a ModelAdmin.

The new search always look up the Search context. So if you have a field that isn't searchable in ytour context, you'll always get the error. Shouldn't be a difficult fix. Just adding a condition to skip fields that aren't searchable.

[purplespider]

From @maxime-rainville's comments I've been able to narrow this down a little further:

It seems to only occur if you've defined $summary_fields and included a field such as a DO has_one (making use of forTemplate()), e.g.:

private static $summary_fields = [ 'Title' => 'Item Title', 'Description' => 'Description', 'Category' => 'Category', // This is a DataObject ];

Removing the $summary_fields definition stops the error from occurring, as presumably in the absence of a separate defined set of $searchable_fields it's trying to use the summary fields to scaffold the search.

So a sensible workaround is to manually specify $searchable_fields and not include non-searchable fields (such as DO's) in the new $searchable_fields array, e.g. add:

private static $searchable_fields = [ 'Title', 'Description', ];

Or even better, define the DO field to search:
private static $searchable_fields = [ 'Title', 'Description', 'Category.Title', ];

Obviously it would still be better if these changes weren't required for it to work with 4.3.

[ScopeyNZ]

Obviously it would still be better if these changes weren't required for it to work with 4.3

I agree that it should work with configuration from 4.2. But if the configuration is invalid I think it also makes sense to throw a notice.

[purplespider]

Agreed, but I didn’t realise defining $searchable_fields whenever you define $summary_fields was a strict requirement?

[maxime-rainville]

This PR is kind of related. https://github.com/silverstripe/silverstripe-framework/pull/8793

I think you'll get a slightly better error message if the PR is merge.

[DorsetDigital]

Agreed, but I didn’t realise defining $searchable_fields whenever you define $summary_fields was a strict requirement?

Just as a bit of a 'me too' post.. I've just bumped up against this same problem.
The issue was that I'd done the same thing as the example here, defined one of the summary_fields to be a related data object, rather than one of the fields on that data object:

eg:

   private static $summary_fields = [
        'JobReference' => 'JobReference',
        'Title' => 'Title',
        'Location' => 'Location'
    ];

Fixing the declaration to this cures the issue:

    private static $summary_fields = [
        'JobReference' => 'JobReference',
        'Title' => 'Title',
        'Location.Title' => 'Location'
    ];

I'm happy to accept that this is my fault for passing an object into something that should (in my mind at least) be expecting a field. If it accepted the data object, I'm not sure what the expected behaviour in the gridfield columns would be - show every field on the related object?

If there was a way to make the error message more meaningful, that would be lovely.

[purplespider]

@DorsetDigital That's a very good point. Fixing the $summary_fields by specifying the DO Field is the correct solution here. Not adding $searchable_fields. 🤦‍♂️

This issue can probably be closed now, @maxime-rainville. Especially if PR #8793 improves the error message.

[wernerkrauss]

DataObject already has a method getTitle() which tries to return a Title or a Name field and falls back to the ID, see API Docs

Wouldn't it be more developer friendly that if you accidently pass a DataObject, it tries to utilize the "getTitle" method if available and only throw an error if it isn't available?

[maxime-rainville]

Better errors messages are good, but I don't it's unreasonable for developers to put non-searchable fields in their DataObject summary fields without the search starting to yell exceptions at them.

I would rather strip the summary_fields non-searchable entries from the search context. Or maybe we could add a sensible default scaffoldSearchField to DataObject ... something that just gets a list of all instance of the DataObject and spits them in a drop down.

[DorsetDigital]

Just as a follow up to this, am I right in saying that with these changes it's no longer going to be possible to use a custom method in the class to return data for the summary fields?

It used to be that you could set up a method on the DataObject:

public function shoutyLabel() {
  return strtoupper($this->Title);
}

and then reference it with:

private static $summary_fields = [
 'shoutyLabel' => 'Title',
 'AnotherProperty' => 'Summary'
];

This now returns the exception that scaffoldSearchField does not exist on string

[kinglozzer]

It should be possible to define a “custom” summary field without also defining $searchable_fields, if that ability has been lost then it’s definitely a regression

[sunnysideup]

The problem occurs if you dont specify searchable fields, then it takes summary fields. Summary fields can have all sorts of things (e.g. DO - as per above, ->Nice, etc...). Therefore searchable fields should not rely on summary fields.

Possible solutions, ... default searchable fields could be:

• the intersection of summary and db fields to be the default searchable fields.
• all the db fields that are numbers / text.
• all the text based fields
• the Name / Title field only