fix: send HTTP 503 from Database::errorPage()#4272
Conversation
|
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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDatabase::errorPage() now sets HTTP 503 and a Retry-After: 60 header when headers have not been sent, then renders the fatal error HTML. The test asserts that http_response_code() is 503 after calling errorPage(). ChangesHTTP error status on database failures
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/phpMyFAQ/DatabaseTest.php (1)
128-144: ⚡ Quick winConsider resetting response code in tearDown for test isolation.
The test sets the HTTP response code to 503 but doesn't reset it afterward. While this likely won't affect other tests in this file (none check response codes), it's better practice to clean up global state between tests.
♻️ Suggested approach to reset response code
Add to the
tearDown()method (around line 38-49):protected function tearDown(): void { $this->setStaticPropertyValue('databaseDriver', $this->originalDatabaseDriver); $this->setStaticPropertyValue('dbType', $this->originalDbType); $this->setStaticPropertyValue('tablePrefix', $this->originalTablePrefix); parent::tearDown(); // Reset HTTP response code to default http_response_code(200); if (isset($this->sqliteTestFile) && is_file($this->sqliteTestFile)) { `@unlink`($this->sqliteTestFile); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/phpMyFAQ/DatabaseTest.php` around lines 128 - 144, The test testErrorPage() sets the global HTTP response code to 503 via http_response_code() but does not restore it, risking test pollution; update the tearDown() method to reset the global response code (call http_response_code(200) or the previous value) after parent::tearDown() so tests are isolated — modify the existing tearDown() that already handles restoring static properties (databaseDriver, dbType, tablePrefix) to also call http_response_code(200) and keep any existing cleanup like unlinking $this->sqliteTestFile.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/phpMyFAQ/DatabaseTest.php`:
- Line 134: Add an assertion to DatabaseTest to verify the Retry-After header
set by Database::errorPage()—after the existing $this->assertEquals(503,
http_response_code()) check, assert that headers_list() (or in_array) contains
the string "Retry-After: 60" (e.g. $this->assertContains('Retry-After: 60',
headers_list()) or $this->assertTrue(in_array('Retry-After: 60',
headers_list()))), so the test validates both the 503 status and the
Retry-After: 60 header.
---
Nitpick comments:
In `@tests/phpMyFAQ/DatabaseTest.php`:
- Around line 128-144: The test testErrorPage() sets the global HTTP response
code to 503 via http_response_code() but does not restore it, risking test
pollution; update the tearDown() method to reset the global response code (call
http_response_code(200) or the previous value) after parent::tearDown() so tests
are isolated — modify the existing tearDown() that already handles restoring
static properties (databaseDriver, dbType, tablePrefix) to also call
http_response_code(200) and keep any existing cleanup like unlinking
$this->sqliteTestFile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bd4cf9c-2f98-4c36-9d67-675dcd0b2d30
📒 Files selected for processing (2)
phpmyfaq/src/phpMyFAQ/Database.phptests/phpMyFAQ/DatabaseTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- phpmyfaq/src/phpMyFAQ/Database.php
|
@crandler thanks, good catch! Could you please use "4.1" as target branch instead of "main"? |
When the DB connection fails, errorPage() rendered the error HTML with the default HTTP 200. That causes downstream caches (nginx proxy_cache, Varnish, CDN edges) to store the error page and keep serving it after the DB has recovered, and it makes status-code based health checks blind to the outage. Set http_response_code(503) and a Retry-After hint before emitting the body; guarded with headers_sent() so existing call sites that may have already produced output are not affected.
http_response_code() emits a 'has no effect' warning when a previous
header('HTTP/...') call has already set the status line, which surfaces
in the test suite with --fail-on-warning. Switch to header() with the
status line plus the response_code parameter -- same effect for real
requests, no warning during tests. Also drop the now-unnecessary status
reset at the top of testErrorPage().
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
crandler
force-pushed
the
fix/database-error-page-http-503
branch
from
8004bf4 to
71ad6cb
Compare
|
Heads-up: the |
headers_list() is always empty in PHP CLI / PHPUnit runs, so the Retry-After assertion added in the previous test update could never match. Use xdebug_get_headers() instead, which records header() calls even in CLI; guard with function_exists() so the assertion is skipped gracefully when Xdebug is not loaded. Also remove the accidentally duplicated <title> assertion.
2 participants
Closes #4271.
Problem
Database::errorPage()echoes the "Fatal phpMyFAQ Error" body without setting an HTTP status, so the response goes out with the PHP default200 OK. Downstream caches treat that as a cacheable success and keep serving the error page after the DB has recovered, and status-only health checks (Nagios, uptime probes, k8s readiness) report green during the outage.Change
Set
http_response_code(503)andRetry-After: 60before emitting the body. Guarded byheaders_sent()so any caller that happens to have output something already is not broken with a warning.503 Service Unavailableis the correct semantic for a transient backend dependency failure;Retry-Afteris the standard hint for clients and caches.Test
testErrorPage()extended: resets the status to 200 before the call and asserts thathttp_response_code()returns503aftererrorPage()runs. The existing string assertions on the body remain unchanged.Summary by CodeRabbit
Bug Fixes
Tests