From e2a68ccffec522178a2f56764a5f5bc082fb1252 Mon Sep 17 00:00:00 2001 From: Andrew Aitken-Fincham Date: Mon, 14 May 2018 17:04:40 +0100 Subject: [PATCH] improve SearchQuery API readability by deprecating functions deprecate filter, exclude, fuzzysearch, inClass deprecate start, limit deprecate page, update SearchQuery_Range add _config.php with Deprecation versioning update index.md method names update solr.md method names --- _config.php | 5 + docs/en/Solr.md | 24 +-- docs/en/index.md | 18 +- src/Search/Queries/SearchQuery.php | 187 ++++++++++++++++-- src/Search/Queries/SearchQuery_Range.php | 30 ++- src/Search/Variants/SearchVariant.php | 1 + src/Search/Variants/SearchVariantSubsites.php | 6 +- tests/SearchVariantSubsitesTest.php | 2 +- tests/SolrIndexTest.php | 8 +- tests/SolrReindexQueuedTest.php | 1 + .../SolrReindexTest_Variant.php | 36 +++- 11 files changed, 259 insertions(+), 59 deletions(-) create mode 100644 _config.php diff --git a/_config.php b/_config.php new file mode 100644 index 0000000..31684ea --- /dev/null +++ b/_config.php @@ -0,0 +1,5 @@ +search('My Term'); +$query->addSearchTerm('My Term'); $params = [ 'spellcheck' => 'true', 'spellcheck.collate' => 'true', ]; $results = $index->search($query, -1, -1, $params); -$results->spellcheck +$results->spellcheck; ``` The built-in `_text` data is better than nothing, but also has some problems: @@ -337,10 +337,8 @@ use SilverStripe\CMS\Model\SiteTree; use SilverStripe\FullTextSearch\Search\Queries\SearchQuery; $query = new SearchQuery(); -$query->classes = [ - ['class' => Page::class, 'includeSubclasses' => true], -]; -$query->search('someterms', [SiteTree::class . '_Title', SiteTree::class . '_Content']); +$query->addClassFilter(Page::class); +$query->addSearchTerm('someterms', [SiteTree::class . '_Title', SiteTree::class . '_Content']); $result = singleton(SolrSearchIndex::class)->search($query, -1, -1); // the request to Solr would be: @@ -366,10 +364,8 @@ use SilverStripe\CMS\Model\SiteTree; use SilverStripe\FullTextSearch\Search\Queries\SearchQuery; $query = new SearchQuery(); -$query->classes = [ - ['class' => 'Page', 'includeSubclasses' => true], -]; -$query->search('Lorem', null, [SiteTree::class . '_Content' => 2]); +$query->addClassFilter(Page::class); +$query->addSearchTerm('Lorem', null, [SiteTree::class . '_Content' => 2]); $result = singleton(SolrSearchIndex::class)->search($query, -1, -1); // the request to Solr would be: @@ -463,7 +459,7 @@ use SilverStripe\FullTextSearch\Search\Queries\SearchQuery; $index = new MyIndex(); $query = new SearchQuery(); -$query->search('My Term'); +$query->addSearchTerm('My Term'); $results = $index->search($query, -1, -1, ['hl' => 'true']); ``` @@ -486,7 +482,7 @@ use SilverStripe\FullTextSearch\Search\Queries\SearchQuery; $index = new MyIndex(); $query = new SearchQuery(); -$query->search('My Term'); +$query->addSearchTerm('My Term'); $results = $index->search($query, -1, -1, [ 'facet' => 'true', 'facet.field' => 'SiteTree_ClassName', @@ -698,8 +694,8 @@ In order to query the field, reverse the search conditions and exclude the range ```php // Wrong: Filter will ignore all empty field values -$myQuery->filter('fieldname', new SearchQuery_Range('*', 'somedate')); +$myQuery->addFilter('fieldname', new SearchQuery_Range('*', 'somedate')); // Better: Exclude the opposite range -$myQuery->exclude('fieldname', new SearchQuery_Range('somedate', '*')); +$myQuery->addExclude('fieldname', new SearchQuery_Range('somedate', '*')); ``` diff --git a/docs/en/index.md b/docs/en/index.md index f835083..fab1dfb 100644 --- a/docs/en/index.md +++ b/docs/en/index.md @@ -85,7 +85,7 @@ Note: There's usually a connector-specific "reindex" task for this. use SilverStripe\FullTextSearch\Search\Queries\SearchQuery; $query = new SearchQuery(); -$query->search('My house is on fire'); +$query->addSearchTerm('My house is on fire'); ``` 4). Apply that query to an index @@ -123,7 +123,7 @@ class PageController extends ContentController public function search(HTTPRequest $request) { $query = new SearchQuery(); - $query->search($request->getVar('q')); + $query->addSearchTerm($request->getVar('q')); return $this->renderWith([ 'SearchResult' => singleton(MyIndex::class)->search($query) ]); @@ -178,13 +178,13 @@ Manual updates are connector specific, please check the connector docs for detai ## Searching Specific Fields By default, the index searches through all indexed fields. -This can be limited by arguments to the `search()` call. +This can be limited by arguments to the `addSearchTerm()` call. ```php use SilverStripe\FullTextSearch\Search\Queries\SearchQuery; $query = new SearchQuery(); -$query->search('My house is on fire', [Page::class . '_Title']); +$query->addSearchTerm('My house is on fire', [Page::class . '_Title']); // No results, since we're searching in title rather than page content $results = singleton(MyIndex::class)->search($query); ``` @@ -201,9 +201,9 @@ use SilverStripe\FullTextSearch\Search\Queries\SearchQuery; use SilverStripe\FullTextSearch\Search\Queries\SearchQuery_Range; $query = new SearchQuery(); -$query->search('My house is on fire'); +$query->addSearchTerm('My house is on fire'); // Only include documents edited in 2011 or earlier -$query->filter(Page::class . '_LastEdited', new SearchQuery_Range(null, '2011-12-31T23:59:59Z')); +$query->addFilter(Page::class . '_LastEdited', new SearchQuery_Range(null, '2011-12-31T23:59:59Z')); $results = singleton(MyIndex::class)->search($query); ``` @@ -220,9 +220,9 @@ The `SearchQuery` API has the concept of a "missing" and "present" field value f use SilverStripe\FullTextSearch\Search\Queries\SearchQuery; $query = new SearchQuery(); -$query->search('My house is on fire'); +$query->addSearchTerm('My house is on fire'); // Needs a value, although it can be false -$query->filter(Page::class . '_ShowInMenus', SearchQuery::$present); +$query->addFilter(Page::class . '_ShowInMenus', SearchQuery::$present); $results = singleton(MyIndex::class)->search($query); ``` @@ -300,7 +300,7 @@ Example: use SilverStripe\FullTextSearch\Search\Queries\SearchQuery; $query = new SearchQuery(); -$query->search( +$query->addSearchTerm( 'My house is on fire', null, [ diff --git a/src/Search/Queries/SearchQuery.php b/src/Search/Queries/SearchQuery.php index a92b441..aae9f9b 100644 --- a/src/Search/Queries/SearchQuery.php +++ b/src/Search/Queries/SearchQuery.php @@ -2,6 +2,7 @@ namespace SilverStripe\FullTextSearch\Search\Queries; +use SilverStripe\Dev\Deprecation; use SilverStripe\View\ViewableData; use stdClass; @@ -48,7 +49,7 @@ class SearchQuery extends ViewableData * @param array $boost Map of composite field names to float values. The higher the value, * the more important the field gets for relevancy. */ - public function search($text, $fields = null, $boost = []) + public function addSearchTerm($text, $fields = null, $boost = []) { $this->search[] = [ 'text' => $text, @@ -56,18 +57,19 @@ class SearchQuery extends ViewableData 'boost' => $boost, 'fuzzy' => false ]; + return $this; } /** - * Similar to {@link search()}, but uses stemming and other similarity algorithms + * Similar to {@link addSearchTerm()}, but uses stemming and other similarity algorithms * to find the searched terms. For example, a term "fishing" would also likely find results * containing "fish" or "fisher". Depends on search implementation. * - * @param string $text See {@link search()} - * @param array $fields See {@link search()} - * @param array $boost See {@link search()} + * @param string $text See {@link addSearchTerm()} + * @param array $fields See {@link addSearchTerm()} + * @param array $boost See {@link addSearchTerm()} */ - public function fuzzysearch($text, $fields = null, $boost = []) + public function addFuzzySearchTerm($text, $fields = null, $boost = []) { $this->search[] = [ 'text' => $text, @@ -75,60 +77,131 @@ class SearchQuery extends ViewableData 'boost' => $boost, 'fuzzy' => true ]; + return $this; } - public function inClass($class, $includeSubclasses = true) + /** + * @return array + */ + public function getSearchTerms() + { + return $this->search; + } + + /** + * @param string $class + * @param bool $includeSubclasses + * @return $this + */ + public function addClassFilter($class, $includeSubclasses = true) { $this->classes[] = [ 'class' => $class, 'includeSubclasses' => $includeSubclasses ]; + return $this; } /** - * Similar to {@link search()}, but typically used to further narrow down + * @return array + */ + public function getClassFilters() + { + return $this->classes; + } + + /** + * Similar to {@link addSearchTerm()}, but typically used to further narrow down * based on other facets which don't influence the field relevancy. * * @param string $field Composite name of the field * @param mixed $values Scalar value, array of values, or an instance of SearchQuery_Range */ - public function filter($field, $values) + public function addFilter($field, $values) { $requires = isset($this->require[$field]) ? $this->require[$field] : []; $values = is_array($values) ? $values : [$values]; $this->require[$field] = array_merge($requires, $values); + return $this; } /** - * Excludes results which match these criteria, inverse of {@link filter()}. + * @return array + */ + public function getFilters() + { + return $this->require; + } + + /** + * Excludes results which match these criteria, inverse of {@link addFilter()}. * * @param string $field * @param mixed $values */ - public function exclude($field, $values) + public function addExclude($field, $values) { $excludes = isset($this->exclude[$field]) ? $this->exclude[$field] : []; $values = is_array($values) ? $values : [$values]; $this->exclude[$field] = array_merge($excludes, $values); + return $this; } - public function start($start) + /** + * @return array + */ + public function getExcludes() + { + return $this->exclude; + } + + public function setStart($start) { $this->start = $start; + return $this; } - public function limit($limit) + /** + * @return int + */ + public function getStart() + { + return $this->start; + } + + public function setLimit($limit) { $this->limit = $limit; + return $this; } - public function page($page) + /** + * @return int + */ + public function getLimit() { - $this->start = $page * self::$default_page_size; - $this->limit = self::$default_page_size; + return $this->limit; } - public function isfiltered() + public function setPageSize($page) + { + $this->setStart($page * self::$default_page_size); + $this->setLimit(self::$default_page_size); + return $this; + } + + /** + * @return int + */ + public function getPageSize() + { + return (int) ($this->getLimit() / $this->getStart()); + } + + /** + * @return bool + */ + public function isFiltered() { return $this->search || $this->classes || $this->require || $this->exclude; } @@ -137,4 +210,84 @@ class SearchQuery extends ViewableData { return "Search Query\n"; } + + /** + * @codeCoverageIgnore + * @deprecated + */ + public function search($text, $fields = null, $boost = []) + { + Deprecation::notice('4.0', 'Use addSearchTerm() instead'); + return $this->addSearchTerm($text, $fields, $boost); + } + + /** + * @codeCoverageIgnore + * @deprecated + */ + public function fuzzysearch($text, $fields = null, $boost = []) + { + Deprecation::notice('4.0', 'Use addFuzzySearchTerm() instead'); + return $this->addFuzzySearchTerm($text, $fields, $boost); + } + + /** + * @codeCoverageIgnore + * @deprecated + */ + public function inClass($class, $includeSubclasses = true) + { + Deprecation::notice('4.0', 'Use addClassFilter() instead'); + return $this->addClassFilter($class, $includeSubclasses); + } + + /** + * @codeCoverageIgnore + * @deprecated + */ + public function filter($field, $values) + { + Deprecation::notice('4.0', 'Use addFilter() instead'); + return $this->addFilter($field, $values); + } + + /** + * @codeCoverageIgnore + * @deprecated + */ + public function exclude($field, $values) + { + Deprecation::notice('4.0', 'Use addExclude() instead'); + return $this->addExclude($field, $values); + } + + /** + * @codeCoverageIgnore + * @deprecated + */ + public function start($start) + { + Deprecation::notice('4.0', 'Use setStart() instead'); + return $this->setStart($start); + } + + /** + * @codeCoverageIgnore + * @deprecated + */ + public function limit($limit) + { + Deprecation::notice('4.0', 'Use setLimit() instead'); + return $this->setLimit($limit); + } + + /** + * @codeCoverageIgnore + * @deprecated + */ + public function page($page) + { + Deprecation::notice('4.0', 'Use setPageSize() instead'); + return $this->setPageSize($page); + } } diff --git a/src/Search/Queries/SearchQuery_Range.php b/src/Search/Queries/SearchQuery_Range.php index 6d3b8c3..89d7804 100644 --- a/src/Search/Queries/SearchQuery_Range.php +++ b/src/Search/Queries/SearchQuery_Range.php @@ -2,6 +2,8 @@ namespace SilverStripe\FullTextSearch\Search\Queries; +use SilverStripe\Dev\Deprecation; + /** * Create one of these and pass as one of the values in filter or exclude to filter or exclude by a (possibly * open ended) range @@ -17,18 +19,40 @@ class SearchQuery_Range $this->end = $end; } - public function start($start) + public function setStart($start) { $this->start = $start; + return $this; } - public function end($end) + public function setEnd($end) { $this->end = $end; + return $this; } - public function isfiltered() + public function isFiltered() { return $this->start !== null || $this->end !== null; } + + /** + * @deprecated + * @codeCoverageIgnore + */ + public function start($start) + { + Deprecation::notice('4.0', 'Use setStart() instead'); + return $this->setStart($start); + } + + /** + * @deprecated + * @codeCoverageIgnore + */ + public function end($end) + { + Deprecation::notice('4.0', 'Use setEnd() instead'); + return $this->setEnd($end); + } } diff --git a/src/Search/Variants/SearchVariant.php b/src/Search/Variants/SearchVariant.php index ea70abf..7a6c9b1 100644 --- a/src/Search/Variants/SearchVariant.php +++ b/src/Search/Variants/SearchVariant.php @@ -5,6 +5,7 @@ namespace SilverStripe\FullTextSearch\Search\Variants; use ReflectionClass; use SilverStripe\Core\ClassInfo; use SilverStripe\Core\Config\Configurable; +use SilverStripe\FullTextSearch\Search\Indexes\SearchIndex; use SilverStripe\FullTextSearch\Utils\CombinationsArrayIterator; /** diff --git a/src/Search/Variants/SearchVariantSubsites.php b/src/Search/Variants/SearchVariantSubsites.php index dc55db3..96ca6a2 100644 --- a/src/Search/Variants/SearchVariantSubsites.php +++ b/src/Search/Variants/SearchVariantSubsites.php @@ -4,6 +4,7 @@ namespace SilverStripe\FullTextSearch\Search\Variants; use SilverStripe\Assets\File; use SilverStripe\CMS\Model\SiteTree; +use SilverStripe\FullTextSearch\Search\Indexes\SearchIndex; use SilverStripe\FullTextSearch\Search\SearchIntrospection; use SilverStripe\FullTextSearch\Search\Queries\SearchQuery; use SilverStripe\ORM\Queries\SQLSelect; @@ -100,6 +101,9 @@ class SearchVariantSubsites extends SearchVariant * * A pull request has been raised for this issue. Once accepted this forked module can be deleted and the parent * project should be used instead. + * + * @param SearchQuery $query + * @param SearchIndex $index */ public function alterQuery($query, $index) { @@ -108,7 +112,7 @@ class SearchVariantSubsites extends SearchVariant } $subsite = $this->currentState(); - $query->filter('_subsite', [$subsite, SearchQuery::$missing]); + $query->addFilter('_subsite', [$subsite, SearchQuery::$missing]); } /** diff --git a/tests/SearchVariantSubsitesTest.php b/tests/SearchVariantSubsitesTest.php index 1e43105..3238f13 100644 --- a/tests/SearchVariantSubsitesTest.php +++ b/tests/SearchVariantSubsitesTest.php @@ -72,7 +72,7 @@ class SearchVariantSubsiteTest extends SapphireTest //apply the subsite filter on the query (for example, if it's passed into a controller and set before searching) //we've chosen an arbirary value of 2 here, to check if it is changed later - $query->filter('_subsite', 2); + $query->addFilter('_subsite', 2); $this->assertNotEmpty($query->require['_subsite']); //apply the search variant's definition and query diff --git a/tests/SolrIndexTest.php b/tests/SolrIndexTest.php index c2ae1fc..00d4121 100644 --- a/tests/SolrIndexTest.php +++ b/tests/SolrIndexTest.php @@ -141,7 +141,7 @@ class SolrIndexTest extends SapphireTest $index->setService($serviceMock); $query = new SearchQuery(); - $query->search( + $query->addSearchTerm( 'term', null, array('Field1' => 1.5, 'HasOneObject_Field1' => 3) @@ -181,7 +181,7 @@ class SolrIndexTest extends SapphireTest $index->setService($serviceMock); $query = new SearchQuery(); - $query->search('term'); + $query->addSearchTerm('term'); $index->search($query); } @@ -218,7 +218,7 @@ class SolrIndexTest extends SapphireTest // Search without highlighting $query = new SearchQuery(); - $query->search( + $query->addSearchTerm( 'term', null, array('Field1' => 1.5, 'HasOneObject_Field1' => 3) @@ -227,7 +227,7 @@ class SolrIndexTest extends SapphireTest // Search with highlighting $query = new SearchQuery(); - $query->search( + $query->addSearchTerm( 'term', null, array('Field1' => 1.5, 'HasOneObject_Field1' => 3) diff --git a/tests/SolrReindexQueuedTest.php b/tests/SolrReindexQueuedTest.php index d269007..84e1c42 100644 --- a/tests/SolrReindexQueuedTest.php +++ b/tests/SolrReindexQueuedTest.php @@ -5,6 +5,7 @@ namespace SilverStripe\FullTextSearch\Tests; use SilverStripe\Core\Config\Config; use SilverStripe\Core\Injector\Injector; use SilverStripe\Dev\SapphireTest; +use SilverStripe\FullTextSearch\Solr\Services\SolrService; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DB; use SilverStripe\FullTextSearch\Search\FullTextSearch; diff --git a/tests/SolrReindexTest/SolrReindexTest_Variant.php b/tests/SolrReindexTest/SolrReindexTest_Variant.php index 94cef6d..333cbd4 100644 --- a/tests/SolrReindexTest/SolrReindexTest_Variant.php +++ b/tests/SolrReindexTest/SolrReindexTest_Variant.php @@ -3,11 +3,13 @@ namespace SilverStripe\FullTextSearch\Tests\SolrReindexTest; use SilverStripe\Dev\TestOnly; +use SilverStripe\FullTextSearch\Search\Indexes\SearchIndex; +use SilverStripe\FullTextSearch\Search\Queries\SearchQuery; use SilverStripe\FullTextSearch\Search\Variants\SearchVariant; use SilverStripe\ORM\DataObject; /** - * Dummy variant that selects items with field Varient matching the current value + * Dummy variant that selects items with field Variant matching the current value * * Variant states are 0 and 1, or null if disabled */ @@ -28,9 +30,9 @@ class SolrReindexTest_Variant extends SearchVariant implements TestOnly self::disable(); self::$current = 0; - self::$variants = array( + self::$variants = [ self::class => singleton(self::class) - ); + ]; } /** @@ -40,8 +42,8 @@ class SolrReindexTest_Variant extends SearchVariant implements TestOnly { self::$current = null; self::$variants = null; - self::$class_variants = array(); - self::$call_instances = array(); + self::$class_variants = []; + self::$call_instances = []; } public function activateState($state) @@ -68,29 +70,43 @@ class SolrReindexTest_Variant extends SearchVariant implements TestOnly { // Always use string values for states for consistent json_encode value if (isset(self::$current)) { - return (string)self::$current; + return (string) self::$current; } } + /** + * @param string $class + * @param SearchIndex $index + */ public function alterDefinition($class, $index) { $self = get_class($this); - $this->addFilterField($index, '_testvariant', array( + $this->addFilterField($index, '_testvariant', [ 'name' => '_testvariant', 'field' => '_testvariant', 'fullfield' => '_testvariant', 'base' => DataObject::getSchema()->baseDataClass($class), 'origin' => $class, 'type' => 'Int', - 'lookup_chain' => array(array('call' => 'variant', 'variant' => $self, 'method' => 'currentState')) - )); + 'lookup_chain' => [ + [ + 'call' => 'variant', + 'variant' => $self, + 'method' => 'currentState' + ] + ] + ]); } + /** + * @param SearchQuery $query + * @param SearchIndex $index + */ public function alterQuery($query, $index) { // I guess just calling it _testvariant is ok? - $query->filter('_testvariant', $this->currentState()); + $query->addFilter('_testvariant', $this->currentState()); } public function appliesTo($class, $includeSubclasses)