FIX Correctly implement backwards compatible null comparisons (#10935)
This commit is contained in:
Guy Sartorelli
GitHub
parent
fb5ad120c5
commit
374771d4d7
|
|
@ -9,6 +9,7 @@ use SilverStripe\Core\ClassInfo;
|
|||
use SilverStripe\Core\Injector\Injector;
|
||||
use SilverStripe\Dev\Debug;
|
||||
use SilverStripe\Dev\Deprecation;
|
||||
use SilverStripe\ORM\Filters\ExactMatchFilter;
|
||||
use SilverStripe\ORM\Filters\SearchFilter;
|
||||
use SilverStripe\ORM\Filters\SearchFilterable;
|
||||
use SilverStripe\View\ArrayData;
|
||||
|
|
@ -707,11 +708,21 @@ class ArrayList extends ViewableData implements SS_List, Filterable, Sortable, L
|
|||
{
|
||||
$itemsToKeep = [];
|
||||
$searchFilters = [];
|
||||
$hasNullFilter = false;
|
||||
|
||||
foreach ($filters as $filterKey => $filterValue) {
|
||||
// Convert null to an empty string for backwards compatability, since nulls are treated specially
|
||||
// Check if we have any null filter values for backwards compatability, since nulls are treated specially
|
||||
// in the ExactMatchFilter
|
||||
$searchFilter = $this->createSearchFilter($filterKey, $filterValue ?? '');
|
||||
if (is_array($filterValue)) {
|
||||
foreach ($filterValue as $value) {
|
||||
if ($value === null) {
|
||||
$hasNullFilter = true;
|
||||
}
|
||||
}
|
||||
} elseif ($filterValue === null) {
|
||||
$hasNullFilter = true;
|
||||
}
|
||||
$searchFilter = $this->createSearchFilter($filterKey, $filterValue);
|
||||
|
||||
// Apply default case sensitivity for backwards compatability
|
||||
if (!str_contains($filterKey, ':case') && !str_contains($filterKey, ':nocase')) {
|
||||
|
|
@ -731,7 +742,23 @@ class ArrayList extends ViewableData implements SS_List, Filterable, Sortable, L
|
|||
foreach ($filters as $filterKey => $filterValue) {
|
||||
/** @var SearchFilter $searchFilter */
|
||||
$searchFilter = $searchFilters[$filterKey];
|
||||
$hasMatch = $searchFilter->matches($this->extractValue($item, $searchFilter->getFullName()) ?? '');
|
||||
$extractedValue = $this->extractValue($item, $searchFilter->getFullName());
|
||||
$hasMatch = null;
|
||||
|
||||
// If we need to do a legacy null comparison, try that first.
|
||||
if (($searchFilter instanceof ExactMatchFilter) && ($hasNullFilter || $extractedValue === null)) {
|
||||
$hasMatch = $this->performLegacyNullMatch($extractedValue, $filterValue);
|
||||
if ($hasMatch !== null && in_array('not', $searchFilter->getModifiers())) {
|
||||
$hasMatch = !$hasMatch;
|
||||
}
|
||||
}
|
||||
|
||||
// If the null comparison wasn't necessary or was incomplete, let searchfilters do the work.
|
||||
if ($hasMatch === null) {
|
||||
$hasMatch = $searchFilter->matches($extractedValue);
|
||||
}
|
||||
|
||||
|
||||
$matches[$hasMatch] = 1;
|
||||
// If this is excludeAny or filterAny and we have a match, we can stop looking for matches.
|
||||
if ($any && $hasMatch) {
|
||||
|
|
@ -754,6 +781,27 @@ class ArrayList extends ViewableData implements SS_List, Filterable, Sortable, L
|
|||
return $list;
|
||||
}
|
||||
|
||||
/**
|
||||
* Required for backwards compatibility since ExactMatch handles null values differently than ArrayList used to.
|
||||
*/
|
||||
private function performLegacyNullMatch(mixed $objectValue, mixed $filterValues): ?bool
|
||||
{
|
||||
if (!is_array($filterValues)) {
|
||||
$filterValues = [$filterValues];
|
||||
}
|
||||
foreach ($filterValues as $filterValue) {
|
||||
// Skip comparisons between two non-null values, we can trust searchfilter for those.
|
||||
if ($objectValue !== null && $filterValue !== null) {
|
||||
continue;
|
||||
}
|
||||
// This is the legacy comparison.
|
||||
if ($filterValue == $objectValue) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return $objectValue === null ? false : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Take the "standard" arguments that the filter/exclude functions take and return a single array with
|
||||
* 'colum' => 'value'
|
||||
|
|
|
|||
|
|
@ -970,6 +970,127 @@ class ArrayListTest extends SapphireTest
|
|||
$this->assertEquals($expected, $list->toArray(), 'List should only contain Steve and Steve and Clair');
|
||||
}
|
||||
|
||||
/**
|
||||
* @dataProvider provideFilterNullComparisons
|
||||
*/
|
||||
public function testFilterNullComparisons(mixed $objectValue, mixed $filterValue, bool $doesMatch, bool $negated = false)
|
||||
{
|
||||
$filterField = 'Value';
|
||||
if ($negated) {
|
||||
$filterField .= ':not';
|
||||
}
|
||||
$list = new ArrayList([['Value' => $objectValue]]);
|
||||
$list = $list->filter($filterField, $filterValue);
|
||||
$this->assertCount($doesMatch ? 1 : 0, $list);
|
||||
}
|
||||
|
||||
public function provideFilterNullComparisons()
|
||||
{
|
||||
// This is for backwards compatibility, since arraylist used to just do a straight == comparison
|
||||
// Everything that passes here would have passed a $objectValue == $filterValue comparison previously
|
||||
$scenarios = [
|
||||
[
|
||||
'objectValue' => null,
|
||||
'filterValues' => null,
|
||||
'doesMatch' => true,
|
||||
],
|
||||
[
|
||||
'objectValue' => null,
|
||||
'filterValues' => '',
|
||||
'doesMatch' => true,
|
||||
],
|
||||
[
|
||||
'objectValue' => '',
|
||||
'filterValues' => null,
|
||||
'doesMatch' => true,
|
||||
],
|
||||
[
|
||||
'objectValue' => null,
|
||||
'filterValues' => 0,
|
||||
'doesMatch' => true,
|
||||
],
|
||||
[
|
||||
'objectValue' => 0,
|
||||
'filterValues' => null,
|
||||
'doesMatch' => true,
|
||||
],
|
||||
[
|
||||
'objectValue' => false,
|
||||
'filterValues' => null,
|
||||
'doesMatch' => true,
|
||||
],
|
||||
[
|
||||
'objectValue' => null,
|
||||
'filterValues' => false,
|
||||
'doesMatch' => true,
|
||||
],
|
||||
[
|
||||
'objectValue' => [],
|
||||
'filterValues' => null,
|
||||
'doesMatch' => true,
|
||||
],
|
||||
[
|
||||
'objectValue' => null,
|
||||
'filterValues' => [[]],
|
||||
'doesMatch' => true,
|
||||
],
|
||||
// Include some multi-value filters
|
||||
[
|
||||
'objectValue' => null,
|
||||
'filterValues' => ['one', '', 1],
|
||||
'doesMatch' => true,
|
||||
],
|
||||
[
|
||||
'objectValue' => null,
|
||||
'filterValues' => ['one', '1', 1],
|
||||
'doesMatch' => false,
|
||||
],
|
||||
[
|
||||
'objectValue' => '',
|
||||
'filterValues' => ['one', null, 1],
|
||||
'doesMatch' => true,
|
||||
],
|
||||
// Check that we're not skipping comparisons that don't match null
|
||||
[
|
||||
'objectValue' => '1',
|
||||
'filterValues' => ['one', null, 1],
|
||||
'doesMatch' => true,
|
||||
],
|
||||
// This is here because 0 == '0' is true, and 0 == null is true, so essentially protecting
|
||||
// against swapping null out for 0 in attempt to pass the other tests.
|
||||
[
|
||||
'objectValue' => '0',
|
||||
'filterValues' => null,
|
||||
'doesMatch' => false,
|
||||
],
|
||||
[
|
||||
'objectValue' => null,
|
||||
'filterValues' => '0',
|
||||
'doesMatch' => false,
|
||||
],
|
||||
// We're comparing with false above so this is just a sanity check.
|
||||
[
|
||||
'objectValue' => true,
|
||||
'filterValues' => null,
|
||||
'doesMatch' => false,
|
||||
],
|
||||
[
|
||||
'objectValue' => null,
|
||||
'filterValues' => true,
|
||||
'doesMatch' => false,
|
||||
],
|
||||
];
|
||||
|
||||
// Ensure the not modifier works as expected
|
||||
foreach ($scenarios as $scenario) {
|
||||
$scenario['doesMatch'] = !$scenario['doesMatch'];
|
||||
$scenario['negated'] = true;
|
||||
$scenarios[] = $scenario;
|
||||
}
|
||||
|
||||
return $scenarios;
|
||||
}
|
||||
|
||||
private function getFilterWithSearchfiltersObjects()
|
||||
{
|
||||
return [
|
||||
|
|
|
|||
Loading…
Reference in New Issue