Add AuthenticationServiceInterface #5901

Merged
merged 3 commits into from Mar 10, 2014

Projects

None yet

5 participants

@danizord
Contributor
danizord commented Mar 3, 2014

Just to allow dependency inversion.

@Ocramius Ocramius commented on the diff Mar 3, 2014
library/Zend/Authentication/AuthenticationService.php
@@ -9,7 +9,7 @@
namespace Zend\Authentication;
-class AuthenticationService
+class AuthenticationService implements AuthenticationServiceInterface
@Ocramius
Ocramius Mar 3, 2014 Member

Can you use also {@inheritDoc} in related docblocks here?

@Maks3w
Maks3w Mar 4, 2014 Member

It's worst. none of the methods are implemented and the class is not abstract.

@Ocramius
Ocramius Mar 4, 2014 Member

The interface was actually extracted from the implementation, as far as I know

@Maks3w
Maks3w Mar 4, 2014 Member

Truth. GH show me an early EOF without many of methods

@danizord
danizord Mar 5, 2014 Contributor

@Ocramius The docblocks are not equal, since the interface don't know about adapters and storages.

@Maks3w
Member
Maks3w commented Mar 4, 2014

@danizord Please describe de aim of this PR and complete it if you want the PR to be reviewed

@Maks3w Maks3w commented on an outdated diff Mar 4, 2014
...end/Authentication/AuthenticationServiceInterface.php
@@ -0,0 +1,39 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2014 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ */
+
+namespace Zend\Authentication;
+
+interface AuthenticationServiceInterface
@Maks3w
Maks3w Mar 4, 2014 Member

Add a class docblock describing the porpuse of the interface

@Maks3w Maks3w commented on an outdated diff Mar 4, 2014
...end/Authentication/AuthenticationServiceInterface.php
@@ -0,0 +1,39 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2014 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ */
+
+namespace Zend\Authentication;
+
+interface AuthenticationServiceInterface
+{
+ /**
+ * @return Result
@Maks3w
Maks3w Mar 4, 2014 Member

Add the desription of this method

@danizord
Contributor
danizord commented Mar 5, 2014

@Maks3w The aim is to be able to typehint AuthenticationServiceInterface instead of AuthenticationService in order to allow other implementations.

@weierophinney weierophinney added this to the 2.3.0 milestone Mar 5, 2014
@fabiocarneiro
Contributor

Wow, that's good.

@Ocramius Ocramius commented on the diff Mar 7, 2014
...end/Authentication/AuthenticationServiceInterface.php
+ */
+ public function hasIdentity();
+
+ /**
+ * Returns the authenticated identity or null if no identity is available
+ *
+ * @return mixed|null
+ */
+ public function getIdentity();
+
+ /**
+ * Clears the identity
+ *
+ * @return void
+ */
+ public function clearIdentity();
@Ocramius
Ocramius Mar 7, 2014 Member

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.

@Maks3w
Maks3w Mar 7, 2014 Member

As well there is authenticate (login) I see reasonable clearIdentity (logout)

@Ocramius
Ocramius Mar 7, 2014 Member

authenticate could well be no-op, while clearIdentity really looks like an expenctancy of state change...

@danizord
danizord Mar 7, 2014 Contributor

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.

@weierophinney
weierophinney Mar 7, 2014 Member

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.

@danizord
danizord Mar 7, 2014 Contributor

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.

@weierophinney weierophinney added a commit that referenced this pull request Mar 10, 2014
@weierophinney weierophinney Merge branch 'feature/5901' into develop
Close #5901
fbf3eef
@weierophinney weierophinney merged commit c7fe40e into zendframework:develop Mar 10, 2014

1 check passed

default The Travis CI build passed
Details
@adamlundrigan adamlundrigan referenced this pull request in ZF-Commons/ZfcUser Jan 14, 2015
Open

Update requirements #474

@weierophinney weierophinney added a commit to zendframework/zend-authentication that referenced this pull request May 14, 2015
@weierophinney weierophinney Merge pull request zendframework/zendframework#5901 from danizord/fea…
…ture/authentication-service-interface

Add AuthenticationServiceInterface
32f3538
@weierophinney weierophinney added a commit to zendframework/zend-authentication that referenced this pull request May 14, 2015
@weierophinney weierophinney Merge branch 'feature/5901' into develop 083d12b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment