From 17675cc44358fd2ffc113d7d785a9e6b26b10561 Mon Sep 17 00:00:00 2001 From: Sukhwinder Dhillon Date: Mon, 20 Jan 2025 10:51:19 +0100 Subject: [PATCH 1/3] Properly apply given default sort CompatController::createSortControl(): - Set Default before assembling to set the correct value for the sort dropdown. - Throw error if 3rd param is given but is not present in $columns SortControl::apply(): - Remove unused param and code. --- src/Compat/CompatController.php | 19 +++++++++++++++---- src/Control/SortControl.php | 8 +------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/Compat/CompatController.php b/src/Compat/CompatController.php index f4c2fb0b..735d0da9 100644 --- a/src/Compat/CompatController.php +++ b/src/Compat/CompatController.php @@ -10,6 +10,7 @@ use ipl\Html\HtmlDocument; use ipl\Html\HtmlString; use ipl\Html\ValidHtml; +use ipl\Orm\Common\SortUtil; use ipl\Orm\Query; use ipl\Stdlib\Contract\Paginatable; use ipl\Web\Control\LimitControl; @@ -294,15 +295,25 @@ public function createSortControl(Query $query, array $columns): SortControl $this->params->shift($sortControl->getSortParam()); - $sortControl->handleRequest($this->getServerRequest()); - $defaultSort = null; - if (func_num_args() === 3) { $defaultSort = func_get_args()[2]; } - return $sortControl->apply($query, $defaultSort); + $default = $defaultSort ?? $query->getModel()->getDefaultSort(); + if (! empty($default)) { + $sortControl->setDefault(SortUtil::normalizeSortSpec($default)); + + $columns = $sortControl->getColumns(); + $default = $sortControl->getDefault(); + if (! empty($defaultSort) && ! isset($columns[$default])) { + throw new InvalidArgumentException(sprintf('Invalid default sort "%s" given', $default)); + } + } + + $sortControl->handleRequest($this->getServerRequest()); + + return $sortControl->apply($query); } /** diff --git a/src/Control/SortControl.php b/src/Control/SortControl.php index 65c2c3d7..6b06c6c4 100644 --- a/src/Control/SortControl.php +++ b/src/Control/SortControl.php @@ -197,11 +197,10 @@ public function getSort(): ?string * Sort the given query according to the request * * @param Query $query - * @param ?array|string $defaultSort * * @return $this */ - public function apply(Query $query, $defaultSort = null): self + public function apply(Query $query): self { if ($this->getRequest() === null) { // handleRequest() has not been called yet @@ -209,11 +208,6 @@ public function apply(Query $query, $defaultSort = null): self $this->handleRequest(ServerRequest::fromGlobals()); } - $default = $defaultSort ?? (array) $query->getModel()->getDefaultSort(); - if (! empty($default)) { - $this->setDefault(SortUtil::normalizeSortSpec($default)); - } - $sort = $this->getSort(); if (! empty($sort)) { $query->orderBy(SortUtil::createOrderBy($sort)); From 71644a14f3885c64a4c7e46eab07b2860599c57b Mon Sep 17 00:00:00 2001 From: Sukhwinder Dhillon Date: Wed, 22 Jan 2025 10:58:55 +0100 Subject: [PATCH 2/3] Add tests for `CompatController::createSortControl()` --- tests/CompatControllerTest.php | 120 +++++++++++++++++++++++++++++++++ tests/SortControl.php | 71 +++++++++++++++++++ 2 files changed, 191 insertions(+) create mode 100644 tests/CompatControllerTest.php create mode 100644 tests/SortControl.php diff --git a/tests/CompatControllerTest.php b/tests/CompatControllerTest.php new file mode 100644 index 00000000..0bd01ab9 --- /dev/null +++ b/tests/CompatControllerTest.php @@ -0,0 +1,120 @@ +controller = new class extends CompatController { + + protected $params; + + public function __construct( + Zend_Controller_Request_Abstract $request = null, + Zend_Controller_Response_Abstract $response = null, + array $invokeArgs = [] + ) { + $this->params = new UrlParams(); + } + }; + + $self = (new self()); + $model = $self->createMock(Model::class); + $model->method('getDefaultSort')->willReturn(['age']); + + $this->query = $self->createMock(Query::class); + $this->query->method('getModel')->willReturn($model); + } + + public function testCreateSortControlUsesDefaultSortFromModel(): void + { + $sortControl = $this->controller->createSortControl( + $this->query, + [ + 'name' => 'Name', + 'age' => 'Age', + 'city' => 'City' + ] + ); + + $this->assertSame('age', $sortControl->getDefault()); + } + + public function testCreateSortControlUsesDefaultSortFromModelWhichIsNotPresentInProvidedColumns(): void + { + $sortControl = $this->controller->createSortControl( + $this->query, + [ + 'name' => 'Name', + 'surname' => 'Surname', + 'city' => 'City' + ] + ); + + $this->assertSame('age', $sortControl->getDefault()); + } + + public function testCreateSortControlUsesProvidedThirdParamAsString(): void + { + $sortControl = $this->controller->createSortControl( + $this->query, + [ + 'name' => 'Name', + 'age' => 'age', + 'city' => 'City' + ], + 'city' + ); + + $this->assertSame('city', $sortControl->getDefault()); + } + + public function testCreateSortControlUsesProvidedThirdParamAsArray(): void + { + $sortControl = $this->controller->createSortControl( + $this->query, + [ + 'name' => 'Name', + 'age' => 'age', + 'city' => 'City' + ], + ['city'] + ); + + $this->assertSame('city', $sortControl->getDefault()); + } + + public function testCreateSortControlThrowsExceptionWhenProvidedThirdParamIsNotPresentInProvidedColumns(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Invalid default sort "car" given'); + + $this->controller->createSortControl( + $this->query, + [ + 'name' => 'Name', + 'age' => 'age', + 'city' => 'City' + ], + ['car'] + ); + } +} diff --git a/tests/SortControl.php b/tests/SortControl.php new file mode 100644 index 00000000..6b7c36e6 --- /dev/null +++ b/tests/SortControl.php @@ -0,0 +1,71 @@ +setColumns($columns); + } + + public static function create(array $options) + { + $normalized = []; + foreach ($options as $spec => $label) { + $normalized[SortUtil::normalizeSortSpec($spec)] = $label; + } + + return new static($normalized); + } + + public function setColumns($columns) + { + $this->columns = array_change_key_case($columns, CASE_LOWER); + + return $this; + } + + public function getColumns() + { + return $this->columns; + } + + public function getDefault(): ?string + { + return $this->default; + } + + public function setDefault(string $default): self + { + // We're working with lowercase keys throughout the sort control + $this->default = strtolower($default); + + return $this; + } + + public function getSortParam() + { + return ''; + } + + public function handleRequest() + { + } + + public function apply() + { + return $this; + } +} From bca6fa5e768ab654a5e0a7f9182a23bbe4b6c5e6 Mon Sep 17 00:00:00 2001 From: Sukhwinder Dhillon Date: Wed, 22 Jan 2025 11:56:29 +0100 Subject: [PATCH 3/3] CompatControllerTest: Skip tests on github because icingaweb is not present --- tests/CompatControllerTest.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/CompatControllerTest.php b/tests/CompatControllerTest.php index 0bd01ab9..456aead5 100644 --- a/tests/CompatControllerTest.php +++ b/tests/CompatControllerTest.php @@ -11,7 +11,7 @@ use Zend_Controller_Request_Abstract; /** - * @runTestsInSeparateProcesses + * @aarunTestsInSeparateProcesses */ class CompatControllerTest extends TestCase { @@ -21,6 +21,10 @@ class CompatControllerTest extends TestCase public function setUp(): void { + if (! class_exists('\Icinga\Web\Controller')) { + $this->markTestSkipped('CompatControllerTest only runs locally'); + } + class_alias('ipl\Tests\Web\SortControl', 'ipl\Web\Control\SortControl'); $this->controller = new class extends CompatController {