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.

Sign up for GitHub

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

NEW Enable ArrayList and EagerLoadedList to use search filters #10925

Merged
merged 1 commit into from
Aug 29, 2023
Merged

NEW Enable ArrayList and EagerLoadedList to use search filters #10925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
217 changes: 129 additions & 88 deletions src/ORM/ArrayList.php
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved a couple of methods in this class to be next to each other where they have similar functionality. I've only moved methods that are already being changed in this PR anyway.

Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@
use ArrayIterator;
use InvalidArgumentException;
use LogicException;
use SilverStripe\Core\ClassInfo;
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Dev\Debug;
use SilverStripe\Dev\Deprecation;
use SilverStripe\ORM\Filters\SearchFilter;
use SilverStripe\ORM\Filters\SearchFilterable;
use SilverStripe\View\ArrayData;
use SilverStripe\View\ViewableData;
use Traversable;
Expand All @@ -26,6 +31,15 @@
*/
class ArrayList extends ViewableData implements SS_List, Filterable, Sortable, Limitable
{
use SearchFilterable;

/**
* Whether filter and exclude calls should be case sensitive by default or not.
* This configuration property is here for backwards compatability.
*
* @deprecated 5.1.0 use SearchFilter.default_case_sensitive instead
*/
private static bool $default_case_sensitive = true;

/**
* Holds the items in the list
Expand Down Expand Up @@ -368,23 +382,6 @@ public function map($keyfield = 'ID', $titlefield = 'Title')
return new Map($list, $keyfield, $titlefield);
}

/**
* Find the first item of this list where the given key = value
*
* @param string $key
* @param string $value
* @return mixed
*/
public function find($key, $value)
{
foreach ($this->items as $item) {
if ($this->extractValue($item, $key) == $value) {
return $item;
}
}
return null;
}

/**
* Returns an array of a single field value for all items in the list.
*
Expand Down Expand Up @@ -594,6 +591,18 @@ public function canFilterBy($by)
return property_exists($firstRecord, $by ?? '');
}

/**
* Find the first item of this list where the given key = value
*
* @param string $key
* @param string $value
* @return mixed
*/
public function find($key, $value)
{
return $this->filter($key, $value)->first();
}

/**
* Filter the list to include items with these characteristics
*
Expand All @@ -605,31 +614,15 @@ public function canFilterBy($by)
* @example $list->filter(array('Name'=>'bob, 'Age'=>array(21, 43))); // bob with the Age 21 or 43
* @example $list->filter(array('Name'=>array('aziz','bob'), 'Age'=>array(21, 43)));
* // aziz with the age 21 or 43 and bob with the Age 21 or 43
*
* Also supports SearchFilter syntax
* @example // include anyone with "sam" anywhere in their name
* $list = $list->filter('Name:PartialMatch', 'sam');
*/
public function filter()
{

$keepUs = call_user_func_array([$this, 'normaliseFilterArgs'], func_get_args());

$itemsToKeep = [];
foreach ($this->items as $item) {
$keepItem = true;
foreach ($keepUs as $column => $value) {
if ((is_array($value) && !in_array($this->extractValue($item, $column), $value ?? []))
|| (!is_array($value) && $this->extractValue($item, $column) != $value)
) {
$keepItem = false;
break;
}
}
if ($keepItem) {
$itemsToKeep[] = $item;
}
}

$list = clone $this;
$list->items = $itemsToKeep;
return $list;
$filters = $this->normaliseFilterArgs(...func_get_args());
return $this->filterOrExclude($filters);
}

/**
Expand All @@ -646,28 +639,118 @@ public function filter()
* @example // all bobs, phils or anyone aged 21 or 43 in the list
* $list = $list->filterAny(array('Name'=>array('bob','phil'), 'Age'=>array(21, 43)));
*
* Also supports SearchFilter syntax
* @example // include anyone with "sam" anywhere in their name
* $list = $list->filterAny('Name:PartialMatch', 'sam');
*
* @param string|array See {@link filter()}
* @return static
*/
public function filterAny()
{
$keepUs = $this->normaliseFilterArgs(...func_get_args());
$filters = $this->normaliseFilterArgs(...func_get_args());
return $this->filterOrExclude($filters, true, true);
}

/**
* Exclude the list to not contain items with these characteristics
*
* @return ArrayList
* @see SS_List::exclude()
* @example $list->exclude('Name', 'bob'); // exclude bob from list
* @example $list->exclude('Name', array('aziz', 'bob'); // exclude aziz and bob from list
* @example $list->exclude(array('Name'=>'bob, 'Age'=>21)); // exclude bob that has Age 21
* @example $list->exclude(array('Name'=>'bob, 'Age'=>array(21, 43))); // exclude bob with Age 21 or 43
* @example $list->exclude(array('Name'=>array('bob','phil'), 'Age'=>array(21, 43)));
* // bob age 21 or 43, phil age 21 or 43 would be excluded
*
* Also supports SearchFilter syntax
* @example // everyone except anyone with "sam" anywhere in their name
* $list = $list->exclude('Name:PartialMatch', 'sam');
*/
public function exclude()
{
$filters = $this->normaliseFilterArgs(...func_get_args());
return $this->filterOrExclude($filters, false);
}

/**
* Return a copy of the list excluding any items that have any of these characteristics
*
* @example // everyone except bob in the list
* $list = $list->excludeAny('Name', 'bob');
* @example // everyone except azis or bob in the list
* $list = $list->excludeAny('Name', array('aziz', 'bob');
* @example // everyone except bob or anyone aged 21 in the list
* $list = $list->excludeAny(array('Name'=>'bob, 'Age'=>21));
* @example // everyone except bob or anyone aged 21 or 43 in the list
* $list = $list->excludeAny(array('Name'=>'bob, 'Age'=>array(21, 43)));
* @example // everyone except all bobs, phils or anyone aged 21 or 43 in the list
* $list = $list->excludeAny(array('Name'=>array('bob','phil'), 'Age'=>array(21, 43)));
*
* Also supports SearchFilter syntax
* @example // everyone except anyone with "sam" anywhere in their name
* $list = $list->excludeAny('Name:PartialMatch', 'sam');
*
* @param string|array See {@link filter()}
*/
public function excludeAny(): static
{
$filters = $this->normaliseFilterArgs(...func_get_args());
return $this->filterOrExclude($filters, false, true);
}

/**
* Apply the appropriate filtering or excluding
*/
protected function filterOrExclude(array $filters, bool $inclusive = true, bool $any = false): static
{
$itemsToKeep = [];
$searchFilters = [];

foreach ($filters as $filterKey => $filterValue) {
// Convert null to an empty string for backwards compatability, since nulls are treated specially
// in the ExactMatchFilter
Comment on lines +712 to +713
Copy link
Member Author

Choose a reason for hiding this comment

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

Converting null to an empty string means we retain backwards compatability where it used to be doing a $value1 == $value2 check. ExactMatchFilter is doing a string === comparison if there's a null value involved.
By swapping null out for an empty string here, we end up with a value that behaves identically to null with the looser comparison and therefore retains the same behaviour we had prior to this PR for ArrayList.

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to hae a Deprecation::notice() when it finds a null $filterValue and say that the behaviour will be changed in CMS 6?

Copy link
Member Author

@GuySartorelli GuySartorelli Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so - we're not going to deprecate passing in null values, so deprecation isn't quite right. We'll just be changing how they're treated in CMS 6. I think calling it out in the CMS 6 changelog will be sufficient.
Note there is an issue to change this behaviour already: #10930

$searchFilter = $this->createSearchFilter($filterKey, $filterValue ?? '');

// Apply default case sensitivity for backwards compatability
if (!str_contains($filterKey, ':case') && !str_contains($filterKey, ':nocase')) {
$caseSensitive = Deprecation::withNoReplacement(fn() => static::config()->get('default_case_sensitive'));
if ($caseSensitive && in_array('case', $searchFilter->getSupportedModifiers())) {
$searchFilter->setModifiers($searchFilter->getModifiers() + ['case']);
} elseif (!$caseSensitive && in_array('nocase', $searchFilter->getSupportedModifiers())) {
$searchFilter->setModifiers($searchFilter->getModifiers() + ['nocase']);
}
}
Comment on lines +716 to +724
Copy link
Member Author

Choose a reason for hiding this comment

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

ArrayList was implicitly case sensitive before, because it was just doing a simple == check. We therefore have to assume any filters that aren't adding a case or nocase filter will use the default case sensitivity.
I've made that default configurable so that people can set their projects to have a uniform default case sensitivity across all the list implementations.


$searchFilters[$filterKey] = $searchFilter;
}

foreach ($this->items as $item) {
foreach ($keepUs as $column => $value) {
$extractedValue = $this->extractValue($item, $column);
$matches = is_array($value) ? in_array($extractedValue, $value) : $extractedValue == $value;
if ($matches) {
$itemsToKeep[] = $item;
$matches = [];
foreach ($filters as $filterKey => $filterValue) {
/** @var SearchFilter $searchFilter */
$searchFilter = $searchFilters[$filterKey];
$hasMatch = $searchFilter->matches($this->extractValue($item, $searchFilter->getFullName()) ?? '');
$matches[$hasMatch] = 1;
// If this is excludeAny or filterAny and we have a match, we can stop looking for matches.
if ($any && $hasMatch) {
break;
}
}

// filterAny or excludeAny allow any true value to be a match; filter or exclude require any false value
// to be a mismatch.
$isMatch = $any ? isset($matches[true]) : !isset($matches[false]);

// If inclusive (filter) and we have a match, or exclusive (exclude) and there is NO match, keep the item.
if (($inclusive && $isMatch) || (!$inclusive && !$isMatch)) {
$itemsToKeep[] = $item;
}
}

$list = clone $this;
$list->items = array_unique($itemsToKeep ?? [], SORT_REGULAR);
$list->items = $itemsToKeep;
Comment on lines -670 to +753
Copy link
Member Author

Choose a reason for hiding this comment

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

We absolutely shouldn't be sorting here. That's what the sort method is for.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah shouldn't .... you could argue it is a behaviour change so existing data on a website could change its presentation after upgrade. However, I'd counter and say we're fixing a bug here.

return $list;
}

Expand Down Expand Up @@ -755,48 +838,6 @@ public function filterByCallback($callback)
return $output;
}

/**
* Exclude the list to not contain items with these characteristics
*
* @return ArrayList
* @see SS_List::exclude()
* @example $list->exclude('Name', 'bob'); // exclude bob from list
* @example $list->exclude('Name', array('aziz', 'bob'); // exclude aziz and bob from list
* @example $list->exclude(array('Name'=>'bob, 'Age'=>21)); // exclude bob that has Age 21
* @example $list->exclude(array('Name'=>'bob, 'Age'=>array(21, 43))); // exclude bob with Age 21 or 43
* @example $list->exclude(array('Name'=>array('bob','phil'), 'Age'=>array(21, 43)));
* // bob age 21 or 43, phil age 21 or 43 would be excluded
*/
public function exclude()
{
$removeUs = $this->normaliseFilterArgs(...func_get_args());

$hitsRequiredToRemove = count($removeUs ?? []);
$matches = [];
foreach ($removeUs as $column => $excludeValue) {
foreach ($this->items as $key => $item) {
if (!is_array($excludeValue) && $this->extractValue($item, $column) == $excludeValue) {
$matches[$key] = isset($matches[$key]) ? $matches[$key] + 1 : 1;
} elseif (is_array($excludeValue) && in_array($this->extractValue($item, $column), $excludeValue ?? [])) {
$matches[$key] = isset($matches[$key]) ? $matches[$key] + 1 : 1;
}
}
}

$keysToRemove = array_keys($matches ?? [], $hitsRequiredToRemove);

$itemsToKeep = [];
foreach ($this->items as $key => $value) {
if (!in_array($key, $keysToRemove ?? [])) {
$itemsToKeep[] = $value;
}
}

$list = clone $this;
$list->items = $itemsToKeep;
return $list;
}

protected function shouldExclude($item, $args)
{
}
Expand Down
42 changes: 3 additions & 39 deletions src/ORM/DataList.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

use SilverStripe\Core\Injector\Injector;
use SilverStripe\Dev\Debug;
use SilverStripe\ORM\Filters\SearchFilter;
use SilverStripe\ORM\Queries\SQLConditionGroup;
use SilverStripe\View\ViewableData;
use Exception;
Expand All @@ -15,6 +14,7 @@
use Traversable;
use SilverStripe\ORM\DataQuery;
use SilverStripe\ORM\ArrayList;
use SilverStripe\ORM\Filters\SearchFilterable;

/**
* Implements a "lazy loading" DataObjectSet.
Expand All @@ -38,6 +38,8 @@
*/
class DataList extends ViewableData implements SS_List, Filterable, Sortable, Limitable
{
use SearchFilterable;

/**
* Whether to use placeholders for integer IDs on Primary and Foriegn keys during a WHERE IN query
* It is significantly faster to not use placeholders
Expand Down Expand Up @@ -665,44 +667,6 @@ protected function isValidRelationName($field)
return preg_match('/^[A-Z0-9\._]+$/i', $field ?? '');
}

/**
* Given a filter expression and value construct a {@see SearchFilter} instance
*
* @param string $filter E.g. `Name:ExactMatch:not`, `Name:ExactMatch`, `Name:not`, `Name`
* @param mixed $value Value of the filter
* @return SearchFilter
*/
protected function createSearchFilter($filter, $value)
{
// Field name is always the first component
$fieldArgs = explode(':', $filter ?? '');
$fieldName = array_shift($fieldArgs);

// Inspect type of second argument to determine context
$secondArg = array_shift($fieldArgs);
$modifiers = $fieldArgs;
if (!$secondArg) {
// Use default filter if none specified. E.g. `->filter(['Name' => $myname])`
$filterServiceName = 'DataListFilter.default';
} else {
// The presence of a second argument is by default ambiguous; We need to query
// Whether this is a valid modifier on the default filter, or a filter itself.
/** @var SearchFilter $defaultFilterInstance */
$defaultFilterInstance = Injector::inst()->get('DataListFilter.default');
if (in_array(strtolower($secondArg ?? ''), $defaultFilterInstance->getSupportedModifiers() ?? [])) {
// Treat second (and any subsequent) argument as modifiers, using default filter
$filterServiceName = 'DataListFilter.default';
array_unshift($modifiers, $secondArg);
} else {
// Second argument isn't a valid modifier, so assume is filter identifier
$filterServiceName = "DataListFilter.{$secondArg}";
}
}

// Build instance
return Injector::inst()->create($filterServiceName, $fieldName, $value, $modifiers);
}

/**
* Return a copy of this list which does not contain any items that match all params
*
Expand Down
Loading