Skip to content

Conversation

@olegbaturin
Copy link

@olegbaturin olegbaturin commented Sep 16, 2025

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

Refactor middleware authentication failure handler
Bump minimum PHP version to 8.1

@codecov
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.47%. Comparing base (6eafec3) to head (0811e9b).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #101      +/-   ##
============================================
- Coverage     95.52%   95.47%   -0.05%     
  Complexity       77       77              
============================================
  Files            10       10              
  Lines           201      199       -2     
============================================
- Hits            192      190       -2     
  Misses            9        9              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

* Default authentication failure handler. Responds with "401 Unauthorized" HTTP status code.
*/
final class AuthenticationFailureHandler implements RequestHandlerInterface
final class AuthenticationFailureHandler implements AuthenticationFailureHandlerInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit in having a dedicated interface? Ability to configure it with a single line only? @vjik is there a better way?

@samdark
Copy link
Member

samdark commented Sep 16, 2025

@olegbaturin thanks for the pull request. Discussion with the code in front of you is way more productive.

@samdark samdark requested a review from Copilot September 29, 2025 22:16
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the middleware authentication failure handler by introducing a dedicated interface and updating the constructor signature. It also bumps the minimum PHP version requirement from 8.0 to 8.1.

  • Introduces AuthenticationFailureHandlerInterface to formalize the failure handler contract
  • Updates Authentication middleware constructor to require the interface instead of optional parameter
  • Moves AuthenticationFailureHandler from Handler namespace to root Auth namespace

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/AuthenticationFailureHandlerInterface.php New interface defining the authentication failure handler contract
src/AuthenticationFailureHandler.php Moved to root namespace and implements new interface
src/Middleware/Authentication.php Updated constructor to require failure handler interface
tests/AuthenticationMiddlewareTest.php Updated test methods to use new interface
tests/AuthenticationFailureHandlerTest.php Updated import for moved class
tests/ConfigTest.php New test for DI configuration
config/di-web.php New DI configuration file
composer.json Updated PHP version constraint and added dependencies
CHANGELOG.md Updated with changes
.github/workflows/*.yml Updated PHP versions in CI workflows
Comments suppressed due to low confidence (1)

tests/AuthenticationMiddlewareTest.php:171

  • The method name 'handle' doesn't match the interface being implemented. Since this implements AuthenticationFailureHandlerInterface, verify that the interface method is also named 'handle'.
            public function handle(ServerRequestInterface $request): ResponseInterface

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@samdark samdark requested a review from vjik September 29, 2025 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants