FIX Correctly implement backwards compatible null comparisons #10935
Conversation
tests/php/ORM/ArrayListTest.php
Outdated
There was a problem hiding this comment.
These were all the null comparisons I could think of, as far as things that should match. If you think of any others, please ask me to add them.
| $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); |
There was a problem hiding this comment.
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.
src/ORM/ArrayList.php
Outdated
| $hasMatch = $searchFilter->matches($this->extractValue($item, $searchFilter->getFullName()) ?? ''); | ||
| $extractedValue = $this->extractValue($item, $searchFilter->getFullName()); | ||
| $hasMatch = $searchFilter->matches($extractedValue); | ||
|
|
||
| // If we didn't get a match, check if we need to do a legacy null | ||
| // comparison for backwards compatibility | ||
| if (!$hasMatch | ||
| && ($searchFilter instanceof ExactMatchFilter) | ||
| && ($hasNullFilter || $extractedValue === null) | ||
| ) { | ||
| $hasMatch = $this->performLegacyNullMatch($extractedValue, $filterValue); | ||
| if (in_array('not', $searchFilter->getModifiers())) { | ||
| $hasMatch = !$hasMatch; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Instead of just casting null to empty string, we now first let the search filter do its thing. Only if it doesn't get a match, do we then check if we need to do a legacy null comparison.
There was a problem hiding this comment.
Changed this to doing the null check first (if necessary), due to some edge cases. But if the object value isn't null and we don't get a match, we still pass it through to the search filter afterward.
|
Oops I need to add tests to make sure this works as expecte dwith the not modifier. Changed to draft pending that since I'm not able to do it right now. |
GuySartorelli
force-pushed
the
pulls/5/fix-broken-tests
branch
from
e0ff5d4 to
b9b166a
Compare
#10925 introduced a change to the way
ArrayListfiltering works. It had a naive cast of null values to empty strings, but that wasn't sufficient for all null comparisons.This resulted in breaking this unit test, because subsites was using
ArrayListto filternullagainst0, which used to work but didn't work after the above linked PR.After making these changes, these failing tests are also passing locally so it looks like it was causing those as well.
Issue