Implement API credentials manager with basic settings screen #6
Conversation
|
We will need WordPress/php-ai-client#79 to be merged and released in order to complete the one TODO in this PR and properly support any registered provider. |
|
@JasonTheAdams This is now ready for a proper review. |
JasonTheAdams
left a comment
JasonTheAdams
left a comment
There was a problem hiding this comment.
All in all it's close to done! I left a handful of little suggestions. 👍
|
|
||
| // If the provider was already found via another client class, just this client class name to the list. | ||
| if ( isset( $wp_ai_client_providers_metadata[ $provider_id ] ) ) { | ||
| $wp_ai_client_providers_metadata[ $provider_id ]['ai_client_classnames'][ AiClient::class ] = true; |
There was a problem hiding this comment.
I don't think this fits the shape of the array defined above for this variable.
There was a problem hiding this comment.
Fixed in 4dacba0, but for some reason that I can't figure out PHPStan is complaining. Do you have any hunch?
There was a problem hiding this comment.
It seems like it's having trouble resolving the imported type. I'd double check the imported type name, and maybe try using a FQCN in the import.
There was a problem hiding this comment.
Just tested, the import itself is working fine. I have no idea what's going on here. If I can't resolve this, I'm just going to redeclare the whole thing for now.
There was a problem hiding this comment.
Fixed via workaround in 0b05a07
It seems intersection types (not union types!) in PHPStan are sort of like a 2nd-class citizen, but that's what we would need here. So I just went with declaring the full shape in one struct, which is okay enough, given it's only 3 properties from elsewhere duplicated here 🤷
Co-authored-by: Jason Adams <[email protected]>
Co-authored-by: Jason Adams <[email protected]>
|
@felixarntz @JasonTheAdams it would be super helpful to get this PR to a |
JasonTheAdams
left a comment
JasonTheAdams
left a comment
There was a problem hiding this comment.
Good work, @felixarntz! LGTM!
This PR implements the "API Credentials Management" point from #1.
Important: It has to be considered that this package may be loaded multiple times within a single WordPress install, whether unprefixed (
WordPress\AI_Client) or prefixed (e.g.Vendor\My_Plugin_Dependencies\WordPress\AI_Client). The implementation of this PR is meant to ensure that the setting and its UI are only registered once, regardless of how often this code is called. Additionally though (and this is probably the most tricky part), it still checks theProviderRegistryfor every instance of the package that is loaded, to ensure that every provider that is available anywhere has UI shown so that the user can provide the API credentials.Note: The settings screen has very basic UI, simply using the classic WordPress Settings API. Eventually, to better productize this, we should consider replacing this with a more appealing design, including potentially usage of React for better interactivity as well as alignment with latest WordPress Core UI principles. But for a first pass, this UI is pragmatic and works.
Screenshot of the settings screen:
