Add provider awareness of their authentication method, decoupling from the assumption of all using API keys #149
Conversation
…m the assumption of all using API keys.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
JasonTheAdams
left a comment
JasonTheAdams
left a comment
There was a problem hiding this comment.
Nice work, @felixarntz! A couple points I'm a bit confused on.
| if ($authenticationMethod === null && $type->isCloud()) { | ||
| $authenticationMethod = RequestAuthenticationMethod::apiKey(); | ||
| } |
There was a problem hiding this comment.
Are we just considering this a safe assumption?
There was a problem hiding this comment.
Fair point. I added this for backward compatibility, but I already started thinking yesterday "what if there's a cloud provider that doesn't need any authentication and therefore wants this to be null?
Maybe we just get rid of it and always use null as a default? It's a BC break, but probably not that important given almost nobody is implementing providers.
| return ApiKeyRequestAuthentication::class; | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
Seems logically impossible to arrive here? How is this method nullable?
There was a problem hiding this comment.
Good point. I suppose for now this can unconditionally return ApiKeyRequestAuthentication because that's the only value for the enum. I'll update this.
| get_class($requestAuthentication) | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
I'm a bit confused by this logic. We get the authentication method enum from the provider class, then we get the authentication class derived from the enum, null if the enum is null, and then we throw an exception if the authentication class is null saying that it does not expect authentication but has an authentication method enum.
The only way this would get here would be if the getImplementationClass() method returned null, but that's logically impossible (see my other comment).
I also notice this down below, too. I guess it all hinges on whether the getImplementationClass() return type is in fact nullable or not.
There was a problem hiding this comment.
This clause is there because one shouldn't attempt to set a RequestAuthentication instance for a provider that doesn't need any authentication (i.e. where authenticationMethod is null).
I agree with your feedback that the getImplementationClass() return type shouldn't be nullable, so I'll update, and then the logic here has to be adjusted accordingly. But at its core the condition will still be needed - it'll instead just check whether the authenticationMethod is null instead of whether the expectedClass is null. Makes sense?
…plementationClass() return type non-nullable.
|
@JasonTheAdams I've updated the PR based on the comments feedback. |
Fixes #143