Skip to content

Connectors: Use validate_plugin() for the connector install check#11701

Open
t-hamano wants to merge 10 commits intoWordPress:trunkfrom
t-hamano:65020-update-is-active-check
Open

Connectors: Use validate_plugin() for the connector install check#11701
t-hamano wants to merge 10 commits intoWordPress:trunkfrom
t-hamano:65020-update-is-active-check

Conversation

@t-hamano
Copy link
Copy Markdown
Contributor

@t-hamano t-hamano commented May 4, 2026

This PR backports the changes from Gutenberg to the core: WordPress/gutenberg#77897

Trac ticket: https://core.trac.wordpress.org/ticket/65020

Use of AI Tools

None


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Previously, `WP_Connector_Registry::register()` silently filled in `__return_true` when no `is_active` callback was provided, which made it impossible for consumers to distinguish "no callback supplied" from "callback always returns true." Defer the check to call sites instead, so the stored connector data reflects what was actually registered.

Update `_wp_register_default_connector_settings()` and `_wp_connectors_get_connector_script_module_data()` to treat a missing callback as active, matching the documented `Defaults to __return_true` semantics.

See #65020.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 4, 2026

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Comment on lines -273 to -276
if ( ! isset( $connector['plugin']['is_active'] ) ) {
$connector['plugin']['is_active'] = '__return_true';
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove Defaults to __return_true. from line no 118

@t-hamano t-hamano marked this pull request as ready for review May 5, 2026 08:30
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props wildworks, westonruter, mukesh27, jorgefilipecosta.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

t-hamano and others added 2 commits May 5, 2026 18:47
…default.

Replace the raw `file_exists()` install probe in `_wp_connectors_get_connector_script_module_data()` with `validate_plugin()`, which adds path traversal protection and confirms the file is a recognised plugin rather than just any file at that path. Restores the `require_once` for `wp-admin/includes/plugin.php` that the function depends on.

Also updates the `is_active` docblock in `WP_Connector_Registry::register()` to spell out that an omitted callback paired with a `file` is treated as active on the assumption that the plugin must be loaded in order to register itself, so callers do not mistake the omission for a no-op.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@westonruter
Copy link
Copy Markdown
Member

See feedback from @jorgefilipecosta at https://github.com/WordPress/gutenberg/pull/77897/changes#r3183140237 and https://github.com/WordPress/gutenberg/pull/77897/changes#r3189900673

If is_active is always then set among the \WP_Connector_Registry::$registered_connectors, then the Connector type modified in b3b40be should be updated as follows:

diff --git a/src/wp-includes/class-wp-connector-registry.php b/src/wp-includes/class-wp-connector-registry.php
index fbf35ad73e..4fd451b2b0 100644
--- a/src/wp-includes/class-wp-connector-registry.php
+++ b/src/wp-includes/class-wp-connector-registry.php
@@ -41,7 +41,7 @@
  *     },
  *     plugin?: array{
  *         file: non-empty-string,
- *         is_active?: callable(): bool
+ *         is_active: callable(): bool
  *     }
  * }
  */

t-hamano and others added 2 commits May 6, 2026 11:11
Move the missing-callback fallback from `_wp_connectors_get_connector_script_module_data()` into `WP_Connector_Registry::register()`, where the registered connector now always has `plugin.is_active` set to `__return_true` when omitted. This lets every consumer call the callback unconditionally instead of repeating an `isset()` guard, and tightens the PHPStan shape so `is_active` is no longer optional on the stored array.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ion.

Follow-up to the previous commit, which moved the `__return_true` fallback back into `WP_Connector_Registry::register()`. Restore the test assertions so they once again expect `plugin.is_active` to be set to `__return_true` when a callback is omitted, matching the registered shape.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@t-hamano
Copy link
Copy Markdown
Contributor Author

t-hamano commented May 6, 2026

Based on the feedback in Gutenberg, I have updated this PR. Ultimately, this PR only leaves minor code quality improvements 😄

@t-hamano t-hamano changed the title Connectors: Stop auto-defaulting plugin.is_active in the registry Connectors: Use validate_plugin() for the connector install check May 6, 2026
`WP_Connector_Registry::register()` always initializes `$connector['plugin']` as an array and always defaults `is_active` to `__return_true`, while `file` is only set when supplied. Reflect that in the type: `plugin` is required on the stored connector and `file` is the optional key, not the other way around.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines +42 to 45
* plugin: array{
* file?: non-empty-string,
* is_active: callable(): bool
* }
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is the correct type to match the current specifications. The plugins should always be an array, and the file field should be optional.

Copy link
Copy Markdown
Member

@westonruter westonruter May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a small discrepancy here, it seems. Namely, the the register() method says that $args is a Connector. But this isn't true, because for Connector the plugin key is optional and both the file and is_active keys are optional. I suggest we replace this line:

* @phpstan-param Connector $args

With the actual shape that is allowed to be passed into register() for $args.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in e15d155

Copy link
Copy Markdown
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good, thank you @t-hamano!

Comment thread src/wp-includes/connectors.php Outdated
t-hamano and others added 2 commits May 6, 2026 17:58
Why: The variable was flagged by `Generic.Formatting.MultipleStatementAlignment` because the equals sign was not aligned with the adjacent `$is_activated` and `$is_installed` assignments.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions

This comment was marked as off-topic.

@github-actions github-actions Bot closed this May 6, 2026
@westonruter
Copy link
Copy Markdown
Member

This was not committed yet.

@westonruter westonruter reopened this May 6, 2026
@westonruter
Copy link
Copy Markdown
Member

I found several PHPStan issues at rule level 10 when looking at src/wp-includes/class-wp-connector-registry.php and src/wp-includes/connectors.php:

 ------ ----------------------------------------------------------------------- 
  Line   connectors.php                                                         
 ------ ----------------------------------------------------------------------- 
  88     Function wp_get_connector() should return array{name:                  
         non-empty-string, description: non-empty-string, logo_url?:            
         non-empty-string, type: non-empty-string, authentication:              
         array{method: 'api_key'|'none', credentials_url?: non-empty-string,    
         setting_name?: non-empty-string, constant_name?: non-empty-string,     
         env_var_name?: non-empty-string}, plugin?: array{file:                 
         non-empty-string}}|null but returns array{name: non-empty-string,      
         description: non-empty-string, logo_url?: non-empty-string, type:      
         non-empty-string, authentication: array{method: 'api_key'|'none',      
         credentials_url?: non-empty-string, setting_name?: non-empty-string,   
         constant_name?: non-empty-string, env_var_name?: non-empty-string},    
         plugin: array{file?: non-empty-string, is_active: callable():          
         bool}}|null.                                                           
         🪪  return.type                                                        
         💡  Type #1 from the union: Offset 'plugin' (array{file:               
         non-empty-string}) does not accept type array{file?:                   
         non-empty-string, is_active: callable(): bool}: Array might not have   
         offset 'file'.                                                         
         at src/wp-includes/connectors.php:88                                   
  150    Function wp_get_connectors() should return array<string, array{name:   
         non-empty-string, description: non-empty-string, logo_url?:            
         non-empty-string, type: non-empty-string, authentication:              
         array{method: 'api_key'|'none', credentials_url?: non-empty-string,    
         setting_name?: non-empty-string, constant_name?: non-empty-string,     
         env_var_name?: non-empty-string}, plugin?: array{file:                 
         non-empty-string}}> but returns array<string, array{name: non-empty-s  
         tring, description: non-empty-string, logo_url?: non-empty-string,     
         type: non-empty-string, authentication: array{method:                  
         'api_key'|'none', credentials_url?: non-empty-string, setting_name?:   
         non-empty-string, constant_name?: non-empty-string, env_var_name?:     
         non-empty-string}, plugin: array{file?: non-empty-string, is_active:   
         callable(): bool}}>.                                                   
         🪪  return.type                                                        
         💡  Offset 'plugin' (array{file: non-empty-string}) does not accept    
         type array{file?: non-empty-string, is_active: callable(): bool}:      
         Array might not have offset 'file'.                                    
         at src/wp-includes/connectors.php:150                                  
  353    Offset 'setting_name' on array{method: 'api_key'|'none',               
         credentials_url?: mixed} in isset() does not exist.                    
         🪪  isset.offset                                                       
         at src/wp-includes/connectors.php:353                                  
  358    Offset 'constant_name' on array{method: 'api_key'|'none',              
         credentials_url?: mixed, setting_name: non-falsy-string} in isset()    
         does not exist.                                                        
         🪪  isset.offset                                                       
         at src/wp-includes/connectors.php:358                                  
  358    Offset 'env_var_name' on *NEVER* in isset() always exists and is not   
         nullable.                                                              
         🪪  isset.offset                                                       
         at src/wp-includes/connectors.php:358                                  
  358    Result of || is always true.                                           
         🪪  booleanOr.alwaysTrue                                               
         at src/wp-includes/connectors.php:358                                  
  359    Parameter #1 $str of function strtoupper expects string, string|null   
         given.                                                                 
         🪪  argument.type                                                      
         at src/wp-includes/connectors.php:359                                  
  361    Offset 'constant_name' on array{method: 'api_key'|'none',              
         credentials_url?: mixed, setting_name: non-falsy-string} in isset()    
         does not exist.                                                        
         🪪  isset.offset                                                       
         at src/wp-includes/connectors.php:361                                  
  365    Offset 'env_var_name' on array{method: 'api_key'|'none',               
         credentials_url?: mixed, setting_name: non-falsy-string,               
         constant_name: non-falsy-string} in isset() does not exist.            
         🪪  isset.offset                                                       
         at src/wp-includes/connectors.php:365                                  
  371    Offset 'is_active' on array{file: 'ai-provider-for…'} in isset() does  
         not exist.                                                             
         🪪  isset.offset                                                       
         at src/wp-includes/connectors.php:371                                  
  381    Parameter #2 $args of method WP_Connector_Registry::register()         
         expects array{name: non-empty-string, description: non-empty-string,   
         logo_url?: non-empty-string, type: non-empty-string, authentication:   
         array{method: 'api_key'|'none', credentials_url?: non-empty-string,    
         setting_name?: non-empty-string, constant_name?: non-empty-string,     
         env_var_name?: non-empty-string}, plugin: array{file?:                 
         non-empty-string, is_active: callable(): bool}}, array{name: string,   
         description: string, type: 'ai_provider', plugin: array{file?:         
         'ai-provider-for…', is_active: Closure(): bool}, authentication:       
         array{method: 'api_key', credentials_url?: non-falsy-string|null,      
         setting_name: non-falsy-string, constant_name: non-falsy-string,       
         env_var_name: non-falsy-string}|array{method: 'none',                  
         credentials_url?: non-falsy-string|null}, logo_url?: string|null}      
         given.                                                                 
         🪪  argument.type                                                      
         💡  Offset 'name' (non-empty-string) does not accept type string.      
         💡  Offset 'description' (non-empty-string) does not accept type       
         string.                                                                
         💡  Offset 'logo_url' (non-empty-string) does not accept type          
         string|null.                                                           
         💡  Offset 'authentication' (array{method: 'api_key'|'none',           
         credentials_url?: non-empty-string, setting_name?: non-empty-string,   
         constant_name?: non-empty-string, env_var_name?: non-empty-string})    
         does not accept type array{method: 'api_key', credentials_url?:        
         non-falsy-string|null, setting_name: non-falsy-string, constant_name:  
         non-falsy-string, env_var_name: non-falsy-string}|array{method:        
         'none', credentials_url?: non-falsy-string|null}: Offset               
         'credentials_url' (non-empty-string) does not accept type              
         non-falsy-string|null.                                                 
         at src/wp-includes/connectors.php:381                                  
  581    Parameter #2 ...$values of function sprintf expects                    
         bool|float|int|string|null, mixed given.                               
         🪪  argument.type                                                      
         at src/wp-includes/connectors.php:581                                  
  586    Parameter #2 ...$values of function sprintf expects                    
         bool|float|int|string|null, mixed given.                               
         🪪  argument.type                                                      
         at src/wp-includes/connectors.php:586                                  
  633    Parameter #1 $apiKey of class                                          
         WordPress\AiClient\Providers\Http\DTO\ApiKeyRequestAuthentication      
         constructor expects string, mixed given.                               
         🪪  argument.type                                                      
         at src/wp-includes/connectors.php:633                                  
  690    Offset 'is_active' does not exist on array{file: non-falsy-string}.    
         🪪  offsetAccess.notFound                                              
         at src/wp-includes/connectors.php:690                                  
  690    Parameter #1 $function of function call_user_func expects callable():  
         mixed, mixed given.                                                    
         🪪  argument.type                                                      
         at src/wp-includes/connectors.php:690                                  
 ------ ----------------------------------------------------------------------- 


 [ERROR] Found 16 errors                                                        

