NEW Enable ArrayList and EagerLoadedList to use search filters #10925

GuySartorelli · 2023-08-23T00:51:21Z

Replaces #10910

Issue

Static lists like ArrayList or EagerLoadedDataList should support SearchFilter syntax for filter/exclude #5911

GuySartorelli · 2023-08-28T03:40:33Z

src/ORM/ArrayList.php

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.

GuySartorelli · 2023-08-28T03:41:23Z

src/ORM/ArrayList.php

+    public function excludeAny(): static
+    {
+        $filters = call_user_func_array([$this, 'normaliseFilterArgs'], func_get_args());
+        return $this->filterOrExclude($filters, false, true);
+    }


Adding this new method isn't necessary by any means - but it's a very low effort, low risk change that helps align the ArrayList API with other lists. I think it's well worth adding and I've provided adequate test coverage.

GuySartorelli · 2023-08-28T03:43:00Z

src/ORM/ArrayList.php

-        $list->items = array_unique($itemsToKeep ?? [], SORT_REGULAR);
+        $list->items = $itemsToKeep;


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

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.

GuySartorelli · 2023-08-28T03:44:46Z

src/ORM/ArrayList.php

+            // Convert null to an empty string for backwards compatability, since nulls are treated specially
+            // in the ExactMatchFilter


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.

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?

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

GuySartorelli · 2023-08-28T03:46:28Z

src/ORM/ArrayList.php

+            // 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']);
+                }
+            }


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.

GuySartorelli · 2023-08-28T03:50:15Z

src/ORM/EagerLoadedList.php

-            if (!$this->canFilterBy($column)) {
+            if (!$this->canFilterBy(explode(':', $column)[0])) {
                throw new InvalidArgumentException("Can't filter by column '$column'");
            }


This exception is here for parity with DataList, so we need to make sure we can filter by the underlying field.

GuySartorelli · 2023-08-28T03:52:37Z

src/ORM/Filters/SearchFilter.php

+    /**
+     * Whether search filters should be case sensitive or not by default.
+     * If null, the database collation setting is used.
+     */
+    private static ?bool $default_case_sensitive = null;


This configuration property isn't strictly necessary for this functionality to work, but it makes testing a lot easier and allows developers to declare a default case sensitivity for their project without having to muck about with database configuration, which is the current way to define the default case sensitivity.

I've added some tests for this in DataList.

GuySartorelli · 2023-08-28T04:00:10Z

src/ORM/Filters/SearchFilter.php

-     * @return Mixed TRUE or FALSE to enforce sensitivity, NULL to use field collation.
+     * @return ?bool TRUE or FALSE to enforce sensitivity, NULL to use field collation.


I haven't actually changed the return type - it was always returning only these types.

GuySartorelli · 2023-08-28T04:04:06Z

src/ORM/Filters/UsesSearchFilters.php

This implementation is just a straight cut & paste from DataList::createSearchFilter() - turned it into a trait to avoid code duplication since it's now used in three separate places and we want to keep the implementations in sync to avoid regressions.

GuySartorelli · 2023-08-28T04:04:28Z

tests/php/ORM/EagerLoadedListTest.php

Some of the new test scenarios are from DataListTest but most are brand new

emteknetnz

Looks good, love all the tests. Some minor changes, mostly just naming

src/ORM/ArrayList.php

emteknetnz · 2023-08-28T22:01:34Z

src/ORM/ArrayList.php

+            // Convert null to an empty string for backwards compatability, since nulls are treated specially
+            // in the ExactMatchFilter


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?

emteknetnz · 2023-08-28T22:10:40Z

src/ORM/ArrayList.php

-        $list->items = array_unique($itemsToKeep ?? [], SORT_REGULAR);
+        $list->items = $itemsToKeep;


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.

src/ORM/Filters/UsesSearchFilters.php

src/ORM/Filters/ComparisonFilter.php

src/ORM/Filters/ExactMatchFilter.php

src/ORM/Filters/PartialMatchFilter.php

src/ORM/Filters/SearchFilter.php

emteknetnz

Add some unicode unit tests

GuySartorelli · 2023-08-29T02:21:41Z

Add some unicode unit tests

Done.

…335)

GuySartorelli marked this pull request as draft August 23, 2023 00:51

GuySartorelli force-pushed the pulls/5/searchfilter-static-lists2 branch from 09bc9fb to c8992b8 Compare August 23, 2023 02:05

GuySartorelli mentioned this pull request Aug 21, 2023

