Skip to content
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

Allow methods to be used for flexible searchable_fields #10199

Merged

Conversation

tiller1010
Copy link
Contributor

Summary

Allow methods to be used for flexible $searchable_fields. A method name can be added as a $searchable_fields key. The method should return an array of fields that can be used when using this search field in a GridFieldFilterHeader.

Testing

  • create a DataObject with $searchable_fields
  • set a key to a method name that returns the appropriate search fields in an array (see comments in SearchContext)
  • create records that use the searchable fields returned in your method
  • confirm the search results will show any records that match at least one of the returned search fields

Concerns

  • It was necessary for me to define the field type in the searchable field options.

@emteknetnz
Copy link
Member

Could you help me understand what this is supposed to do from a UI perspective? Based on

                        public function getSearchableFirstName($queryName = '')
                        {
                            return [
                                'Customer.FirstName' => $queryName,
                                'ShippingAddress.FirstName' => $queryName,
                            ];
                        }

I'm assuming that when a user searches on FirstName it would now search on both the Customer.FirstName database field as well as the ShippingAddress.FirstName field?

@tiller1010
Copy link
Contributor Author

@emteknetnz

I'm assuming that when a user searches on FirstName it would now search on both the Customer.FirstName database field as well as the ShippingAddress.FirstName field?

That's exactly right.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm finding the syntax of this confusing. I don't think there's a need for a separate method as we're limited to what's in the database, and we cannot use dynamic content in this context i.e. filtering a DataList (as far as I'm aware, maybe I'm wrong)

I think putting everything in $searchable_fields would make more sense

private static $searchable_fields = [
    'CustomFirstName' => [
        'title' => 'First Name',
        'field' => TextField::class,
        'filter' => 'PartialMatchFilter',
        'match_any' => [
            'Customer.FirstName,
            'ShippingAddress.FirstName'
        ]
    ]
]

To merge this PR we'll also need unit test coverage added to this PR, as well as documentation about this additional functionality updated here rather than as an code comment as it is now

@tiller1010 tiller1010 requested a review from emteknetnz February 7, 2022 18:35
Copy link
Contributor

@michalkleiner michalkleiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok from my perspective, thanks!

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work.

I've tested this locally to confirm it works as expected

Some very minor changes then I'm happy to merge

There's some PHPCS linting to fix as well https://app.travis-ci.com/github/silverstripe/silverstripe-framework/jobs/558865008

src/ORM/Search/SearchContext.php Outdated Show resolved Hide resolved
$relations = explode('.', $dottedRelation);
$fieldName = array_pop($relations);

// Apply join
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need these inline comments explaining the code or the line breaks above them

docs/en/02_Developer_Guides/00_Model/11_Scaffolding.md Outdated Show resolved Hide resolved
@tiller1010 tiller1010 requested a review from emteknetnz February 9, 2022 14:37
@emteknetnz emteknetnz merged commit 6b1c5eb into silverstripe:4 Feb 9, 2022
@tiller1010 tiller1010 deleted the method-based-searchable_fields branch February 11, 2022 14:05
GuySartorelli added a commit to GuySartorelli/silverstripe-framework that referenced this pull request Jul 6, 2022
…ripe#10199)

* Allow methods to be used for flexible searchable_fields

* match_any key

* Documentation

* Update docs/en/02_Developer_Guides/00_Model/11_Scaffolding.md

Co-authored-by: GuySartorelli <[email protected]>

* Search fields test

* Newlines

* Update src/ORM/Search/SearchContext.php

Co-authored-by: Steve Boyd <[email protected]>

* Update docs/en/02_Developer_Guides/00_Model/11_Scaffolding.md

Co-authored-by: Steve Boyd <[email protected]>

* Removed comments and whitespace. Linting fixes

Co-authored-by: GuySartorelli <[email protected]>
Co-authored-by: Steve Boyd <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants