Add AuthenticationServiceInterface #5901
Conversation
| @@ -9,7 +9,7 @@ | |||
|
|
|||
| namespace Zend\Authentication; | |||
|
|
|||
| class AuthenticationService | |||
| class AuthenticationService implements AuthenticationServiceInterface | |||
There was a problem hiding this comment.
Can you use also {@inheritDoc} in related docblocks here?
There was a problem hiding this comment.
It's worst. none of the methods are implemented and the class is not abstract.
There was a problem hiding this comment.
The interface was actually extracted from the implementation, as far as I know
There was a problem hiding this comment.
Truth. GH show me an early EOF without many of methods
There was a problem hiding this comment.
@Ocramius The docblocks are not equal, since the interface don't know about adapters and storages.
|
@danizord Please describe de aim of this PR and complete it if you want the PR to be reviewed |
|
|
||
| namespace Zend\Authentication; | ||
|
|
||
| interface AuthenticationServiceInterface |
There was a problem hiding this comment.
Add a class docblock describing the porpuse of the interface
|
@Maks3w The aim is to be able to typehint |
|
Wow, that's good. |
| * | ||
| * @return void | ||
| */ | ||
| public function clearIdentity(); |
There was a problem hiding this comment.
After thinking some more about this, I think that we should drop this method from the interface before we're ready to go.
While authenticate, hasIdentity and getIdentity don't imply any mutation of state in the authentication service, clearIdentity obviously does.
This seems like an implementation detail, and most consumers won't need it either.
There was a problem hiding this comment.
As well there is authenticate (login) I see reasonable clearIdentity (logout)
There was a problem hiding this comment.
authenticate could well be no-op, while clearIdentity really looks like an expenctancy of state change...
There was a problem hiding this comment.
Actually, @Ocramius, as said on IRC, the methods hasIdentity() and getIdentity() means that the service is stateful, so, calling authenticate() will likely change the service state and affect the hasIdentity() and getIdentity() returns.
Anyway, I'm ok to remove clearIdentity() from interface.
There was a problem hiding this comment.
authenticate() writes the discovered identity, if any, to the composed storage object; clearIdentity() removes it. The statefulness is in the underlying storage adapter -- and the AuthenticationService acts as a facade that interacts with the storage adapter, the credential verification adapter, etc.
I'd argue clearIdentity() should remain, @Ocramius -- it's a facade operation.
There was a problem hiding this comment.
What about splitting the interface into two?
1 - A interface that only defines authenticate() (implementations can be stateless).
2 - Another interface that extends the first one (or not?) + defines the Identity management API.
Not sure about naming, though.
…-interface Add AuthenticationServiceInterface
…ture/authentication-service-interface Add AuthenticationServiceInterface
Just to allow dependency inversion.