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

FIX Correctly implement backwards compatible null comparisons #10935

Merged
merged 1 commit into from
Aug 31, 2023
Merged

FIX Correctly implement backwards compatible null comparisons #10935

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
54 changes: 51 additions & 3 deletions src/ORM/ArrayList.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
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;
Expand Down Expand Up @@ -707,11 +708,21 @@ protected function filterOrExclude(array $filters, bool $inclusive = true, bool
{
$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);
Comment on lines +711 to +725
Copy link
Member Author

Choose a reason for hiding this comment

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

Don't cast the null value to empty string anymore since 0 == '' evaluates false. Instead, just take note of if we have any null values for later.


// Apply default case sensitivity for backwards compatability
if (!str_contains($filterKey, ':case') && !str_contains($filterKey, ':nocase')) {
Expand All @@ -731,7 +742,23 @@ protected function filterOrExclude(array $filters, bool $inclusive = true, bool
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) {
Expand All @@ -754,6 +781,27 @@ protected function filterOrExclude(array $filters, bool $inclusive = true, bool
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'
Expand Down
121 changes: 121 additions & 0 deletions tests/php/ORM/ArrayListTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,6 +970,127 @@ public function testMultipleWithArrayFilterAdvanced()
$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 [
Expand Down