FIX Image in summaryfields breaks search

[sabina-talipova]

Description

Fields that pass validation on hasMethod() check, but do not have a corresponding field or relations in the DB are excluded from searchable_fields.

Parent issue

• Image in summary_fields breaks searching/linking DOs #10761

[GuySartorelli]

Haven't tested locally yet but there are some things in the implementation and tests that don't quite make sense to me

[GuySartorelli]

I think this approach isn't complete.

I have used the following class when testing locally. Note that the Company class mentioned is just some random DataObject with a Name field.

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 implementation

Correctly included:

• Direct fields on the class (e.g. Name)
• Direct fields on the class, even if we're calling Plain or Nice etc on that field (e.g. Biography.Plain)
• Direct fields on a has_one relation (e.g. Company.Category)
• Direct fields on a many_many or has_many relation (e.g. PastCompanies.Name)

Still included, but shouldn't be:

• Methods on the class (e.g. MethodTwo)

Not included, but should be:

• Direct fields on a relation if we're calling Plain or Nice etc on that field (e.g. Company.Name.Plain)

Not tested:

• Direct fields on chained relations (e.g. Company.Customers.Location)
• Direct fields on chained relations if we're calling Plain or Nice etc on that field (e.g. Company.Customers.Location.Plain)

Recommendation

Instead of hasRelations(), which is still allowing methods and isn't correctly allowing relation fields in some cases, have a method, similar to the below, which takes a dot-syntax field name (e.g. Name or Company.Customers.Location.Plain) and returns only the part that is backed by the database (e.g. Name or Company.Customers.Location).

Note that this implementation is based heavily on relObject(), but has been changed so that any time we come across something that cannot be backed by the database (e.g. a method call that isn't for getting a relation list), we return null - and otherwise we return the dot syntax field name for the database-backed field, which excludes method calls such as Plain.
This works correctly for chained relations.

    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 foreach ($summaryFields as $key => $name) { block:

- $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;
 }

[GuySartorelli]

This is shaping up nicely. Just a few small things to clean up now.

[GuySartorelli]

Looks good, works well locally, and provides a much nicer default behaviour than the previous behaviour.