Update Authentication validator #14
Conversation
|
|
||
| $adapter->setIdentity($identity); | ||
| $adapter->setCredential($credential); | ||
|
|
||
| $result = $this->service->authenticate($this->adapter); |
There was a problem hiding this comment.
This needs to change to $adapter instead of $this->adapter, as that's what we're authenticating against. I'll make that change during merge.
| } | ||
| } else { | ||
| $adapter = $this->adapter; | ||
| } |
There was a problem hiding this comment.
Extract the logic for pulling the adapter from the AuthenticationService to a new method, so you can then instead say $adapter = $this->adapter ?: $this->getAdapterFromAuthenticationService();.
I'll make that change in a separate commit that I push to your branch.
| break; | ||
| default: | ||
| $this->error(self::GENERAL); | ||
| if (!$result->isValid()) { |
There was a problem hiding this comment.
Flip this conditional around:
if $result->isValid()) {
return true;
}
// now do error workReturn early whenever you can, as it helps make the main code flow more readable.
I'll do this in a separate commit I push to your branch.
| Result::FAILURE_CREDENTIAL_INVALID => self::CREDENTIAL_INVALID, | ||
| Result::FAILURE_IDENTITY_AMBIGUOUS => self::IDENTITY_AMBIGUOUS, | ||
| Result::FAILURE_UNCATEGORIZED => self::UNCATEGORIZED, | ||
| ]; |
There was a problem hiding this comment.
I'd argue this should be a private constant (which we can do as we're now targeting PHP 5.6 and up in develop). I'll do this and push to your branch.
| public function errorMessagesProvider() | ||
| { | ||
| return [ | ||
| [ |
There was a problem hiding this comment.
Provide names for each data set; these help us locate the specific set that caused an error more easily.
I'll do this in a separate commit that I push to your branch.
Authentication validator is able to use adapter from the given AuthenticationService: