Skip to content

Commit 6efccf5

Browse files
mhvismhvis
authored
Fixes and changes for column filters (#369)
* Remove operator option on AbstractFilter because it is nowhere used * AbstractFilter: get rid of $this->$key This line is risky and inflexible as it implicitely requires each option to have a property on the child class. * For searchColumns, check isSearchable() in getter instead of before set This fixes a bug which is demonstrated in the test cases included in this commit. This is not the most elegant solution, let me know if you have something better. * Make column search behaviour the same as for global search This is the default behaviour that I would expect for a column search.
1 parent 2952f8c commit 6efccf5

File tree

9 files changed

+104
-41
lines changed

9 files changed

+104
-41
lines changed

‎src/Adapter/Doctrine/ORM/SearchCriteriaProvider.php‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
namespace Omines\DataTablesBundle\Adapter\Doctrine\ORM;
1414

15+
use Doctrine\ORM\Query\Expr;
1516
use Doctrine\ORM\Query\Expr\Comparison;
1617
use Doctrine\ORM\QueryBuilder;
1718
use Omines\DataTablesBundle\Column\AbstractColumn;
@@ -43,8 +44,7 @@ private function processSearchColumns(QueryBuilder $queryBuilder, DataTableState
4344
continue;
4445
}
4546
}
46-
$search = $queryBuilder->expr()->literal($search);
47-
$queryBuilder->andWhere(new Comparison($column->getField(), $column->getOperator(), $search));
47+
$queryBuilder->andWhere($this->getSearchComparison($column, $search));
4848
}
4949
}
5050
}
@@ -56,11 +56,19 @@ private function processGlobalSearch(QueryBuilder $queryBuilder, DataTableState
5656
$comparisons = $expr->orX();
5757
foreach ($state->getDataTable()->getColumns() as $column) {
5858
if ($column->isGlobalSearchable() && !empty($column->getField()) && $column->isValidForSearch($globalSearch)) {
59-
$comparisons->add(new Comparison($column->getLeftExpr(), $column->getOperator(),
60-
$expr->literal($column->getRightExpr($globalSearch))));
59+
$comparisons->add($this->getSearchComparison($column, $globalSearch));
6160
}
6261
}
6362
$queryBuilder->andWhere($comparisons);
6463
}
6564
}
65+
66+
private function getSearchComparison(AbstractColumn $column, string $search): Comparison
67+
{
68+
return new Comparison(
69+
$column->getLeftExpr(),
70+
$column->getOperator(),
71+
(new Expr())->literal($column->getRightExpr($search)),
72+
);
73+
}
6674
}

‎src/DataTableState.php‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,10 @@ private function handleSearch(ParameterBag $parameters): void
104104
$column = $this->dataTable->getColumn((int) $key);
105105
$value = $this->isInitial ? $search : $search['search']['value'] ?? '';
106106

107-
if ($column->isSearchable() && ('' !== mb_trim($value))) {
107+
// We do not check for $column->isSearchable() here, because at this point the
108+
// field option may not have been set yet. This makes the check for isSearchable()
109+
// unreliable.
110+
if ('' !== mb_trim($value)) {
108111
$this->setColumnSearch($column, $value);
109112
}
110113
}
@@ -204,11 +207,13 @@ public function setOrderBy(array $orderBy = []): static
204207
/**
205208
* Returns an array of column-level searches.
206209
*
210+
* @param bool $onlySearchable if true, only returns columns for which isSearchable() is true
207211
* @return SearchColumn[]
208212
*/
209-
public function getSearchColumns(): array
213+
public function getSearchColumns(bool $onlySearchable = true): array
210214
{
211-
return $this->searchColumns;
215+
// `searchColumns` may include columns that are not searchable, so we filter them out here.
216+
return array_filter($this->searchColumns, fn ($searchInfo) => !$onlySearchable || $searchInfo['column']->isSearchable());
212217
}
213218

214219
public function setColumnSearch(AbstractColumn $column, string $search, bool $isRegex = false): static

‎src/Filter/AbstractFilter.php‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -16,47 +16,47 @@
1616