I've fixed these in e15d155 by amending this PR.

Comment on lines +78 to +79
* file?: non-empty-string,
* is_active: callable(): bool,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to be consistent with Connector on WP_Connector_Registry.

Comment on lines +141 to +142
* file?: non-empty-string,
* is_active: callable(): bool,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to be consistent with Connector on WP_Connector_Registry.

* @phpstan-return ?array{
* name: non-empty-string,
* description: non-empty-string,
* description: string,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the default is an empty string.

However, it would seem preferable to remain non-empty-string and to instead have the property be optional as with the other properties.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ai_registry = AiClient::defaultRegistry();

foreach ( $ai_registry->getRegisteredProviderIds() as $connector_id ) {
foreach ( array_filter( $ai_registry->getRegisteredProviderIds() ) as $connector_id ) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because returns list<string> and not list<non-empty-string>. The array_filter() can be removed with an upstream fix: WordPress/php-ai-client#232

'method' => 'api_key',
);
if ( $credentials_url ) {
$authentication['credentials_url'] = $credentials_url;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because credentials_url is either a non-empty-string or undefined. It doesn't accept null.

'logo_url' => $logo_url,
);
if ( $logo_url ) {
$defaults[ $connector_id ]['logo_url'] = $logo_url;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, because the type is non-empty-string when provided, not non-empty-string|null

if ( 'api_key' === $args['authentication']['method'] ) {
$sanitized_id = str_replace( '-', '_', $id );

if ( ! isset( $args['authentication']['setting_name'] ) ) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this change and the other changes in this block, the if conditions were removed. This is because it was checking isset() for array keys which are never set on $defaults.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Suppress whitespace changes to make the review of the diff easier.)

@westonruter
Copy link
Copy Markdown
Member

For reference, the command I used to obtain the above PHPStan errors:

composer phpstan -- --configuration=phpstan.neon.dist --level=10 src/wp-includes/class-wp-connector-registry.php src/wp-includes/connectors.php 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants