Doc doesn't match authenticate() implementations #24585
Conversation
The `AuthenticationManagerInterface::authenticate()` docblock was changed [way back when](symfony@3d97638) to say it will never return null. But existing implementations [do](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Security/Core/Authentication/Provider/RememberMeAuthenticationProvider.php#L43) [return](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Security/Core/Authentication/Provider/AnonymousAuthenticationProvider.php#L41) [null](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Security/Core/Authentication/Provider/UserAuthenticationProvider.php#L59). So I assume the doc is wrong, not the implementations.
|
Changing the The mistake is actually the fact that AuthenticationProviderInterface extends this interface IMO, while it is used to implement the inner of the AuthenticationProviderManager IMO. |
|
I'd agree with @stof: what is in the interface PhpDoc is the real contract, not what is in the implementations. Maybe we need a PR on the implementations you've linked to throw an exception instead of |
|
@stof Aha! We are considering throwing in my own auth provider, this means we can safely do so. If so that makes it a bigger PR than I expected and will take a bit of time due to other tasks, but unless others step in before that I can give it a try. |
Should rely on. Even the |
…n void (glye) This PR was merged into the 2.7 branch. Discussion ---------- [Security] Fixed auth provider authenticate() cannot return void | Q | A | ------------- | --- | Branch? | 2.7 and up | Bug fix? | yes | New feature? | no | BC breaks? | no (arguably) | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | The `AuthenticationManagerInterface` [requires](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Security/Core/Authentication/AuthenticationManagerInterface.php#L30) that `authenticate()` must return a TokenInterface, never null. Several authentication providers are violating this. Changed to throw exception instead. See discussion in earlier PR #24585 which was changing the docblock rather than the implementations. Commits ------- 6e18b56 [Security] Fixed auth provider authenticate() cannot return void
The
AuthenticationManagerInterface::authenticate()docblock was changed way back when to say it will never return null. But existing implementations then and now do return null. So it seems the doc is wrong, not the implementations.Closed, see alternative approach: #24644