refactor for compliance with PSR-7, PSR-15 and PSR-17 #341
Conversation
Part 1: refactor `/calendar` path, `/calendars` path, `/missals` path, and `/decrees` path Fixes #337
and better handling of content-type for the response in CalendarHandler
I had initially made them handlers by taking the response and operating on it, but now with error middleware in place they are more transparent, and have gone back to being simple ParamsInterface classes
and realign tests
and fix a couple bugs that the Test interface caught!
Other fixes: * add feedback to start-server.sh * add php.supp to .gitignore (when debugging with valgrind) * add empty check to acceptValues
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughMassive refactor from path-based scripts to a PSR-7/PSR-15 architecture: new public/index front controller, handler classes per route, middleware pipeline with centralized error handling, router redesign with dynamic API path discovery, new HTTP enums/exceptions, enum-based data paths, updated params, tests reorganized, and tooling/config tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Front as public/index.php
participant Router as Router
participant Pipe as MiddlewarePipeline
participant Err as ErrorHandlingMiddleware
participant H as <Handler>
participant Emitter as SAPI Emitter
Client->>Front: HTTP request
Front->>Router: bootstrap + Router::route()
Router->>Router: getApiPaths() / build PSR-7 request
Router->>Pipe: handle(request)
Pipe->>Err: process(request, next)
Err->>H: process→handle(request)
H-->>Err: Response (or throws ApiException)
Err-->>Pipe: Response (problem+json on error)
Pipe-->>Router: Response
Router->>Emitter: emitResponse(Response)
Emitter-->>Client: HTTP response
sequenceDiagram
autonumber
actor Client
participant H as RegionalDataHandler
participant FS as Filesystem
participant Schema as LitSchema
participant Enc as Encoder
Client->>H: PUT /data/nation/{code} (JSON)
H->>H: validate method, Accept (INTERMEDIATE), Content-Type
H->>H: parse body → RegionalDataParams
H->>Schema: validateDataAgainstSchema()
Schema-->>H: ok / throws 422
H->>FS: write i18n files + calendar file
FS-->>H: OK / error
H->>Enc: encodeResponseBody(JSON|YAML)
Enc-->>H: Body + headers
H-->>Client: 201 Created
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120–180 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 50
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (19)
phpunit_tests/ApiTestCase.php (1)
25-29: Ensure tests respect API_BASE_PATH by normalizing base_uri and using relative pathsThe current TestCase hard-codes base_uri without any API_BASE_PATH and all HTTP calls use absolute paths (leading “/”), which override Guzzle’s base path. On deployments under a non-root path (e.g.
/api), requests like->get('/calendar')will hithttp://host/calendarinstead ofhttp://host/api/calendar.Please apply the following critical fixes:
- In phpunit_tests/ApiTestCase.php:
- Read and normalize
API_BASE_PATH(ensure it has a trailing slash).- Include it in the
base_uriwhen instantiatingnew Client([...]).- Update the health-check
get('/')toget('').- Update
setUp()client creation and all calls to use''instead of'/…'.--- a/phpunit_tests/ApiTestCase.php +++ b/phpunit_tests/ApiTestCase.php @@ protected static function setUpBeforeClass(): void - $client = new Client([ - 'base_uri' => "{$_ENV['API_PROTOCOL']}://{$_ENV['API_HOST']}:{$_ENV['API_PORT']}", + $rawBase = $_ENV['API_BASE_PATH'] ?? '/'; + $path = rtrim('/'.trim($rawBase, '/'), '/').'/'; // "/" or "/api/" or "/api/v1/" + $baseUri = sprintf( + '%s://%s:%d%s', + $_ENV['API_PROTOCOL'], + $_ENV['API_HOST'], + (int) $_ENV['API_PORT'], + $path + ); + $client = new Client([ + 'base_uri' => $baseUri, 'http_errors' => false, 'timeout' => 2.0, ]); @@ - $response = $client->get('/'); + $response = $client->get(''); protected function setUp(): void @@ - $this->http = new Client([ - 'base_uri' => "{$_ENV['API_PROTOCOL']}://{$_ENV['API_HOST']}:{$_ENV['API_PORT']}", + $this->http = new Client([ + 'base_uri' => $baseUri, 'http_errors' => false, 'timeout' => 2.0, ]);
- In every test under
phpunit_tests/Routes/**replace absolute paths with relative ones:
- E.g. change
to$this->http->get('/calendar');$this->http->get('calendar');- And similarly for
->post('/schemas'),->get('/events/...'), etc.Affected test files include (but are not limited to):
- phpunit_tests/Routes/Readonly/CalendarTest.php
- phpunit_tests/Routes/Readonly/CalendarsTest.php
- phpunit_tests/Routes/Readonly/EasterTest.php
- phpunit_tests/Routes/Readonly/EventsTest.php
- phpunit_tests/Routes/Readonly/SchemasTest.php
- phpunit_tests/Routes/ReadWrite/RegionalDataTest.php
After applying these changes, verify with:
API_BASE_PATH=/ composer start && composer test API_BASE_PATH=/api composer start && composer testphpunit_tests/Routes/Readonly/EasterTest.php (1)
13-15: Use relative path so Guzzle appends API_BASE_PATHWith base_uri including API_BASE_PATH, absolute paths like “/easter” override the base path. Use a relative path.
- $response = $this->http->get('/easter'); + $response = $this->http->get('easter');phpunit_tests/Routes/Readonly/SchemasTest.php (2)
23-28: Regex should account for API_BASE_PATHEndpoints may be served under a base path (e.g., “/api” or “/api/v1”). Incorporate API_BASE_PATH into the expected URL pattern.
- $regex = sprintf( - '/^%s:\/\/%s:%d\/schemas\/(?:[A-Z][A-Za-z]+|openapi)\.json$/', - $_ENV['API_PROTOCOL'], - $_ENV['API_HOST'], - $_ENV['API_PORT'] - ); + $basePathRaw = $_ENV['API_BASE_PATH'] ?? '/'; + $basePath = rtrim('/' . trim($basePathRaw, '/'), '/'); // "" or "/api" or "/api/v1" + $regex = sprintf( + '/^%s:\/\/%s:%d%s\/schemas\/(?:[A-Z][A-Za-z]+|openapi)\.json$/', + $_ENV['API_PROTOCOL'], + $_ENV['API_HOST'], + $_ENV['API_PORT'], + preg_quote($basePath, '/') + );
13-16: Make request paths relative so base_uri path is honoredAbsolute paths bypass the base path in base_uri. Switch to relative paths.
- $response = $this->http->get('/schemas'); + $response = $this->http->get('schemas');- $response = $this->http->post('/schemas'); + $response = $this->http->post('schemas');- $response = $this->http->get('/schemas', [ + $response = $this->http->get('schemas', [ 'headers' => ['Accept' => 'application/yaml'] ]);- $response = $this->http->put('/schemas'); + $response = $this->http->put('schemas');- $response = $this->http->patch('/schemas'); + $response = $this->http->patch('schemas');- $response = $this->http->delete('/schemas'); + $response = $this->http->delete('schemas');Also applies to: 51-56, 70-75, 79-81, 85-87, 91-93
phpunit_tests/Routes/Readonly/CalendarsTest.php (2)
327-334: Fail fast if project root is not found.ApiTestCase::findProjectRoot() returns ?string. If null, $filePath becomes "/jsondata/world_dioceses.json", yielding a misleading “File not found” later.
- $root = ApiTestCase::findProjectRoot(); - $filePath = $root . '/jsondata/world_dioceses.json'; + $root = ApiTestCase::findProjectRoot(); + if (null === $root) { + throw new \RuntimeException('Unable to locate project root from ApiTestCase::findProjectRoot().'); + } + $filePath = $root . '/jsondata/world_dioceses.json';
360-365: Regex should escape protocol/host; dots in 127.0.0.1 currently match any char.preg pattern is built with raw $_ENV values; host like 127.0.0.1 makes the regex too permissive. Escape dynamic parts.
- self::$WIDER_REGION_API_PATH_PATTERN = sprintf( - '/^%s:\/\/%s:%d\/data\/widerregion\/(Europe|Africa|Asia|Oceania|Americas)\?locale=\{locale\}$/', - $_ENV['API_PROTOCOL'], - $_ENV['API_HOST'], - $_ENV['API_PORT'] - ); + $proto = preg_quote((string)($_ENV['API_PROTOCOL'] ?? ''), '/'); + $host = preg_quote((string)($_ENV['API_HOST'] ?? ''), '/'); + $port = (int)($_ENV['API_PORT'] ?? 0); + self::$WIDER_REGION_API_PATH_PATTERN = sprintf( + '/^%s:\/\/%s:%d\/data\/widerregion\/(Europe|Africa|Asia|Oceania|Americas)\?locale=\{locale\}$/', + $proto, + $host, + $port + );phpunit_tests/Routes/Readonly/EventsTest.php (2)
20-21: Fix PHPUnit assertion call: too many argumentsassertSame expects (expected, actual, message). There’s a duplicated message arg that will cause a type error.
- $this->assertSame(JSON_ERROR_NONE, json_last_error(), 'Invalid JSON: ' . json_last_error_msg(), 'Invalid JSON: ' . json_last_error_msg()); + $this->assertSame(JSON_ERROR_NONE, json_last_error(), 'Invalid JSON: ' . json_last_error_msg());
12-12: Initialize typed static property to avoid “uninitialized typed property” pitfallsSafer to declare the static property as nullable and initialize it, then check against null.
- private static object $metadata; + private static ?object $metadata = null; @@ - if (false === isset(self::$metadata)) { + if (self::$metadata === null) {Also applies to: 160-166
src/Utilities.php (2)
475-483: Incorrect English ordinal suffix logic (11/12/13 edge cases).Current logic yields “111st” instead of “111th”. Handle 11–13 as special cases using
% 100.- if ($ord === 1 || ( $ord % 10 === 1 && $ord <> 11 )) { - $ord_suffix = 'st'; - } elseif ($ord === 2 || ( $ord % 10 === 2 && $ord <> 12 )) { - $ord_suffix = 'nd'; - } elseif ($ord === 3 || ( $ord % 10 === 3 && $ord <> 13 )) { - $ord_suffix = 'rd'; - } else { - $ord_suffix = 'th'; - } + $mod100 = $ord % 100; + if ($mod100 >= 11 && $mod100 <= 13) { + $ord_suffix = 'th'; + } else { + switch ($ord % 10) { + case 1: $ord_suffix = 'st'; break; + case 2: $ord_suffix = 'nd'; break; + case 3: $ord_suffix = 'rd'; break; + default: $ord_suffix = 'th'; + } + }
498-503: Guard Latin ordinals array bounds.Accessing
$latinOrdinals[$num]without checking may emit notices for out-of-range keys. Throw a domain error if missing.- case 'la': - $ordinal = $latinOrdinals[$num]; - break; + case 'la': + if (!array_key_exists($num, $latinOrdinals)) { + throw new ServiceUnavailableException('Missing Latin ordinal for ' . $num); + } + $ordinal = $latinOrdinals[$num]; + break;src/Params/RegionalDataParams.php (1)
26-26: Make payload nullable and initialize; avoid uninitialized typed property.
$payloadis only set conditionally; accessing it when not provided will fatally error.- public DiocesanData|NationalData|WiderRegionData $payload; + public DiocesanData|NationalData|WiderRegionData|null $payload = null;src/Health.php (1)
317-320: Bug: preg_match check is always “true” unless an error occurs
preg_match()returns 1 (match), 0 (no match), or false (error). Comparing with!== falseturns both 1 and 0 into true, so$isVersionedDataPathis effectively always true unless there’s a regex error. This will incorrectly rewrite non-versioned paths.- $isVersionedDataPath = preg_match($versionedPattern, $dataPath) !== false; + $isVersionedDataPath = preg_match($versionedPattern, $dataPath) === 1;src/Handlers/EasterHandler.php (1)
126-130: Wrong day-of-week source for Julian date string
- You format the Julian date line with the day-of-week taken from the Gregorian timestamp.
- $julianDateString = self::$dayOfTheWeek->format($gregorian_easter->format('U')) + $julianDateString = self::$dayOfTheWeek->format($julian_easter->format('U')) . ', ' . self::$dayMonthYear->format($julian_easter->format('U'));src/Enum/RomanMissal.php (1)
323-331: Use GlobIterator (or glob()) instead of DirectoryIterator with glob://
DirectoryIterator("glob://$i18n_path*.json")is non-standard;GlobIteratoris the SPL type intended for patterns.Apply:
- if ($i18n_path) { - $iterator = new \DirectoryIterator("glob://$i18n_path*.json"); - foreach ($iterator as $file) { - $locales[] = $file->getBasename('.json'); - } - } + if ($i18n_path) { + /** @var \GlobIterator $iterator */ + $iterator = new \GlobIterator($i18n_path . '*.json', \FilesystemIterator::KEY_AS_PATHNAME); + foreach ($iterator as $file) { + /** @var \SplFileInfo $file */ + $locales[] = $file->getBasename('.json'); + } + }Add at the top of the file if you prefer imports:
use GlobIterator; use FilesystemIterator; use SplFileInfo;src/Handlers/CalendarHandler.php (4)
4308-4316: Add cURL timeouts to avoid hanging on GitHub APIExternal calls should be bounded. Add sane connect/read timeouts.
$ch = curl_init(); curl_setopt($ch, CURLOPT_URL, $GithubReleasesAPI); curl_setopt($ch, CURLOPT_USERAGENT, 'LiturgicalCalendar'); curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); + curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 5); + curl_setopt($ch, CURLOPT_TIMEOUT, 10); //curl_setopt($ch, CURLOPT_HTTPHEADER, $GhRequestHeaders);
736-738: Replace die() with exceptions in handlersCalling die() inside a PSR-15 handler can kill the worker and bypass middleware/error handling. Throw typed exceptions instead.
- die("createPropriumDeTemporeLiturgicalEventByKey requires a key from the Proprium de Tempore, instead got $key"); + throw new \InvalidArgumentException(sprintf( + 'createPropriumDeTemporeLiturgicalEventByKey requires a key from the Proprium de Tempore; got %s', + var_export($key, true) + ));- die("this is strange, {$event_key} is not suppressed? Where is it?"); + throw new \RuntimeException("Unexpected state: '{$event_key}' is not suppressed and cannot be reinstated.");Also applies to: 3845-3846
1605-1613: Bug: wrong date variable used when resolving coinciding events (late Ordinary Time loop)Inside the second loop you use $firstOrdinaryDate when you should be looking at $lastOrdinary. This breaks supersession messages and can throw.
- $litEvent = $this->Cal->solemnityFromDate($firstOrdinaryDate) ?? $this->Cal->feastLordFromDate($firstOrdinaryDate); + $litEvent = $this->Cal->solemnityFromDate($lastOrdinary) ?? $this->Cal->feastLordFromDate($lastOrdinary); - if (null === $litEvent) { - throw new \RuntimeException('We were expecting to find either a Solemnity or a Feast of the Lord on ' . $lastOrdinary->format('Y-m-d') . ' but no LiturgicalEvent was not found'); - } + if (null === $litEvent) { + throw new \RuntimeException('Expected Solemnity or Feast of the Lord on ' . $lastOrdinary->format('Y-m-d') . ' but none was found'); + }
4243-4250: Probable typo: calls dateIsNotSunday() on collection instead of the local helperLiturgicalEventCollection likely doesn’t expose dateIsNotSunday(DateTime). You defined a local static (self::dateIsNotSunday), which is used elsewhere. Use it here too.
- } elseif ($liturgicalEvent->grade->value <= LitGrade::FEAST->value && $this->Cal->notInSolemnities($currentLitEventDate) && $this->Cal->dateIsNotSunday($currentLitEventDate)) { + } elseif ($liturgicalEvent->grade->value <= LitGrade::FEAST->value && $this->Cal->notInSolemnities($currentLitEventDate) && self::dateIsNotSunday($currentLitEventDate)) {src/Handlers/MetadataHandler.php (1)
71-76: Bug: corpus_christi setting points at Ascension enumIn buildNationalCalendarData(), 'corpus_christi' is assigned Ascension::THURSDAY rather than CorpusChristi::THURSDAY. This will serialize the wrong value and break downstream behavior.
Apply this diff (and import the enum):
@@ -use LiturgicalCalendar\Api\Enum\Route; +use LiturgicalCalendar\Api\Enum\Route; +use LiturgicalCalendar\Api\Enum\CorpusChristi; @@ - 'corpus_christi' => Ascension::THURSDAY->value, + 'corpus_christi' => CorpusChristi::THURSDAY->value,Also applies to: 5-13
♻️ Duplicate comments (1)
src/Handlers/RegionalDataHandler.php (1)
1089-1102: Static analysis false positive: validateDataAgainstSchema is used
- Despite PHPMD flagging it as unused, it is invoked in handle() for PUT/PATCH (see Lines 1446, 1452, 1458).
No action needed.
Summary by CodeRabbit