1717
abstract class AbstractFilter
1818
{
19-
protected string $template_html;
20-
protected string $template_js;
21-
protected string $operator;
19+
/**
20+
* @var array<string, mixed>
21+
*/
22+
protected array $options = [];
23+
24+
public function __construct()
25+
{
26+
// Initialize the options with the default values set on the OptionsResolver
27+
$this->set([]);
28+
}
2229

2330
/**
2431
* @param array<string, mixed> $options
2532
*/
26-
public function set(array $options): void
33+
public function set(array $options): static
2734
{
2835
$resolver = new OptionsResolver();
2936
$this->configureOptions($resolver);
37+
$this->options = $resolver->resolve($options);
3038

31-
foreach ($resolver->resolve($options) as $key => $value) {
32-
$this->$key = $value;
33-
}
39+
return $this;
3440
}
3541

3642
protected function configureOptions(OptionsResolver $resolver): static
3743
{
38-
$resolver->setDefaults([
39-
'template_html' => null,
40-
'template_js' => null,
41-
'operator' => 'CONTAINS',
44+
$resolver->setRequired([
45+
'template_html',
46+
'template_js',
4247
]);
4348

4449
return $this;
4550
}
4651

4752
public function getTemplateHtml(): string
4853
{
49-
return $this->template_html;
54+
return $this->options['template_html'];
5055
}
5156

5257
public function getTemplateJs(): string
5358
{
54-
return $this->template_js;
55-
}
56-
57-
public function getOperator(): string
58-
{
59-
return $this->operator;
59+
return $this->options['template_js'];
6060
}
6161

6262
abstract public function isValidValue(mixed $value): bool;

‎src/Filter/ChoiceFilter.php‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,6 @@
1616

1717
class ChoiceFilter extends AbstractFilter
1818
{
19-
protected ?string $placeholder = null;
20-
21-
/** @var array<string, string> */
22-
protected array $choices = [];
23-
2419
protected function configureOptions(OptionsResolver $resolver): static
2520
{
2621
parent::configureOptions($resolver);
@@ -40,19 +35,19 @@ protected function configureOptions(OptionsResolver $resolver): static
4035

4136
public function getPlaceholder(): ?string
4237
{
43-
return $this->placeholder;
38+
return $this->options['placeholder'];
4439
}
4540

4641
/**
4742
* @return string[]
4843
*/
4944
public function getChoices(): array
5045
{
51-
return $this->choices;
46+
return $this->options['choices'];
5247
}
5348

5449
public function isValidValue(mixed $value): bool
5550
{
56-
return array_key_exists($value, $this->choices);
51+
return array_key_exists($value, $this->getChoices());
5752
}
5853
}

‎src/Filter/TextFilter.php‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,6 @@
1616

1717
class TextFilter extends AbstractFilter
1818
{
19-
protected ?string $placeholder = null;
20-
2119
protected function configureOptions(OptionsResolver $resolver): static
2220
{
2321
parent::configureOptions($resolver);
@@ -35,7 +33,7 @@ protected function configureOptions(OptionsResolver $resolver): static
3533

3634
public function getPlaceholder(): ?string
3735
{
38-
return $this->placeholder;
36+
return $this->options['placeholder'];
3937
}
4038

4139
public function isValidValue(mixed $value): bool

‎tests/Functional/FunctionalTest.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,17 @@ public function testServiceDataTable(): void
102102
$this->assertCount(2, $json->data);
103103
$this->assertStringStartsWith('Company ', $json->data[0]->company);
104104
$this->assertSame('LastName24 (Company 4)', $json->data[0]->fullName);
105+
106+
// A column search should be based on substring matching by default (same as global
107+
// search). Not on exact matching.
108+
$json = $this->callDataTableUrl('/service?_dt=persons&draw=2&order[0][column]=2&order[0][dir]=desc&columns[1][search][value]=name24');
109+
$this->assertCount(1, $json->data);
110+
$this->assertSame('LastName24 (Company 4)', $json->data[0]->fullName);
111+
112+
// Search for `LastName1` in the first name column should return no results. This
113+
// tests that the column search only applies to the specified column.
114+
$json = $this->callDataTableUrl('/service?_dt=persons&draw=2&order[0][column]=2&order[0][dir]=desc&columns[1][search][value]=LastName1');
115+
$this->assertCount(0, $json->data);
105116
}
106117

107118
public function testCustomDataTable(): void

‎tests/Unit/Adapter/ORMAdapterTest.php‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,9 @@
1919
use Omines\DataTablesBundle\DataTableFactory;
2020
use Omines\DataTablesBundle\DataTableState;
2121
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
22+
use Symfony\Component\HttpFoundation\Request;
2223
use Tests\Fixtures\AppBundle\DataTable\Type\GroupedTableType;
24+
use Tests\Fixtures\AppBundle\DataTable\Type\ServicePersonTableType;
2325

2426
class ORMAdapterTest extends KernelTestCase
2527
{
@@ -44,6 +46,32 @@ public function testCountGroupedDataTable(): void
4446
iterator_to_array($data->getData()); // Evaluate the iterator to trigger the exception
4547
}
4648

49+
/**
50+
* Tests that column searches are applied when `field` is set by ORMAdapter.
51+
*
52+
* When `field` and `searchable` are not set manually, isSearchable() will only
53+
* return true after the `field` option is set by ORMAdapter. This tests that the
54+
* column search still is applied correctly.
55+
*/
56+
public function testColumnSearch(): void
57+
{
58+
$datatable = $this->factory->createFromType(ServicePersonTableType::class)
59+
->setMethod(Request::METHOD_GET);
60+
$datatable->handleRequest(Request::create('/?_dt=' . $datatable->getName() . '&columns[1][search][value]=John'));
61+
62+
// At this point, $searchColumns is empty because isSearchable() returns false due
63+
// to `field` not being set yet. Ideally it should contain the search value.
64+
$searchColumns = $datatable->getState()->getSearchColumns();
65+
$this->assertCount(0, $searchColumns);
66+
67+
// After calling getData(), the column search should be correctly returned.
68+
$datatable->getAdapter()->getData($datatable->getState());
69+
70+
$searchColumns = $datatable->getState()->getSearchColumns();
71+
$this->assertCount(1, $searchColumns);
72+
$this->assertSame('John', $searchColumns['firstName']['search']);
73+
}
74+
4775
public function testORMAdapterQueryEvent(): void
4876
{
4977
$query = $this->createMock(Query::class);

‎tests/Unit/DataTableTest.php‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ public function testDataTableState(): void
9898
$this->assertSame(10, $state->getLength());
9999
$this->assertSame('foo', $state->getGlobalSearch());
100100
$this->assertCount(2, $state->getOrderBy());
101-
$this->assertSame('bar', $state->getSearchColumns()['foo']['search']);
101+
$this->assertSame('bar', $state->getSearchColumns(onlySearchable: false)['foo']['search']);
102102

103103
// Test boundaries
104104
$state->setStart(-1);
@@ -111,6 +111,28 @@ public function testDataTableState(): void
111111
$this->assertSame($state, $column->getState());
112112
}
113113

114+
/**
115+
* Tests that getSearchColumns only returns columns for which `isSearchable()` is true.
116+
*/
117+
public function testDataTableStateSearchColumns(): void
118+
{
119+
$datatable = $this
120+
->createMockDataTable()
121+
->add('foo', TextColumn::class, ['searchable' => true])
122+
->add('bar', TextColumn::class, ['searchable' => false])
123+
->setMethod(Request::METHOD_GET)
124+
;
125+
$datatable->handleRequest(Request::create('/?_dt=' . $datatable->getName()));
126+
127+
$state = $datatable->getState();
128+
$state->setColumnSearch($datatable->getColumn(0), 'foo');
129+
$state->setColumnSearch($datatable->getColumn(1), 'bar');
130+
131+
$searchColumns = $state->getSearchColumns();
132+
$this->assertCount(1, $searchColumns);
133+
$this->assertSame('foo', $searchColumns['foo']['search']);
134+
}
135+
114136
public function testPostMethod(): void
115137
{
116138
$datatable = $this->createMockDataTable();

‎tests/Unit/FilterTest.php‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ public function testChoiceFilter(): void
3333

3434
$filter->set([
3535
'choices' => ['foo' => 'bar', 'bar' => 'baz'],
36-
'operator' => 'bar',
3736
'placeholder' => 'foobar',
3837
'template_html' => 'foobar.html',
3938
]);
@@ -44,7 +43,6 @@ public function testChoiceFilter(): void
4443
$this->assertSame('foobar', $filter->getPlaceholder());
4544
$this->assertSame('foobar.html', $filter->getTemplateHtml());
4645
$this->assertSame('@DataTables/Filter/select.js.twig', $filter->getTemplateJs());
47-
$this->assertSame('bar', $filter->getOperator());
4846
}
4947

5048
public function testTextFilter(): void
@@ -57,12 +55,10 @@ public function testTextFilter(): void
5755

5856
$filter->set([
5957
'template_js' => 'foobar.js',
60-
'operator' => 'foo',
6158
'placeholder' => 'baz',
6259
]);
6360
$this->assertSame('@DataTables/Filter/text.html.twig', $filter->getTemplateHtml());
6461
$this->assertSame('foobar.js', $filter->getTemplateJs());
65-
$this->assertSame('foo', $filter->getOperator());
6662
$this->assertSame('baz', $filter->getPlaceholder());
6763
}
6864
}

0 commit comments

Comments
 (0)