Skip to content

Commit

Permalink
API Add strong-typing to SearchContext::getQuery() $limit param
Browse files Browse the repository at this point in the history
Seems the two search context implementations had different ideas about
what was correct for the limit parameter, and the basic one wasn't even
handling the values correctly.
  • Loading branch information
GuySartorelli committed Jan 7, 2025
1 parent 3871869 commit a83de6e
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 24 deletions.
23 changes: 11 additions & 12 deletions src/ORM/Search/BasicSearchContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,24 +33,16 @@ class BasicSearchContext extends SearchContext
* If a filter is applied to a relationship in dot notation,
* the parameter name should have the dots replaced with double underscores,
* for example "Comments__Name" instead of the filter name "Comments.Name".
* @param array|bool|string $sort Field to sort on.
* @param array|null|string $limit
* @param array|false|string $sort Field to sort on.
* @param int|array|null $limit
* @param SS_List $existingQuery
*/
public function getQuery($searchParams, $sort = false, $limit = false, $existingQuery = null): SS_List
public function getQuery($searchParams, $sort = false, int|array|null $limit = null, $existingQuery = null): SS_List
{
if (!$existingQuery || !is_a($existingQuery, SS_List::class)) {
throw new InvalidArgumentException('getQuery requires a pre-existing SS_List list to be passed as $existingQuery.');
}

if ((count(func_get_args()) >= 3) && (!in_array(gettype($limit), ['array', 'NULL', 'string']))) {
Deprecation::notice(
'5.1.0',
'$limit should be type of array|string|null'
);
$limit = null;
}

$searchParams = $this->applySearchFilters($this->normaliseSearchParams($searchParams));
$result = $this->applyGeneralSearchField($searchParams, $existingQuery);

Expand All @@ -65,7 +57,14 @@ public function getQuery($searchParams, $sort = false, $limit = false, $existing
}

// Limit must be last so that ArrayList results don't have an applied limit before they can be filtered/sorted.
$result = $result->limit($limit);
if (is_array($limit)) {
$result = $result->limit(
isset($limit['limit']) ? $limit['limit'] : null,
isset($limit['start']) ? $limit['start'] : null
);
} else {
$result = $result->limit($limit);
}

return $result;
}
Expand Down
14 changes: 2 additions & 12 deletions src/ORM/Search/SearchContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -142,22 +142,15 @@ protected function applyBaseTableFields()
* If a filter is applied to a relationship in dot notation,
* the parameter name should have the dots replaced with double underscores,
* for example "Comments__Name" instead of the filter name "Comments.Name".
* @param array|bool|string $sort Database column to sort on.
* @param array|false|string $sort Database column to sort on.
* Falls back to {@link DataObject::$default_sort} if not provided.
* @param int|array|null $limit
* @param DataList $existingQuery
* @return DataList<T>
* @throws Exception
*/
public function getQuery($searchParams, $sort = false, $limit = false, $existingQuery = null)
public function getQuery($searchParams, $sort = false, int|array|null $limit = null, $existingQuery = null)
{
if ((count(func_get_args()) >= 3) && (!in_array(gettype($limit), ['integer', 'array', 'NULL']))) {
Deprecation::notice(
'5.1.0',
'$limit should be type of int|array|null'
);
$limit = null;
}
$this->setSearchParams($searchParams);
$query = $this->prepareQuery($sort, $limit, $existingQuery);
return $this->search($query);
Expand Down Expand Up @@ -197,9 +190,6 @@ private function prepareQuery($sort, $limit, ?DataList $existingQuery): DataList
}
$query = null;
if ($existingQuery) {
if (!($existingQuery instanceof DataList)) {
throw new InvalidArgumentException("existingQuery must be DataList");
}
if ($existingQuery->dataClass() != $this->modelClass) {
throw new InvalidArgumentException("existingQuery's dataClass is " . $existingQuery->dataClass()
. ", $this->modelClass expected.");
Expand Down

0 comments on commit a83de6e

Please sign in to comment.