NEW: Allow different search filters on TreeDropdownField

[elliot-sawyer]

This pull request comes about when encountering performance issues on the tree method when searching on a large dataset.

I have a site with >20K SiteTree records. The client love the Insert Link feature on the WYSIWYG, and starts enthusiastically searching for Titles containing "tra". The offending link can be reproduced after logging into the CMS and visiting admin/Modals/editorInternalLink/field/PageID/tree?search=tra&flatList=1&format=json. Unfortunately, this kicks off some really inefficient queries to the backend, exacerbated by the following issues:

1. Lots of records, not much we can do about that
2. We're searching on Title and MenuTitle by default, which is not indexed, because...
3. It's using PartialMatch, so an index on those columns would be ineffective

To address this, I propose replacing the hardcoded PartialMatch filter with a configuration variable, to allow developers to modify the search behaviour. In my case, I added indexes on SiteTree for MenuTitle and Title. After changing the filter to StartsWith the query responds in ~6s vs the 30+ (usually an execution_limit error) it was taking previously, albeit at the expense of fewer results.

[GuySartorelli]

Thank you, this is great!

Could you please write a test for this to avoid us breaking it in the future?

It also seems like a good candidate for some documentation - other developers might not intuitively think to try this config setting in combination with indexes if they experience a similar problem.

[michalkleiner]

Nice! I'd probably name the config just private static $search_filter = 'PartialMatch'; as it's not configuring a default that can later be changed, it's the filter itself.

[elliot-sawyer]

Yeah was just thinking about tests, will see how best to do that

[elliot-sawyer]

I'm having a hard time running tests for Silverstripe 5, could either of you share a docker-compose.yml file I could use?

Last error I got was PHP Fatal error: Declaration of SilverStripe\Core\Injector\Injector::has($name) must be compatible with Psr\Container\ContainerInterface::has(string $id): bool in /app/vendor/silverstripe/framework/src/Core/Injector/Injector.php on line 880 with the command vendor/bin/phpunit vendor/silverstripe/framework/tests/php/Forms/TreeDropdownFieldTest.php

My composer.json:

{
    "name": "silverstripe/installer",
    "type": "silverstripe-recipe",
    "description": "The SilverStripe Framework Installer",
    "require": {
        "php": "^7.4 || ^8.0",
        "silverstripe/framework": "dev-feature/change-search-filter-treedropdownfield as 5.x-dev",
        "silverstripe/assets" : "2.x-dev"
    },
    "require-dev": {
        "phpunit/phpunit": "^9.5"
    },
    "repositories": [
        {
            "name": "silverstripe/framework",
            "type": "vcs",
            "url": "git@github.com:elliot-sawyer/silverstripe-framework.git"
        }
    ],
    "extra": {
        "resources-dir": "_resources",
        "project-files-installed": [
            "app/.htaccess",
            "app/_config.php",
            "app/_config/mimevalidator.yml",
            "app/_config/mysite.yml",
            "app/src/Page.php",
            "app/src/PageController.php"
        ],
        "public-files-installed": [
            ".htaccess",
            "index.php",
            "web.config"
        ]
    },
    "config": {
        "process-timeout": 600,
        "allow-plugins": {
            "composer/installers": true,
            "silverstripe/recipe-plugin": true,
            "silverstripe/vendor-plugin": true
        }
    },
    "prefer-stable": true,
    "minimum-stability": "dev"
}

[GuySartorelli]

That might just be a result of us not merging 5.0 up to 5 yet - try rebasing off 5.0 (for testing purposes - keep this pr targeting 5 for now) and setting your assets dependency to 2.0.x-dev

[elliot-sawyer]

I got it running somehow but still incredibly hard to write tests for the TreeDropdownField. Couldn't say specifically what I did, but it's something like this:

1. Remove errant `app/src/Page* since I'm just testing framework
2. use as 5.0.0-beta3 instead of as 5.x-dev
3. rebased my branch against the 5.0.0-beta3 SHA instead of tip of 5.x
4. removed every test except my new one (because testSchema would fail)

Now stuck with the following error:

1) SilverStripe\Forms\Tests\TreeDropdownFieldTest::testSearchFilter
SilverStripe\ORM\Connect\DatabaseException: Couldn't run query:

INSERT INTO "SilverStripe_ORM_Tests_HierarchyTest_HierarchyOnSubclassTestObject"
 ("ClassName", "LastEdited", "Created")
 VALUES
 (?, ?, ?)

Identifier name 'SilverStripe_ORM_Tests_HierarchyTest_HierarchyOnSubclassTestObject' is too long

That error is a bit strange, since table_name is defined on that object

[elliot-sawyer]

SilverStripe_ORM_Tests_HierarchyTest_HierarchyOnSubclassTestObject is 66 characters vs the 64-character length limit for MySQL table names. So that's the reason for the failure, but why is it ignoring $table_name?

[GuySartorelli]

You may find it easiest to just do a new composer create-project --prefer-source with installer and then checkout your pr into the framework module

[elliot-sawyer]

@GuySartorelli @michalkleiner I have added a test. It counts TestObject records from the fixture before applying the filter, and after

[GuySartorelli]

One small change to make.

Also I've written up some documentation for this change - can you both please review it to make sure I understood this correctly?
silverstripe/developer-docs#208

[GuySartorelli]

@elliot-sawyer @michalkleiner can one or both of you please take a looks at my docs PR as per #10735 (review)? I just want to make sure I've understood these changes correctly before I click merge.

[elliot-sawyer]

Apologies, I had thumbs-up'ed it to indicate approval. I fixed the test too