Fix deprecated classes usage - #129
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #129 +/- ##
========================================
Coverage 0.00% 0.00%
Complexity 110 110
========================================
Files 15 15
Lines 290 291 +1
========================================
- Misses 290 291 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates yiisoft/user authentication integration to eliminate usage of deprecated Yii Auth interfaces by introducing a shared authenticator implementation, migrating existing authenticators to the newer interface, and bumping the yiisoft/auth dependency accordingly.
Changes:
- Add
UserAuthenticator(sharedAuthenticatorInterfaceimplementation) and a focused unit test for it. - Migrate
ApiAuth,WebAuth, and deprecatedUserAuthfromAuthenticationMethodInterfacetoAuthenticatorWithChallengeInterfaceand reuse sharedauthenticate()logic via inheritance. - Bump
yiisoft/authrequirement to^3.3.0and record the change in the changelog.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/UserAuthenticatorTest.php | Adds tests for the new shared authenticator behavior (guest vs logged-in identity). |
| src/UserAuthenticator.php | Introduces a reusable AuthenticatorInterface implementation based on CurrentUser. |
| src/UserAuth.php | Migrates deprecated UserAuth to new auth interface and reuses shared authentication logic. |
| src/Method/WebAuth.php | Migrates WebAuth to new auth interface and reuses shared authentication logic. |
| src/Method/ApiAuth.php | Migrates ApiAuth to new auth interface and reuses shared authentication logic. |
| composer.json | Narrows yiisoft/auth requirement to ^3.3.0. |
| CHANGELOG.md | Adds changelog entry for the deprecated-usage fix. |
Suppressed comments (3)
src/UserAuth.php:26
$currentUseris promoted to a private property but is only used to callparent::__construct()and is never referenced afterwards. This duplicates state withUserAuthenticatorand can lead to confusion (and potential divergence if$currentUserwere ever reassigned). Consider acceptingCurrentUser $currentUseras a plain parameter and passing it toparent::__construct($currentUser)without storing it as a separate property.
public function __construct(private CurrentUser $currentUser, private ResponseFactoryInterface $responseFactory) {
parent::__construct($this->currentUser);
}
src/Method/ApiAuth.php:21
$currentUseris promoted to a property but is only used to callparent::__construct()and is never referenced afterwards. Consider takingCurrentUser $currentUseras a plain parameter and passing it to the parent without storing a separate, shadowing property.
public function __construct(private readonly CurrentUser $currentUser) {
parent::__construct($this->currentUser);
}
src/Method/WebAuth.php:20
WebAuthno longer implementsYiisoft\Auth\AuthenticationMethodInterface, butconfig/di-web.phpstill bindsAuthenticationMethodInterface::class => WebAuth::class(and README/tests refer to that interface). Any DI/autowiring expectingAuthenticationMethodInterfacewill now fail with aTypeErrorwhen given aWebAuth. The DI config + docs/tests should be updated to bind/reference the new auth interface (AuthenticatorWithChallengeInterface/AuthenticatorInterface, depending on intended integration) orWebAuthshould continue implementing the old interface for BC.
final class WebAuth extends UserAuthenticator implements AuthenticatorWithChallengeInterface
{
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…n as a plain parameter and the WebAuth bindings in the config/di-web.php file are set to the AuthenticatorWithChallengeInterface
| /** | ||
| * Implementation of the AuthenticatorInterface for the user. | ||
| */ | ||
| class UserAuthenticator implements AuthenticatorInterface |
There was a problem hiding this comment.
Does it make sense to create a separate class? I don’t think it’s worth it.
There was a problem hiding this comment.
Yes, it's worth it because it gets rid of duplicate code:
https://github.com/yiisoft/user/blob/master/src/Method/ApiAuth.php#L22
https://github.com/yiisoft/user/blob/master/src/Method/WebAuth.php#L29
https://github.com/yiisoft/user/blob/master/src/UserAuth.php#L28
There was a problem hiding this comment.
Duplication itself isn't the strongest argument for inheritance — it's only ~4 lines. In this case, inheritance complicates the code more than the duplication it removes: it turns two previously independent, self-contained classes (WebAuth and ApiAuth, UserAuth is deprecated anyway) into a hierarchy for the sake of reusing a couple of lines, and UserAuthenticator effectively becomes a new part of the package's public API even though it's only meant as an implementation detail.
There was a problem hiding this comment.
The main purpose of the UserAuthenticator is to implement the AuthenticatorInterface for users.
It was created to be used for authentication logic with CurrentUser, not for inheritance.
However, it is also used for inheritance, since the previous AuthenticationMethodInterface violated the interface segregation principle, resulting in code duplication.
There was a problem hiding this comment.
The main purpose of the UserAuthenticator is to implement the AuthenticatorInterface for users.
It was created to be used for authentication logic with CurrentUser, not for inheritance.
ApiAuth resolves this task, isn't it?
There was a problem hiding this comment.
I think that duplication of this code:
if ($this->currentUser->isGuest()) {
return null;
}
return $this->currentUser->getIdentity();is OK. Creating of UserAuthenticator complicates code for me.
Let's wait for other opinions :)
There was a problem hiding this comment.
The UserAuthenticator is not an alternative implementation of the authentication logic.
This is how it must be done following the addition of the AuthenticatorInterface.
In my opinion, ApiAuth should be deprecated, as it contains redundant code (see https://github.com/yiisoft/user/blob/master/src/Method/ApiAuth.php#L31).
There was a problem hiding this comment.
In my opinion, ApiAuth should be deprecated, as it contains redundant code (see
In major version we can change interface of ApiAuth to AuthenticatorInterface and remove challenge() method.
There was a problem hiding this comment.
The UserAuthenticator has been removed
There was a problem hiding this comment.
In my opinion, ApiAuth should be deprecated, as it contains redundant code (see
In major version we can change interface of
ApiAuthtoAuthenticatorInterfaceand removechallenge()method.
In my opinion, changing the interface of ApiAuth to AuthenticatorInterface could cause confusion for users, since the AuthenticatorInterface does not refer to the Api or Web application.
The class implemented for AuthenticatorInterface is universal for any application that uses sessions.
Uh oh!
There was an error while loading. Please reload this page.