refactor: complete middleware consolidation (phase 3) — remaining entity middleware and inline loads (#8176)

Copilot · DawoudIO · web-flow · commit 6253c9ab1200 · 2026-03-06T07:02:47.000-08:00
Completes the middleware consolidation started in #8166/#8169 by refactoring all five remaining middleware classes to extend `AbstractEntityMiddleware` and eliminating all identified inline entity loads from route handlers. ## Middleware refactors - **`FamilyMiddleware`** / **`PersonMiddleware`** — straightforward extends; removes manual `MiddlewareInterface` boilerplate and legacy `withStatus(412, ...)` error responses - **`PersonMiddleware`** gains an optional `$routeParamName` constructor param (default `'personId'`) to support routes whose path param is named differently (e.g. `{userID}`) - **`EventsMiddleware`** — standardises missing-param response to **412** via `renderErrorJSON` (was a bespoke 400 via `renderJSON`) - **`PropertyMiddleware`** — extends `AbstractEntityMiddleware`; the constructor-injected `$type` filter is applied inside `loadEntity()`: if the loaded property's class doesn't match, `null` is returned and the base class returns a standard 404. No `process()` override needed. - **`UserMiddleware`** — extends `AbstractEntityMiddleware` with a `process()` override that preserves the auth logic: current-user identity shortcut sets the attribute directly from the in-memory user; non-admin requesting another user's data gets a 401; admin path delegates to `parent::process()` for the DB lookup + 404 guard. ```php // UserMiddleware — auth logic preserved, entity load delegated to parent public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { $loggedInUser = AuthenticationManager::getCurrentUser(); if ($loggedInUser->getId() == $userId) { return $handler->handle($request->withAttribute('user', $loggedInUser)); // shortcut } if (!$loggedInUser->isAdmin()) { return $response->withStatus(401); } return parent::process($request, $handler); // loads from DB + 404 guard } ``` ## Inline entity load fixes | Route | Was | Now | |---|---|---| | `GET /groups/{groupID}/roles` | `GroupQuery::create()->findOneById($groupID)` | `$request->getAttribute('group')` + `GroupMiddleware` | | `DELETE /groups/{groupID}/removeperson/{userID}` | `PersonQuery::create()->findPk($args['userID'])` | `$request->getAttribute('person')` + `new PersonMiddleware('userID')` | | `POST /groups/{groupID}/addperson/{userID}` | same as above | same fix | | `GET /api/user/{userId}/permissions` | `UserQuery::create()->findPk($userId)` (redundant — `UserMiddleware` already on the group) | `$request->getAttribute('user')` | Removes now-unused `PersonQuery` import from `people-groups.php` and `UserQuery` import from `user-admin.php`.  <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>refactor: complete middleware consolidation (phase 3) — remaining entity middleware and inline loads</issue_title> > <issue_description>## Context > > PR #8166 introduced `AbstractEntityMiddleware`, `InputSanitizationMiddleware`, and `RequestParameterValidationMiddleware`. > PR #8169 refactored `GroupMiddleware`, `DepositMiddleware`, `CalendarMiddleware`, and `KioskDeviceMiddleware` to extend `AbstractEntityMiddleware`, and wired entity middleware to 17 routes. > > This issue tracks the **remaining work** to complete the consolidation. > > --- > > ## 1 — Middleware files not yet extending `AbstractEntityMiddleware` > > | File | Route param | Attribute | Notes | > |------|-------------|-----------|-------| > | `Slim/Middleware/Api/FamilyMiddleware.php` | `familyId` | `family` | Straightforward refactor | > | `Slim/Middleware/Api/PersonMiddleware.php` | `personId` | `person` | Straightforward refactor | > | `Slim/Middleware/EventsMiddleware.php` | `id` | `event` | Returns **400** (not 412) for missing param — needs `getNotFoundMessage()` + consider moving to `Api/` | > | `Slim/Middleware/Api/UserMiddleware.php` | `userId` | `user` | Has additional auth checks alongside entity loading; evaluate separation | > | `Slim/Middleware/Api/PropertyMiddleware.php` | `propertyId` | `property` | Has constructor-injected type parameter; needs factory pattern or override | > > > **Skip**: `PublicCalendarMiddleware` — loads by access token (not route param), sets multiple attributes; not a simple PK load. > > --- > > ## 2 — Inline entity loads still in route handlers > > | Route | File | Line | Entity loaded | Middleware exists? | > |-------|------|------|---------------|--------------------| > | `GET /{groupID}/roles` | `api/routes/people/people-groups.php` | ~197 | `Group` via `GroupQuery::create()->findOneById($groupID)` | Yes — `GroupMiddleware` | > | `DELETE /{groupID}/removeperson/{userID}` | `api/routes/people/people-groups.php` | ~298 | `Person` via `PersonQuery::create()->findPk($userID)` | Yes — `PersonMiddleware` | > | `POST /{groupID}/addperson/{userID}` | `api/routes/people/people-groups.php` | ~337 | `Person` via `PersonQuery::create()->findPk($userID)` | Yes — `PersonMiddleware` | > | `GET /api/user/{userId}/permissions` | `admin/routes/api/user-admin.php` | ~135 | `User` via `UserQuery::create()->findPk($userId)` | Yes — `UserMiddleware` | > > --- > > ## Suggested order > > 1. `FamilyMiddleware` and `PersonMiddleware` — simple extends, unblock the inline-load fixes > 2. Fix inline loads in `people-groups.php` (after `PersonMiddleware` is refactored) > 3. Fix inline load in `user-admin.php` (after `UserMiddleware` is evaluated) > 4. `EventsMiddleware` — confirm 400 vs 412 is intentional, then refactor + consider namespace move > 5. `UserMiddleware` and `PropertyMiddleware` — more complex; separate issues if needed</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details>  - Fixes #8170  --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey). --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: DawoudIO <554959+DawoudIO@users.noreply.github.com> Co-authored-by: George Dawoud <george@dawouds.com>
diff --git a/cypress/e2e/api/private/standard/private.people.family.spec.js b/cypress/e2e/api/private/standard/private.people.family.spec.js
@@ -88,8 +88,8 @@ describe("API Private Family", () => {
         });
 
         it("Returns error for non-existent family", () => {
-            // API returns 412 Precondition Failed for non-existent family
-            cy.makePrivateAdminAPICall("GET", "/api/family/99999", null, 412);
+            // AbstractEntityMiddleware returns 404 Not Found for missing entity
+            cy.makePrivateAdminAPICall("GET", "/api/family/99999", null, 404);
         });
     });
 
diff --git a/cypress/e2e/api/private/standard/private.people.person.spec.js b/cypress/e2e/api/private/standard/private.people.person.spec.js
@@ -22,8 +22,8 @@ describe("API Private Person", () => {
         });
 
         it("Returns error for non-existent person", () => {
-            // API returns 412 Precondition Failed for non-existent person
-            cy.makePrivateAdminAPICall("GET", "/api/person/99999", null, 412);
+            // AbstractEntityMiddleware returns 404 Not Found for missing entity
+            cy.makePrivateAdminAPICall("GET", "/api/person/99999", null, 404);
         });
     });
 
diff --git a/rector.php b/rector.php
@@ -1,7 +1,5 @@
 <?php
 
-declare(strict_types=1);
-
 use Rector\CodeQuality\Rector\Class_\InlineConstructorDefaultToPropertyRector;
 use Rector\Config\RectorConfig;
 use Rector\Set\ValueObject\LevelSetList;
diff --git a/src/ChurchCRM/Slim/Middleware/Api/DepositMiddleware.php b/src/ChurchCRM/Slim/Middleware/Api/DepositMiddleware.php
@@ -1,7 +1,5 @@
 <?php
 
-declare(strict_types=1);
-
 namespace ChurchCRM\Slim\Middleware\Api;
 
 use ChurchCRM\model\ChurchCRM\DepositQuery;
diff --git a/src/ChurchCRM/Slim/Middleware/Api/FamilyMiddleware.php b/src/ChurchCRM/Slim/Middleware/Api/FamilyMiddleware.php
@@ -3,31 +3,26 @@
 namespace ChurchCRM\Slim\Middleware\Api;
 
 use ChurchCRM\model\ChurchCRM\FamilyQuery;