Static lists like ArrayList or EagerLoadedDataList should support SearchFilter syntax for filter/exclude #5911

Closed

6 tasks

GuySartorelli force-pushed the pulls/5/searchfilter-static-lists2 branch 7 times, most recently from 1417710 to eb6fa35 Compare August 28, 2023 04:03

GuySartorelli commented Aug 28, 2023

View reviewed changes

GuySartorelli added a commit to creative-commoners/developer-docs that referenced this pull request Aug 28, 2023

DOC Document changes from silverstripe/silverstripe-framework#10925

2ad9089

GuySartorelli added a commit to creative-commoners/developer-docs that referenced this pull request Aug 28, 2023

DOC Document changes from silverstripe/silverstripe-framework#10925

b8ff34f

GuySartorelli force-pushed the pulls/5/searchfilter-static-lists2 branch from eb6fa35 to 69f06e5 Compare August 28, 2023 04:37

GuySartorelli mentioned this pull request Aug 28, 2023

CMS 6: Remove backwards compatibility casting null to empty string in ArrayList::filterOrExclude() #10930

Open

2 tasks

GuySartorelli marked this pull request as ready for review August 28, 2023 04:59

emteknetnz requested changes Aug 28, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/5/searchfilter-static-lists2 branch 3 times, most recently from 0977590 to e66b425 Compare August 28, 2023 23:46

GuySartorelli mentioned this pull request Aug 29, 2023

Silverstripe 5 support signify-nz/silverstripe-searchfilter-arraylist#3

Merged

emteknetnz requested changes Aug 29, 2023

View reviewed changes

GuySartorelli mentioned this pull request Aug 29, 2023

CMS 6: Make SearchFilter::matches() and ComparisonFilter::match() abstract #10933

Open

NEW Enable ArrayList and EagerLoadedList to use search filters

4998353

GuySartorelli force-pushed the pulls/5/searchfilter-static-lists2 branch from e66b425 to 4998353 Compare August 29, 2023 02:21

emteknetnz approved these changes Aug 29, 2023

View reviewed changes

emteknetnz merged commit b4463d9 into silverstripe:5 Aug 29, 2023

emteknetnz deleted the pulls/5/searchfilter-static-lists2 branch August 29, 2023 03:40

emteknetnz pushed a commit to silverstripe/developer-docs that referenced this pull request Aug 29, 2023

DOC Document changes from silverstripe/silverstripe-framework#10925 (#…

b45605e

…335)

GuySartorelli mentioned this pull request Aug 30, 2023

FIX Correctly implement backwards compatible null comparisons #10935

Merged

sig-steve mentioned this pull request Nov 27, 2023

Silverstripe 5 Incompatibilities signify-nz/silverstripe-searchfilter-arraylist#4

Closed

GuySartorelli mentioned this pull request Jun 17, 2024

ENH Use class name instead of self #11270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW Enable ArrayList and EagerLoadedList to use search filters #10925

NEW Enable ArrayList and EagerLoadedList to use search filters #10925

GuySartorelli commented Aug 23, 2023

GuySartorelli Aug 28, 2023

GuySartorelli Aug 28, 2023

GuySartorelli Aug 28, 2023

emteknetnz Aug 28, 2023

GuySartorelli Aug 28, 2023

emteknetnz Aug 28, 2023

GuySartorelli Aug 28, 2023 •

edited

Loading

GuySartorelli Aug 28, 2023

GuySartorelli Aug 28, 2023

GuySartorelli Aug 28, 2023

GuySartorelli Aug 28, 2023

GuySartorelli Aug 28, 2023

GuySartorelli Aug 28, 2023

emteknetnz left a comment

emteknetnz Aug 28, 2023

emteknetnz Aug 28, 2023

emteknetnz left a comment

GuySartorelli commented Aug 29, 2023

		$list->items = array_unique($itemsToKeep ?? [], SORT_REGULAR);
		$list->items = $itemsToKeep;

		// Convert null to an empty string for backwards compatability, since nulls are treated specially
		// in the ExactMatchFilter

		* @return Mixed TRUE or FALSE to enforce sensitivity, NULL to use field collation.
		* @return ?bool TRUE or FALSE to enforce sensitivity, NULL to use field collation.

NEW Enable ArrayList and EagerLoadedList to use search filters #10925

NEW Enable ArrayList and EagerLoadedList to use search filters #10925

Conversation

GuySartorelli commented Aug 23, 2023

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

GuySartorelli commented Aug 29, 2023

GuySartorelli Aug 28, 2023 •

edited

Loading