From e12b25e5107f6bf454a4788b7b2e454889c7365f Mon Sep 17 00:00:00 2001 From: "o.kravchuk" Date: Mon, 27 Jul 2026 12:17:44 +0300 Subject: [PATCH] magento/magento2#32292: Fix FilterPool crash on non-AbstractDb collections FilterPool::applyFilters always called getSelect() after MC-24195, which fatals for UI grid collections that extend Framework Data\Collection only (API-backed / in-memory grids). Keep SQL WHERE regrouping for AbstractDb and restore the pre-2.4.2 applier-only path for other collections. Adds unit and integration coverage for both paths. --- .../DataProvider/FilterPoolTest.php | 211 ++++++++++++++++ .../UiComponent/DataProvider/FilterPool.php | 67 +++++- .../DataProvider/FilterPoolTest.php | 227 ++++++++++++++++++ 3 files changed, 494 insertions(+), 11 deletions(-) create mode 100644 dev/tests/integration/testsuite/Magento/Framework/View/Element/UiComponent/DataProvider/FilterPoolTest.php create mode 100644 lib/internal/Magento/Framework/View/Test/Unit/Element/UiComponent/DataProvider/FilterPoolTest.php diff --git a/dev/tests/integration/testsuite/Magento/Framework/View/Element/UiComponent/DataProvider/FilterPoolTest.php b/dev/tests/integration/testsuite/Magento/Framework/View/Element/UiComponent/DataProvider/FilterPoolTest.php new file mode 100644 index 0000000000000..810cd94d1dcfe --- /dev/null +++ b/dev/tests/integration/testsuite/Magento/Framework/View/Element/UiComponent/DataProvider/FilterPoolTest.php @@ -0,0 +1,211 @@ +filterPool = $objectManager->get(FilterPool::class); + $this->filterBuilder = $objectManager->get(FilterBuilder::class); + $this->filterGroupBuilder = $objectManager->get(FilterGroupBuilder::class); + $this->searchCriteriaFactory = $objectManager->get(SearchCriteriaFactory::class); + $this->entityFactory = $objectManager->get(EntityFactoryInterface::class); + } + + /** + * Loading a grid with a non-database collection and no filters must not fatal. + */ + public function testApplyFiltersOnNonDbCollectionWithNoFiltersDoesNotThrow(): void + { + $collection = $this->createNonDbCollection(); + $criteria = $this->searchCriteriaFactory->create(); + $criteria->setFilterGroups([]); + + $this->filterPool->applyFilters($collection, $criteria); + + $this->assertCount(2, $collection->getItems()); + } + + /** + * Filters on non-database collections are applied via addFieldToFilter (RegularFilter). + */ + public function testApplyFiltersOnNonDbCollectionAppliesFieldFilters(): void + { + $collection = $this->createNonDbCollection(); + $this->assertCount(2, $collection->getItems()); + + $filter = $this->filterBuilder + ->setField('sku') + ->setValue('SKU-1') + ->setConditionType('eq') + ->create(); + $filterGroup = $this->filterGroupBuilder->setFilters([$filter])->create(); + $criteria = $this->searchCriteriaFactory->create(); + $criteria->setFilterGroups([$filterGroup]); + + $this->filterPool->applyFilters($collection, $criteria); + + $items = $collection->getItems(); + $this->assertCount(1, $items); + $item = current($items); + $this->assertSame('SKU-1', $item->getData('sku')); + } + + /** + * Multiple filters on a non-database collection are all applied (AND via sequential filtering). + */ + public function testApplyFiltersOnNonDbCollectionAppliesMultipleFilters(): void + { + $collection = $this->createNonDbCollection([ + ['sku' => 'A', 'status' => 1], + ['sku' => 'B', 'status' => 1], + ['sku' => 'A', 'status' => 0], + ]); + + $skuFilter = $this->filterBuilder + ->setField('sku') + ->setValue('A') + ->setConditionType('eq') + ->create(); + $statusFilter = $this->filterBuilder + ->setField('status') + ->setValue(1) + ->setConditionType('eq') + ->create(); + + $filterGroups = [ + $this->filterGroupBuilder->setFilters([$skuFilter])->create(), + $this->filterGroupBuilder->setFilters([$statusFilter])->create(), + ]; + $criteria = $this->searchCriteriaFactory->create(); + $criteria->setFilterGroups($filterGroups); + + $this->filterPool->applyFilters($collection, $criteria); + + $items = $collection->getItems(); + $this->assertCount(1, $items); + $item = current($items); + $this->assertSame('A', $item->getData('sku')); + $this->assertSame(1, $item->getData('status')); + } + + /** + * Build an in-memory collection that supports addFieldToFilter (eq only). + * + * @param array $rows + * @return Collection + */ + private function createNonDbCollection(array $rows = []): Collection + { + if ($rows === []) { + $rows = [ + ['sku' => 'SKU-1', 'name' => 'First'], + ['sku' => 'SKU-2', 'name' => 'Second'], + ]; + } + + $collection = new class ($this->entityFactory) extends Collection { + /** + * @var array + */ + private $appliedFilters = []; + + /** + * @inheritdoc + */ + public function addFieldToFilter($field, $condition = null) + { + $this->appliedFilters[] = [$field, $condition]; + return $this; + } + + /** + * @inheritdoc + */ + public function loadData($printQuery = false, $logQuery = false) + { + return $this; + } + + /** + * Filter loaded items in memory using recorded eq filters. + * + * @inheritdoc + */ + public function getItems() + { + $items = parent::getItems(); + foreach ($this->appliedFilters as [$field, $condition]) { + if (!is_array($condition)) { + continue; + } + foreach ($condition as $type => $value) { + if ($type !== 'eq') { + continue; + } + $items = array_filter( + $items, + static function (DataObject $item) use ($field, $value) { + return (string) $item->getData($field) === (string) $value; + } + ); + } + } + return $items; + } + }; + + foreach ($rows as $row) { + $collection->addItem(new DataObject($row)); + } + + return $collection; + } +} diff --git a/lib/internal/Magento/Framework/View/Element/UiComponent/DataProvider/FilterPool.php b/lib/internal/Magento/Framework/View/Element/UiComponent/DataProvider/FilterPool.php index a8dd8eee55c2b..8e6d9105bc669 100644 --- a/lib/internal/Magento/Framework/View/Element/UiComponent/DataProvider/FilterPool.php +++ b/lib/internal/Magento/Framework/View/Element/UiComponent/DataProvider/FilterPool.php @@ -7,8 +7,8 @@ namespace Magento\Framework\View\Element\UiComponent\DataProvider; -use Magento\Framework\Data\Collection; use Magento\Framework\Api\Search\SearchCriteriaInterface; +use Magento\Framework\Data\Collection; use Magento\Framework\Data\Collection\AbstractDb; use Magento\Framework\DB\Select; @@ -20,33 +20,67 @@ */ class FilterPool { - /** - * @var FilterApplierInterface[] - */ - protected $appliers; - /** * @param FilterApplierInterface[] $appliers */ - public function __construct(array $appliers = []) + public function __construct(protected array $appliers = []) { - $this->appliers = $appliers; } /** - * Apply filters from search criteria + * Apply filters from search criteria. * * @param Collection|AbstractDb $collection * @param SearchCriteriaInterface $criteria * @return void + * @throws \Zend_Db_Select_Exception */ public function applyFilters(Collection $collection, SearchCriteriaInterface $criteria) { + // MC-24195 + if (!$collection instanceof AbstractDb) { + $this->applyFiltersToCollection($collection, $criteria); + return; + } + + $this->applyFiltersToDbCollection($collection, $criteria); + } + + /** + * Apply filters without touching Select (non-database collections). + * + * @param Collection $collection + * @param SearchCriteriaInterface $criteria + * @return void + */ + private function applyFiltersToCollection( + Collection $collection, + SearchCriteriaInterface $criteria + ): void { + foreach ($criteria->getFilterGroups() as $filterGroup) { + foreach ($filterGroup->getFilters() as $filter) { + $this->getApplier($filter->getConditionType())->apply($collection, $filter); + } + } + } + + /** + * Apply filters and regroup WHERE so OR applies within each filter group. + * + * @param AbstractDb $collection + * @param SearchCriteriaInterface $criteria + * @return void + * @throws \Zend_Db_Select_Exception + */ + private function applyFiltersToDbCollection( + AbstractDb $collection, + SearchCriteriaInterface $criteria + ): void { $groupedParts = $collection->getSelect()->getPart(Select::WHERE); foreach ($criteria->getFilterGroups() as $filterGroup) { $filterParts = []; foreach ($filterGroup->getFilters() as $filter) { - $filterApplier = $this->appliers[$filter->getConditionType()] ?? $this->appliers['regular']; + $filterApplier = $this->getApplier($filter->getConditionType()); $filterApplier->apply($collection, $filter); $whereParts = $collection->getSelect()->getPart(Select::WHERE); if (is_array($whereParts) && count($whereParts)) { @@ -70,7 +104,18 @@ public function applyFilters(Collection $collection, SearchCriteriaInterface $cr } /** - * Remove were join condition in the beginning of applied filter + * Resolve filter applier for the given condition type. + * + * @param string|null $conditionType + * @return FilterApplierInterface + */ + private function getApplier(?string $conditionType): FilterApplierInterface + { + return $this->appliers[$conditionType] ?? $this->appliers['regular']; + } + + /** + * Remove where join condition in the beginning of applied filter * * @param string $part * @return string diff --git a/lib/internal/Magento/Framework/View/Test/Unit/Element/UiComponent/DataProvider/FilterPoolTest.php b/lib/internal/Magento/Framework/View/Test/Unit/Element/UiComponent/DataProvider/FilterPoolTest.php new file mode 100644 index 0000000000000..3124bc64815f7 --- /dev/null +++ b/lib/internal/Magento/Framework/View/Test/Unit/Element/UiComponent/DataProvider/FilterPoolTest.php @@ -0,0 +1,227 @@ +regularApplier = $this->createMock(FilterApplierInterface::class); + $this->filterPool = new FilterPool(['regular' => $this->regularApplier]); + } + + /** + * Non-AbstractDb collections must not call getSelect() (GitHub #32292). + */ + public function testApplyFiltersOnNonDbCollectionWithEmptyFilterGroupsDoesNotCallGetSelect(): void + { + $criteria = $this->createMock(SearchCriteriaInterface::class); + $criteria->method('getFilterGroups')->willReturn([]); + + $collection = $this->getMockBuilder(Collection::class) + ->disableOriginalConstructor() + ->onlyMethods(['addFieldToFilter']) + ->getMock(); + + $this->regularApplier->expects($this->never())->method('apply'); + + $this->filterPool->applyFilters($collection, $criteria); + $this->addToAssertionCount(1); + } + + /** + * Filters on non-AbstractDb collections are delegated to appliers only. + */ + public function testApplyFiltersOnNonDbCollectionInvokesAppliers(): void + { + $filterA = $this->createFilterMock('eq', 'sku', 'ABC'); + $filterB = $this->createFilterMock('like', 'name', '%test%'); + + $group = $this->createMock(FilterGroup::class); + $group->method('getFilters')->willReturn([$filterA, $filterB]); + + $criteria = $this->createMock(SearchCriteriaInterface::class); + $criteria->method('getFilterGroups')->willReturn([$group]); + + $collection = $this->getMockBuilder(Collection::class) + ->disableOriginalConstructor() + ->onlyMethods(['addFieldToFilter']) + ->getMock(); + + $applied = []; + $this->regularApplier->expects($this->exactly(2)) + ->method('apply') + ->willReturnCallback(function ($coll, $filter) use ($collection, &$applied) { + $this->assertSame($collection, $coll); + $applied[] = $filter; + }); + + $this->filterPool->applyFilters($collection, $criteria); + + $this->assertSame([$filterA, $filterB], $applied); + } + + /** + * Condition-type-specific appliers are preferred over the regular applier. + */ + public function testApplyFiltersOnNonDbCollectionUsesConditionSpecificApplier(): void + { + $fulltextApplier = $this->createMock(FilterApplierInterface::class); + $filterPool = new FilterPool([ + 'regular' => $this->regularApplier, + 'fulltext' => $fulltextApplier, + ]); + + $filter = $this->createFilterMock('fulltext', 'query', 'search term'); + $group = $this->createMock(FilterGroup::class); + $group->method('getFilters')->willReturn([$filter]); + + $criteria = $this->createMock(SearchCriteriaInterface::class); + $criteria->method('getFilterGroups')->willReturn([$group]); + + $collection = $this->getMockBuilder(Collection::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->regularApplier->expects($this->never())->method('apply'); + $fulltextApplier->expects($this->once()) + ->method('apply') + ->with($collection, $filter); + + $filterPool->applyFilters($collection, $criteria); + } + + /** + * AbstractDb path still captures and rewrites Select WHERE parts for OR groups. + */ + public function testApplyFiltersOnDbCollectionRegroupsWherePartsWithOr(): void + { + $whereState = []; + $select = $this->createMock(Select::class); + $select->method('getPart') + ->with(Select::WHERE) + ->willReturnCallback(static function () use (&$whereState) { + return $whereState; + }); + $select->method('reset') + ->with(Select::WHERE) + ->willReturnCallback(static function () use (&$whereState) { + $whereState = []; + }); + $select->method('setPart') + ->willReturnCallback(static function ($part, $value) use (&$whereState) { + if ($part === Select::WHERE) { + $whereState = $value; + } + }); + + /** @var AbstractDb&MockObject $collection */ + $collection = $this->createPartialMockWithReflection( + AbstractDb::class, + ['getSelect', 'getResource'] + ); + $collection->method('getSelect')->willReturn($select); + + $filterA = $this->createFilterMock('eq', 'email', '%1%'); + $filterB = $this->createFilterMock('eq', 'email', '%2%'); + + $group = $this->createMock(FilterGroup::class); + $group->method('getFilters')->willReturn([$filterA, $filterB]); + + $criteria = $this->createMock(SearchCriteriaInterface::class); + $criteria->method('getFilterGroups')->willReturn([$group]); + + $call = 0; + $this->regularApplier->expects($this->exactly(2)) + ->method('apply') + ->willReturnCallback(function () use (&$whereState, &$call) { + $call++; + // Simulate RegularFilter / addFieldToFilter appending a WHERE part + $whereState[] = ($call === 1 ? '' : 'AND ') . "email LIKE '%{$call}%'"; + }); + + $this->filterPool->applyFilters($collection, $criteria); + + $this->assertNotEmpty($whereState); + $combined = implode(' ', $whereState); + $this->assertStringContainsString(Select::SQL_OR, $combined); + $this->assertStringContainsString("email LIKE '%1%'", $combined); + $this->assertStringContainsString("email LIKE '%2%'", $combined); + } + + /** + * Empty filter groups on AbstractDb must not error. + */ + public function testApplyFiltersOnDbCollectionWithEmptyFilterGroups(): void + { + $select = $this->createMock(Select::class); + $select->method('getPart')->with(Select::WHERE)->willReturn([]); + $select->expects($this->never())->method('setPart'); + + /** @var AbstractDb&MockObject $collection */ + $collection = $this->createPartialMockWithReflection( + AbstractDb::class, + ['getSelect', 'getResource'] + ); + $collection->method('getSelect')->willReturn($select); + + $criteria = $this->createMock(SearchCriteriaInterface::class); + $criteria->method('getFilterGroups')->willReturn([]); + + $this->regularApplier->expects($this->never())->method('apply'); + $this->filterPool->applyFilters($collection, $criteria); + $this->addToAssertionCount(1); + } + + /** + * @param string $conditionType + * @param string $field + * @param string $value + * @return Filter&MockObject + */ + private function createFilterMock(string $conditionType, string $field, string $value): Filter + { + $filter = $this->createMock(Filter::class); + $filter->method('getConditionType')->willReturn($conditionType); + $filter->method('getField')->willReturn($field); + $filter->method('getValue')->willReturn($value); + return $filter; + } +}