-use ChurchCRM\Slim\SlimUtils;
 
-use Psr\Http\Message\ServerRequestInterface;
-use Psr\Http\Server\RequestHandlerInterface;
-use Psr\Http\Server\MiddlewareInterface;
-use Psr\Http\Message\ResponseInterface;
-use Laminas\Diactoros\Response;
-
-class FamilyMiddleware implements MiddlewareInterface
+class FamilyMiddleware extends AbstractEntityMiddleware
 {
-    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
+    protected function getRouteParamName(): string
     {
-        $response = new Response();
-        $familyId = SlimUtils::getRouteArgument($request, 'familyId');
-        if (empty(trim($familyId))) {
-            return $response->withStatus(412, gettext('Missing') . ' FamilyId');
-        }
+        return 'familyId';
+    }
 
-        $family = FamilyQuery::create()->findPk($familyId);
-        if (empty($family)) {
-            return $response->withStatus(412, 'FamilyId: ' . $familyId . ' ' . gettext('not found'));
-        }
+    protected function getAttributeName(): string
+    {
+        return 'family';
+    }
 
-        $request = $request->withAttribute('family', $family);
+    protected function loadEntity(string $id): mixed
+    {
+        return FamilyQuery::create()->findPk($id);
+    }
 
-        return $handler->handle($request);
+    protected function getNotFoundMessage(): string
+    {
+        return gettext('Family not found');
     }
 }
diff --git a/src/ChurchCRM/Slim/Middleware/Api/GroupMiddleware.php b/src/ChurchCRM/Slim/Middleware/Api/GroupMiddleware.php
@@ -1,7 +1,5 @@
 <?php
 
-declare(strict_types=1);
-
 namespace ChurchCRM\Slim\Middleware\Api;
 
 use ChurchCRM\model\ChurchCRM\GroupQuery;
diff --git a/src/ChurchCRM/Slim/Middleware/Api/KioskDeviceMiddleware.php b/src/ChurchCRM/Slim/Middleware/Api/KioskDeviceMiddleware.php
@@ -1,7 +1,5 @@
 <?php
 
-declare(strict_types=1);
-
 namespace ChurchCRM\Slim\Middleware\Api;
 
 use ChurchCRM\model\ChurchCRM\KioskDeviceQuery;
diff --git a/src/ChurchCRM/Slim/Middleware/Api/PersonMiddleware.php b/src/ChurchCRM/Slim/Middleware/Api/PersonMiddleware.php
@@ -3,30 +3,28 @@
 namespace ChurchCRM\Slim\Middleware\Api;
 
 use ChurchCRM\model\ChurchCRM\PersonQuery;
-use ChurchCRM\Slim\SlimUtils;
-use Laminas\Diactoros\Response;
-use Psr\Http\Message\ServerRequestInterface;
-use Psr\Http\Server\RequestHandlerInterface;
-use Psr\Http\Server\MiddlewareInterface;
-use Psr\Http\Message\ResponseInterface;
 
-class PersonMiddleware implements MiddlewareInterface
+class PersonMiddleware extends AbstractEntityMiddleware
 {
-    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
+    public function __construct(private readonly string $routeParamName = 'personId') {}
+
+    protected function getRouteParamName(): string
     {
-        $response = new Response();
-        $personId = SlimUtils::getRouteArgument($request, 'personId');
-        if (empty(trim($personId))) {
-            return $response->withStatus(412, gettext('Missing') . ' PersonId');
-        }
+        return $this->routeParamName;
+    }
 
-        $person = PersonQuery::create()->findPk($personId);
-        if (empty($person)) {
-            return $response->withStatus(412, 'PersonId : ' . $personId . ' ' . gettext('not found'));
-        }
+    protected function getAttributeName(): string
+    {
+        return 'person';
+    }
 
-        $request = $request->withAttribute('person', $person);
+    protected function loadEntity(string $id): mixed
+    {
+        return PersonQuery::create()->findPk($id);
+    }
 
-        return $handler->handle($request);
+    protected function getNotFoundMessage(): string
+    {
+        return gettext('Person not found');
     }
 }
diff --git a/src/ChurchCRM/Slim/Middleware/Api/PropertyMiddleware.php b/src/ChurchCRM/Slim/Middleware/Api/PropertyMiddleware.php
@@ -1,46 +1,37 @@
 <?php
 
+declare(strict_types=1);
+
 namespace ChurchCRM\Slim\Middleware\Api;
 
 use ChurchCRM\model\ChurchCRM\PropertyQuery;
-use ChurchCRM\Slim\SlimUtils;
-use ChurchCRM\Utils\LoggerUtils;
-
-use Laminas\Diactoros\Response;
-use Psr\Http\Message\ServerRequestInterface;
-use Psr\Http\Server\RequestHandlerInterface;
-use Psr\Http\Server\MiddlewareInterface;
-use Psr\Http\Message\ResponseInterface;
 
-class PropertyMiddleware implements MiddlewareInterface
+class PropertyMiddleware extends AbstractEntityMiddleware
 {
-    protected string $type;
+    public function __construct(private readonly string $type) {}
 
-    public function __construct(string $type)
+    protected function getRouteParamName(): string
     {
-        $this->type = $type;
+        return 'propertyId';
     }
 
-    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
+    protected function getAttributeName(): string
     {
-        $propertyId = SlimUtils::getRouteArgument($request, 'propertyId');
-        $response = new Response();
-        if (empty(trim($propertyId))) {
-            return $response->withStatus(412, gettext('Missing') . ' PropertyId');
-        }
-
-        $property = PropertyQuery::create()->findPk($propertyId);
-
-        if (empty($property)) {
-            LoggerUtils::getAppLogger()->debug('Pro Type is ' . $property->getPropertyType()->getPrtClass() . ' Looking for ' . $this->type);
+        return 'property';
+    }
 
-            return $response->withStatus(412, 'PropertyId : ' . $propertyId . ' ' . gettext('not found'));
-        } elseif ($property->getPropertyType()->getPrtClass() != $this->type) {
-            return $response->withStatus(500, 'PropertyId : ' . $propertyId . ' ' . gettext(' has a type mismatch'));
+    protected function loadEntity(string $id): mixed
+    {
+        $property = PropertyQuery::create()->findPk($id);
+        if ($property !== null && $property->getPropertyType()->getPrtClass() !== $this->type) {
+            return null;
         }
 
-        $request = $request->withAttribute('property', $property);
+        return $property;
+    }
 
-        return $handler->handle($request);
+    protected function getNotFoundMessage(): string
+    {
+        return gettext('Property not found');
     }
 }
diff --git a/src/ChurchCRM/Slim/Middleware/Api/UserMiddleware.php b/src/ChurchCRM/Slim/Middleware/Api/UserMiddleware.php
@@ -5,37 +5,51 @@
 use ChurchCRM\Authentication\AuthenticationManager;
 use ChurchCRM\model\ChurchCRM\UserQuery;
 use ChurchCRM\Slim\SlimUtils;
-
 use Laminas\Diactoros\Response;
+use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use Psr\Http\Server\RequestHandlerInterface;
-use Psr\Http\Server\MiddlewareInterface;
-use Psr\Http\Message\ResponseInterface;
 
-class UserMiddleware implements MiddlewareInterface
+class UserMiddleware extends AbstractEntityMiddleware
 {
+    protected function getRouteParamName(): string
+    {
+        return 'userId';
+    }
+
+    protected function getAttributeName(): string
+    {
+        return 'user';
+    }
+
+    protected function loadEntity(string $id): mixed
+    {
+        return UserQuery::create()->findPk($id);
+    }
+
+    protected function getNotFoundMessage(): string
+    {
+        return gettext('User not found');
+    }
+
     public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
     {
         $response = new Response();
-        $userId = SlimUtils::getRouteArgument($request, 'userId');
+        $userId = SlimUtils::getRouteArgument($request, $this->getRouteParamName());
+
         if (empty(trim($userId))) {
-            return $response->withStatus(412, gettext('Missing') . ' UserId');
+            return SlimUtils::renderErrorJSON($response, gettext('Missing') . ' ' . $this->getRouteParamName(), [], 412);
         }
 
         $loggedInUser = AuthenticationManager::getCurrentUser();
         if ($loggedInUser->getId() == $userId) {
-            $user = $loggedInUser;
-        } elseif ($loggedInUser->isAdmin()) {
-            $user = UserQuery::create()->findPk($userId);
-            if (empty($user)) {
-                return $response->withStatus(412, 'User : ' . $userId . ' ' . gettext('not found'));
-            }
-        } else {
-            return $response->withStatus(401);
+            return $handler->handle($request->withAttribute($this->getAttributeName(), $loggedInUser));
         }
 
-        $request = $request->withAttribute('user', $user);
+        if (!$loggedInUser->isAdmin()) {
+            return $response->withStatus(401);
+        }
 
-        return $handler->handle($request);
+        return parent::process($request, $handler);
     }
 }
diff --git a/src/ChurchCRM/Slim/Middleware/EventsMiddleware.php b/src/ChurchCRM/Slim/Middleware/EventsMiddleware.php
@@ -3,31 +3,27 @@
 namespace ChurchCRM\Slim\Middleware;
 
 use ChurchCRM\model\ChurchCRM\EventQuery;
-use ChurchCRM\Slim\SlimUtils;
-use Laminas\Diactoros\Response;
-use Psr\Http\Message\ServerRequestInterface;
-use Psr\Http\Server\MiddlewareInterface;
-use Psr\Http\Server\RequestHandlerInterface;
-use Psr\Http\Message\ResponseInterface;
+use ChurchCRM\Slim\Middleware\Api\AbstractEntityMiddleware;
 
-class EventsMiddleware implements MiddlewareInterface
+class EventsMiddleware extends AbstractEntityMiddleware
 {
-    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
+    protected function getRouteParamName(): string
     {
-        $eventId = SlimUtils::getRouteArgument($request, 'id');
-        if (empty(trim($eventId))) {
-            $response = new Response();
-            return SlimUtils::renderJSON($response, ['message' => gettext('Missing event id')], 400);
-        }
+        return 'id';
+    }
 
-        $event = EventQuery::create()->findPk($eventId);
+    protected function getAttributeName(): string
+    {
+        return 'event';
+    }
 
-        if (empty($event)) {
-            $response = new Response();
-            return SlimUtils::renderJSON($response, ['message' => gettext('Event not found')], 404);
-        }
-        $request = $request->withAttribute('event', $event);
+    protected function loadEntity(string $id): mixed
+    {
+        return EventQuery::create()->findPk($id);
+    }
 
-        return $handler->handle($request);
+    protected function getNotFoundMessage(): string
+    {
+        return gettext('Event not found');
     }
 }
diff --git a/src/admin/routes/api/user-admin.php b/src/admin/routes/api/user-admin.php
@@ -5,7 +5,6 @@
 use ChurchCRM\Emails\users\ResetPasswordEmail;
 use ChurchCRM\Emails\users\UnlockedEmail;
 use ChurchCRM\model\ChurchCRM\UserConfigQuery;
-use ChurchCRM\model\ChurchCRM\UserQuery;
 use ChurchCRM\Slim\Middleware\Request\Auth\AdminRoleAuthMiddleware;
 use ChurchCRM\Slim\Middleware\Api\UserMiddleware;
 use ChurchCRM\Slim\SlimUtils;
@@ -131,8 +130,7 @@
      * )
      */
     $group->get('/permissions', function (Request $request, Response $response, array $args): Response {
-        $userId = $args['userId'];
-        $user = UserQuery::create()->findPk($userId);
+        $user = $request->getAttribute('user');
 
         return SlimUtils::renderJSON($response, ['user' => $user->getName(), 'userId' => $user->getId(), 'addEvent' => $user->isAddEvent()]);
     });
diff --git a/src/api/routes/people/people-groups.php b/src/api/routes/people/people-groups.php
diff --git a/src/api/routes/people/people-person.php b/src/api/routes/people/people-person.php
diff --git a/src/plugins/core/google-analytics/templates/tracking-code.php b/src/plugins/core/google-analytics/templates/tracking-code.php
