Rework

- Drop unused DateTime
- Small refactorings & improvements

Author: johanib

ci/qa/phpstan-baseline.php

@@ -715,6 +715,12 @@
 	'count' => 1,
 	'path' => __DIR__ . '/../../src/Surfnet/StepupRa/RaBundle/Service/RaCandidateService.php',
 ];
+$ignoreErrors[] = [
+	'message' => '#^Left side of && is always true\\.$#',
+	'identifier' => 'booleanAnd.leftAlwaysTrue',
+	'count' => 2,
+	'path' => __DIR__ . '/../../src/Surfnet/StepupRa/RaBundle/Service/RaListingService.php',
+];
 $ignoreErrors[] = [
 	'message' => '#^Parameter \\#1 \\$raInstitution of method Surfnet\\\\StepupMiddlewareClient\\\\Identity\\\\Dto\\\\RaListingSearchQuery\\:\\:setRaInstitution\\(\\) expects string, string\\|null given\\.$#',
 	'identifier' => 'argument.type',
@@ -847,6 +853,12 @@
 	'count' => 1,
 	'path' => __DIR__ . '/../../src/Surfnet/StepupRa/RaBundle/Twig/Extensions/Extension/SecondFactorType.php',
 ];
+$ignoreErrors[] = [
+	'message' => '#^Call to function is_int\\(\\) with int will always evaluate to true\\.$#',
+	'identifier' => 'function.alreadyNarrowedType',
+	'count' => 1,
+	'path' => __DIR__ . '/../../src/Surfnet/StepupRa/RaBundle/Value/TimeFrame.php',
+];
 $ignoreErrors[] = [
 	'message' => '#^Class Surfnet\\\\StepupRa\\\\RaBundle\\\\VettingProcedure has an uninitialized property \\$authorityId\\. Give it default value or assign it in the constructor\\.$#',
 	'identifier' => 'property.uninitialized',

ci/qa/phpstan.neon

@@ -4,7 +4,6 @@ includes:
 rules:
 
 parameters:
-    treatPhpDocTypesAsCertain: false
     checkUninitializedProperties: true
     level: 9
     paths:

src/Surfnet/StepupRa/RaBundle/Controller/RaLocationController.php

@@ -66,7 +66,7 @@ public function __construct(
     #[IsGranted('ROLE_RA')]
     public function manage(Request $request): Response
     {
-        $institutionParameter = $request->query->has('institution') ? $request->query->get('institution') : $request->request->get('institution');
+        $institutionParameter = $request->query->get('institution') ?? $request->request->get('institution');
 
         $identity = $this->getUser()->getIdentity();
         $this->logger->notice('Starting search for locations');
@@ -101,8 +101,8 @@ public function manage(Request $request): Response
 
         $command = new SearchRaLocationsCommand();
         $command->institution = $institution;
-        $command->orderBy = $this->getString($request, 'orderBy');
-        $command->orderDirection = $this->getString($request, 'orderDirection');
+        $command->orderBy = $this->getOrderBy($request);
+        $command->orderDirection = $this->getOrderDirection($request);
 
         $locations = $this->raLocationService->search($command);
 

src/Surfnet/StepupRa/RaBundle/Controller/RaManagementController.php

@@ -135,8 +135,8 @@ public function raCandidateSearch(Request $request): Response
         $command->actorInstitution = $institution;
         $command->raInstitution    = null;
         $command->pageNumber = $request->query->has('p') ? $request->query->getInt('p') : $request->request->getInt('p', 1);
-        $command->orderBy = $this->getString($request, 'orderBy');
-        $command->orderDirection = $this->getString($request, 'orderDirection');
+        $command->orderBy = $this->getOrderBy($request);
+        $command->orderDirection = $this->getOrderDirection($request);
 
         $raCandidateList = $this->raCandidateService->search($command);
 

src/Surfnet/StepupRa/RaBundle/Controller/RecoveryTokenController.php

@@ -58,8 +58,8 @@ public function search(Request $request): Response
         $command = new SearchRecoveryTokensCommand();
         $command->actorId = $identity->id;
         $command->pageNumber = $request->query->has('p') ? $request->query->getInt('p') : $request->request->getInt('p', 1);
-        $command->orderBy = $this->getString($request, 'orderBy');
-        $command->orderDirection = $this->getString($request, 'orderDirection');
+        $command->orderBy = $this->getOrderBy($request);
+        $command->orderDirection = $this->getOrderDirection($request);
 
         // Huh, why do we load the recovery tokens at this point?
         // This is just to get the filter options for the available institutions of the search results.

src/Surfnet/StepupRa/RaBundle/Controller/SecondFactorController.php

@@ -73,8 +73,8 @@ public function search(Request $request): Response
         $command = new SearchRaSecondFactorsCommand();
         $command->actorId = $identity->id;
         $command->pageNumber = $request->query->has('p') ? $request->query->getInt('p') : $request->request->getInt('p', 1);
-        $command->orderBy = $this->getString($request, 'orderBy');
-        $command->orderDirection = $this->getString($request, 'orderDirection');
+        $command->orderBy = $this->getOrderBy($request);
+        $command->orderDirection = $this->getOrderDirection($request);
 
         $secondFactors = $this->secondFactorService->search($command);
 

src/Surfnet/StepupRa/RaBundle/Controller/Traits/OrderFromRequest.php

@@ -24,17 +24,32 @@ trait OrderFromRequest
 {
 
     /**
-     * Convenience method to read get / post params as string, with default null.
-     * This is a commonly used pattern in the RA controllers
+     * Convenience method to read orderBy from both get & post params as string, with default null.
      */
-    private function getString(Request $request, string $paramName): null|string
+    public function getOrderBy(Request $request): null|string
     {
-        if ($request->query->has($paramName)) {
-            return $request->query->getString($paramName);
+        if ($request->query->has('orderBy')) {
+            return $request->query->getString('orderBy');
         }
 
-        if ($request->request->has($paramName)) {
-            return $request->request->getString($paramName);
+        if ($request->request->has('orderBy')) {
+            return $request->request->getString('orderBy');
+        }
+
+        return null;
+    }
+
+    /**
+     * Convenience method to read orderDirection from both get & post params as string, with default null.
+     */
+    public function getOrderDirection(Request $request): null|string
+    {
+        if ($request->query->has('orderDirection')) {
+            return $request->query->getString('orderDirection');
+        }
+
+        if ($request->request->has('orderDirection')) {
+            return $request->request->getString('orderDirection');
         }
 
         return null;

src/Surfnet/StepupRa/RaBundle/DateTime/DateTime.php

@@ -113,7 +113,7 @@ public function getUserIdentifier(): string
         $parts = explode(':', $this->originalIdentity->nameId);
         $identifier = end($parts);
 
-        if ($identifier === false || $identifier === '') {
+        if ($identifier === '') {
             throw new LogicException('Cannot determine user identifier from nameId');
         }
 

src/Surfnet/StepupRa/RaBundle/Security/AuthenticatedIdentity.php

@@ -31,19 +31,14 @@ protected function setUp(): void
     {
         $this->traitObject = new class {
             use OrderFromRequest;
-
-            public function getStringPublic(Request $request, string $paramName): null|string
-            {
-                return $this->getString($request, $paramName);
-            }
         };
     }
 
     #[Test]
     public function it_returns_value_from_query_parameter(): void
     {
         $request = new Request(['orderBy' => 'name']);
-        $result = $this->traitObject->getStringPublic($request, 'orderBy');
+        $result = $this->traitObject->getOrderBy($request);
 
         $this->assertSame('name', $result);
     }
@@ -52,7 +47,7 @@ public function it_returns_value_from_query_parameter(): void
     public function it_returns_value_from_request_parameter(): void
     {
         $request = new Request([], ['orderBy' => 'date']);
-        $result = $this->traitObject->getStringPublic($request, 'orderBy');
+        $result = $this->traitObject->getOrderBy($request);
 
         $this->assertSame('date', $result);
     }
@@ -61,7 +56,7 @@ public function it_returns_value_from_request_parameter(): void
     public function it_returns_null_when_parameter_not_found(): void
     {
         $request = new Request();
-        $result = $this->traitObject->getStringPublic($request, 'orderBy');
+        $result = $this->traitObject->getOrderBy($request);
 
         $this->assertNull($result);
     }
@@ -71,7 +66,7 @@ public function it_prioritizes_query_parameter_over_request_parameter(): void
     {
         $request = new Request(['orderBy' => 'query_value'], ['orderBy' => 'request_value']);
 
-        $result = $this->traitObject->getStringPublic($request, 'orderBy');
+        $result = $this->traitObject->getOrderBy($request);
 
         $this->assertSame('query_value', $result);
     }
@@ -80,18 +75,26 @@ public function it_prioritizes_query_parameter_over_request_parameter(): void
     public function it_handles_empty_string_values(): void
     {
         $request = new Request(['orderBy' => '']);
-        $result = $this->traitObject->getStringPublic($request, 'orderBy');
+        $result = $this->traitObject->getOrderBy($request);
 
         $this->assertSame('', $result);
     }
 
     #[Test]
     public function it_handles_numeric_string_values(): void
     {
-        $request = new Request(['page' => '42']);
-        $result = $this->traitObject->getStringPublic($request, 'page');
+        $request = new Request(['orderBy' => '42']);
+        $result = $this->traitObject->getOrderBy($request);
 
         $this->assertSame('42', $result);
     }
-}
 
+    #[Test]
+    public function it_returns_order_direction_from_query_parameter(): void
+    {
+        $request = new Request(['orderDirection' => 'asc']);
+        $result = $this->traitObject->getOrderDirection($request);
+
+        $this->assertSame('asc', $result);
+    }
+}