Skip to content

Conversation

@terabytesoftw
Copy link
Member

@terabytesoftw terabytesoftw commented Aug 11, 2025

Q A
Is bugfix? ✔️
New feature?
Breaks BC?

Summary by CodeRabbit

  • New Behavior

    • Error responses now consistently use appropriate HTTP status codes; internal errors return 500.
    • Misconfigured error handling now results in a 500 status instead of 200.
    • Behavior is consistent across environments.
  • Bug Fixes

    • Eliminated inconsistent status code setting during exception handling.
  • Tests

    • Updated tests to reflect standardized status handling and removed environment-specific assumptions.
  • Chores

    • Streamlined internal error-handling logic and cleaned up legacy utilities and imports.

…t` by removing unused code and improving error handling logic.
@coderabbitai
Copy link

coderabbitai bot commented Aug 11, 2025

Walkthrough

Refactors ErrorHandler to stop manually setting HTTP status codes, assigns 500 explicitly in fallback responses, and updates tests and test stubs to remove SAPI simulation and related assertions. Swaps mocked function from php_sapi_name to http_response_code. Adjusts a test to expect 500 on misconfigured error handler.

Changes

Cohort / File(s) Summary
Error handling core
src/http/ErrorHandler.php
Removed manual HTTP status code setting; cleaned imports; fallback response now sets status 500 explicitly; no public API changes.
HTTP exception tests
tests/http/ErrorHandlerTest.php
Deleted parameterized HttpException SAPI test; removed related imports.
Stateless app tests
tests/http/StatelessApplicationTest.php
Stopped forcing SAPI in three tests; updated expected status from 200 to 500 for invalid error handler action; adjusted imports.
Test mocks (HTTP functions)
tests/support/MockerExtension.php
Replaced mock export from php_sapi_name() to http_response_code(?int): int, delegating to HTTPFunctions.
HTTPFunctions stub
tests/support/stub/HTTPFunctions.php
Removed SAPI state and methods (php_sapi_name, set_sapi); reset() no longer touches SAPI; http_response_code unchanged.

Sequence Diagram(s)

sequenceDiagram
  participant App
  participant ErrorHandler
  participant Renderer
  participant Response

  App->>ErrorHandler: handleException(Throwable)
  alt Normal render path
    ErrorHandler->>Renderer: render(Throwable)
    Renderer-->>ErrorHandler: PSR-7 Response
    ErrorHandler-->>App: Response (status from render path)
  else Fallback
    ErrorHandler->>ErrorHandler: createErrorResponse()
    ErrorHandler->>Response: setStatusCode(500)
    ErrorHandler-->>App: 500 Response
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested labels

bug

Poem

A whisk of code, a hop so light,
We nix the SAPI from our sight.
Status flows where render goes,
And 500 blooms when fallback shows.
Ears up high, I thump with glee—
Clean paths, clear tests, more harmony! 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4a40e57 and 3ef5bfc.

📒 Files selected for processing (5)
  • src/http/ErrorHandler.php (2 hunks)
  • tests/http/ErrorHandlerTest.php (1 hunks)
  • tests/http/StatelessApplicationTest.php (2 hunks)
  • tests/support/MockerExtension.php (1 hunks)
  • tests/support/stub/HTTPFunctions.php (0 hunks)
💤 Files with no reviewable changes (1)
  • tests/support/stub/HTTPFunctions.php
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-07-20T16:35:15.341Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#6
File: tests/http/RequestTest.php:1536-1552
Timestamp: 2025-07-20T16:35:15.341Z
Learning: In the yii2-extensions/psr-bridge project, the base TestCase class already handles $_SERVER cleanup in setUp() and tearDown() methods, so individual test methods that extend TestCase don't need manual $_SERVER restoration.

Applied to files:

  • tests/http/ErrorHandlerTest.php
📚 Learning: 2025-07-20T16:35:15.341Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#6
File: tests/http/RequestTest.php:1536-1552
Timestamp: 2025-07-20T16:35:15.341Z
Learning: In the yii2-extensions/psr-bridge project, the base TestCase class already handles $_SERVER cleanup in setUp() and tearDown() methods (lines 28 and 32), so individual test methods that extend TestCase don't need manual $_SERVER restoration.

Applied to files:

  • tests/http/ErrorHandlerTest.php
📚 Learning: 2025-07-20T16:33:57.495Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#6
File: tests/http/RequestTest.php:1564-1578
Timestamp: 2025-07-20T16:33:57.495Z
Learning: The TestCase class in yii2-extensions/psr-bridge automatically handles $_SERVER superglobal cleanup by saving its original state before each test and restoring it afterward in setUp() and tearDown() methods. Manual $_SERVER cleanup in individual test methods is unnecessary when extending this TestCase.

Applied to files:

  • tests/http/ErrorHandlerTest.php
📚 Learning: 2025-08-08T15:28:00.137Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#71
File: tests/adapter/ServerRequestAdapterTest.php:2215-2215
Timestamp: 2025-08-08T15:28:00.137Z
Learning: In yii2-extensions/psr-bridge, tests extend tests/TestCase which defines a protected const COOKIE_VALIDATION_KEY. Test code should use self::COOKIE_VALIDATION_KEY instead of hardcoded cookieValidationKey literals.

Applied to files:

  • tests/http/ErrorHandlerTest.php
📚 Learning: 2025-08-08T15:24:06.045Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#71
File: tests/TestCase.php:23-27
Timestamp: 2025-08-08T15:24:06.045Z
Learning: In yii2-extensions/psr-bridge (tests/TestCase.php), maintainer preference: it’s acceptable to use random-looking strings for test-only constants like COOKIE_VALIDATION_KEY; no need to replace with an obviously non-secret value unless CI/secret scanners become problematic.

Applied to files:

  • tests/http/ErrorHandlerTest.php
📚 Learning: 2025-08-10T13:15:30.365Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#78
File: tests/adapter/ServerParamsPsr7Test.php:8-8
Timestamp: 2025-08-10T13:15:30.365Z
Learning: In the yii2-extensions/psr-bridge project, classes referenced in PHPDoc annotations (such as throws declarations) must be imported even if not used in the actual code, to avoid ghost class references and maintain proper documentation.

Applied to files:

  • tests/http/ErrorHandlerTest.php
📚 Learning: 2025-08-10T20:39:09.325Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#82
File: tests/adapter/UploadedFilesPsr7Test.php:197-248
Timestamp: 2025-08-10T20:39:09.325Z
Learning: In the yii2-extensions/psr-bridge project, the TestCase base class configures the runtime path as `dirname(__DIR__) . '/runtime'` in the application configuration. Tests that need to save files to the runtime directory can rely on this pre-configured path.

Applied to files:

  • tests/http/ErrorHandlerTest.php
📚 Learning: 2025-08-08T15:28:00.137Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#71
File: tests/adapter/ServerRequestAdapterTest.php:2215-2215
Timestamp: 2025-08-08T15:28:00.137Z
Learning: In yii2-extensions/psr-bridge tests, prefer using self::COOKIE_VALIDATION_KEY from tests/TestCase over hardcoded 'cookieValidationKey' strings to avoid secret scanners FP and improve maintainability.

Applied to files:

  • tests/http/ErrorHandlerTest.php
📚 Learning: 2025-08-10T20:39:09.325Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#82
File: tests/adapter/UploadedFilesPsr7Test.php:197-248
Timestamp: 2025-08-10T20:39:09.325Z
Learning: In the yii2-extensions/psr-bridge project, the TestCase base class (tests/TestCase.php) automatically handles temporary file cleanup. The `createTmpFile()` method tracks files in `$this->tmpFiles[]`, and the `tearDown()` method calls `closeTmpFile()` to clean them up. Tests extending TestCase don't need manual file cleanup when using these methods.

Applied to files:

  • tests/http/ErrorHandlerTest.php
📚 Learning: 2025-08-06T22:52:05.608Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#64
File: tests/http/StatelessApplicationTest.php:1939-1967
Timestamp: 2025-08-06T22:52:05.608Z
Learning: In yii2-extensions/psr-bridge tests, when testing specific component methods like Request::resolve(), it's necessary to call $app->handle($request) first to initialize all application components before testing the method in isolation. This ensures proper component lifecycle initialization.

Applied to files:

  • tests/http/ErrorHandlerTest.php
🧬 Code Graph Analysis (1)
tests/support/MockerExtension.php (1)
tests/support/stub/HTTPFunctions.php (2)
  • HTTPFunctions (37-193)
  • http_response_code (149-156)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: phpunit / PHP 8.4-windows-latest
  • GitHub Check: mutation / PHP 8.3-ubuntu-latest
  • GitHub Check: mutation / PHP 8.3-ubuntu-latest
🔇 Additional comments (6)
src/http/ErrorHandler.php (2)

9-9: LGTM: Import cleanup aligns with refactoring objectives.

The consolidated import statement removes unused dependencies (console\Exception, web\HttpException, http_response_code) while grouping related exception classes, supporting the transition away from manual HTTP status handling.


165-165: LGTM: Explicit status code setting improves error handling clarity.

Setting the status code explicitly to 500 for fallback error responses ensures predictable behavior and aligns with the removal of manual HTTP status handling elsewhere in the codebase.

tests/http/ErrorHandlerTest.php (1)

7-7: LGTM: Removes unused import after test removal.

The removal of TestWith from the PHPUnit attributes import is appropriate since the parameterized cross-SAPI test that used this attribute was removed as part of the SAPI simulation cleanup.

tests/support/MockerExtension.php (1)

78-82: LGTM: Mock change aligns with SAPI simulation removal.

Switching from php_sapi_name to http_response_code mock is consistent with the refactoring to remove SAPI simulation. The function signature correctly matches the standard http_response_code function and properly delegates to the HTTPFunctions stub.

tests/http/StatelessApplicationTest.php (2)

13-14: LGTM: Import reorganization for consistency.

Separating the Exception import from the grouped import statement is a minor stylistic change that doesn't affect functionality.


1084-1087: LGTM: Test expectation updated to match improved error handling.

The change from expecting status 200 to 500 for misconfigured ErrorHandler scenarios correctly reflects the new explicit fallback error handling that sets 500 status codes. This represents better error handling behavior where misconfigurations result in proper HTTP error status codes.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix_mini_75

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@terabytesoftw terabytesoftw added the bug Something isn't working label Aug 11, 2025
@codecov
Copy link

codecov bot commented Aug 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (4a40e57) to head (dab7910).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##                main       #88   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
+ Complexity       321       319    -2     
===========================================
  Files             12        12           
  Lines            805       801    -4     
===========================================
- Hits             805       801    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ensure RAW format responses do not contain HTML tags..
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants