Fix AI credentials detection#537
Conversation
…le, PHP constant and database value This fixes the invalid error on the AI settings page. Before this only the database option value was checked when evaluating if an API key is set. This introduces the helper `has_api_key_configured` that also checks for env value or PHP constant. If one of the values is set, the helper returns true.
This fixes the state display o a connector in the dashboard widget if the API key is configured using a env or a constant by using the new helper function `has_api_key_configured`
|
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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #537 +/- ##
=============================================
- Coverage 70.72% 70.57% -0.16%
+ Complexity 1144 1143 -1
=============================================
Files 67 67
Lines 5510 5522 +12
=============================================
Hits 3897 3897
- Misses 1613 1625 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I’m not sure how to test this in the e2e tests. Sure, we could add a test spec for this, but this would require to set the config value in .wp-env.test.json and this would break other tests. Should set up an additional env with the value and run a seperate test script for this? Do you have any other ideas? |
I don't think we need explicit e2e testing for this, since the outcome - behavior when there are/arent creds - hasn't changed. What's changing here is the internal handling for where credentials can be sourced from, so PHPUnit (Integration) testing should be enough IMO. |
|
Why don't you check directly on the AI provider instance whether it's configured? The connectors for AI providers gets registered automatically based on the info stored in AI provider registry. In fact, the names for the env variable and PHP constant are standardized there. |
I agree with @justlevine here that we don't really need an E2E test for this. That said, we do load a test plugin in our test environment and so you could use that to help with this. As an example of something I've done before, you could add something like this in the // Set the OpenAI API key as a constant when override param is set.
add_action(
'init',
static function () {
if (
isset( $_GET['ai_api_key_override'] ) &&
! defined( 'OPENAI_API_KEY' )
) {
define( 'OPENAI_API_KEY', 'valid-api-key' );
}
},
1
);And then in your E2E test, do the following:
Could then replicate this for the dashboard widget |
@gziolo Not sure I completely follow? Is this around the question about E2E tests? Or a suggestion on how the code in this PR could be better? Need to do a full review but in looking quickly, does look like it pulls the env and constant name from the Connector Registry. As far as I know though, that registry doesn't tell us if a Connector is actually configured. Let me know if that's not correct though. EDIT: Reading again, I think what you're referring to is the AI Client Registry, not the Connectors Registry. In that case I do think we can use that instead though it's not super different from the approach taken here |
| * @param array{setting_name?: string, env_var_name?: string, constant_name?: string} $auth Authentication configuration. | ||
| * @return bool True if an API key is set by one of the configuration options, false otherwise. | ||
| */ | ||
| function has_api_key_configured( array $auth ): bool { |
There was a problem hiding this comment.
Based on feedback from @gziolo and looking at things closer, a different approach we can take here is using the AI Client Registry. I'm not super opinionated either way as Core itself uses both approaches but it is slightly less code This is likely the better approach as I think about it more, see https://github.com/WordPress/wordpress-develop/blob/88612f5798c7ec0d2d94044114a2f1e8fe111897/src/wp-includes/connectors.php#L692-L697
$registry = AiClient::defaultRegistry();
$connectors = get_ai_connectors();
$has_credentials = false;
foreach ( $connectors as $connector_id => $connector_data ) {
if (
! $registry->hasProvider( $connector_id ) ||
! $registry->isProviderConfigured( $connector_id )
) {
continue;
}
$has_credentials = true;
break;
}
There was a problem hiding this comment.
Only looked briefly, but ::isProviderConfigured() also seems more future-compat and likely to stay robust enough for whatever Connector changes come in WP7.1.
|
@dkotter, I'm talking about production code and the fact that AI providers need to be configured before they get discovered by the connectors registry. Even when the AI provider uses db setting, it's the connector that sets everything on the AI provider instance. See: |
I think we're saying the same things, apologize for the back and forth (see my comment here on recommended changes that I think matches what you're saying). If we just use |
4 participants
What?
Closes #536
Why?
if the provider API key is set up using constant in
wp-config.phpor an ENV (instead of providing the key via the connectors settings page)How?
has_api_key_configuredthat checks environment variable, PHP constant and database option.has_ai_credentialsandAI_Status_Widgetto get the correct state informationUse of AI Tools
AI assistance: Yes
Tool(s): IDE Code completion
Model(s): Devstral-Small-2-24B-Instruct-2512
Used for: IDE Code completion
Testing Instructions
Screenshots or screencast
Changelog Entry
Fixed connector status in settings page an dashboard widget if key is configured using env or constant