-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix: add wp_supports_ai() and related constant + filter
#11149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
7ff6888
26d6c92
3aa0aae
941cc8b
78b010b
a5cef8e
98b881f
e47bedb
3ee6aaa
d4b8da5
a2d738f
95a827a
b3f04fd
f7f754d
5a06aea
2b05524
ffc7e32
414588a
d0b4a03
007b572
bb965aa
eda3323
e0719a0
e8387df
bd314e7
9773705
069e6d4
4f43aad
0d1ffb9
9126d8b
bcb364b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,27 @@ | |||||||||
|
|
||||||||||
| use WordPress\AiClient\AiClient; | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Returns whether AI features are supported in the current environment. | ||||||||||
| * | ||||||||||
| * @since 7.0.0 | ||||||||||
| */ | ||||||||||
| function wp_supports_ai(): bool { | ||||||||||
| // Constant check gives a hard short-circuit for environments that cannot be overridden with a filter, such as wp-config.php settings or hosting provider configurations. | ||||||||||
| if ( defined( 'WP_AI_SUPPORT' ) && ! WP_AI_SUPPORT ) { | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with this, and it's established in WordPress. Relying on constants without allowing to filter them is problematic for several reasons, including testing. We shouldn't avoid this pattern because we're scared of some malicious actor enabling AI again via filter. If you have a malicious actor that can do that, you have worse problems.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but weaponization of AI swarms is more than just about immediately API key bills, when you think about the number of abandoned WP sites and our responsibility . And there are dozens of constants in core that have no matching filter.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding testing, that's the reason the function has a filter. So we just not getting coverage of a single early return. (Conversely, there's no reason to add a constant at all if it's just to serve a default. You can accomplish the same thing with a different-priority hook callback) Will also repeat the argument that plugins shouldn't be able to override user choice on this, now that we're sub thread and not top-level any more. |
||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Filters whether the current request should use AI. | ||||||||||
| * | ||||||||||
| * @since 7.0.0 | ||||||||||
| * | ||||||||||
| * @param bool $is_enabled Whether the current request should use AI. Default true. | ||||||||||
| */ | ||||||||||
| return (bool) apply_filters( 'wp_supports_ai', true ); | ||||||||||
|
justlevine marked this conversation as resolved.
Outdated
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Creates a new AI prompt builder using the default provider registry. | ||||||||||
| * | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -166,6 +166,11 @@ class WP_AI_Client_Prompt_Builder { | |
| */ | ||
| public function __construct( ProviderRegistry $registry, $prompt = null ) { | ||
| try { | ||
| if ( ! wp_supports_ai() ) { | ||
| // The catch block will convert this to a WP_Error. | ||
| throw new \RuntimeException( __( 'AI features are not supported in the current environment.' ) ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WordPress should not throw exceptions. This is a discouraged pattern that we shouldn't randomly start here. Instead, we need to incorporate this check right above the other filter in this class to see whether a specific prompt is supported: Because, if AI is disabled, the prompt for sure is not supported. This also addresses my primary concern voiced in several places before: We should not require devs to make two checks. They should simply be able to call the relevant Including this check in the existing central place for checking AI capability/prompt support is critical.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @felixarntz can you please clarify, perhaps with a code sample what you're trying to say? And I'm not by computer to double check right not but IIRC and as previously noted
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @felixarntz I think I figured out what you meant - please take a look at and let me know if this works: eda3323 If I understood correctly, the part I was missing is that there's no guarantee the constructor will be called before the first usage of an If that's still not it, please lmk 🤔
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope that wasn't it... I was sleep deprived and didnt realize that I reverted the change in 069e6d4 which also adds a test showing that $builder->is_supported() returns false identically to if someone set the I did some more searching, and see quite a few uses cases of exception throwing, not the least in our own core ai work, so now I'm even more puzzled about your concern here.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I mean is we should move this check into the If AI is disabled, it should behave the same way as if that filter returned
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @felixarntz did as you requested in 4f43aad . IMO it's an unnecessary change that hurts readability and makes the cognitive complexity of
This was always true... |
||
| } | ||
|
|
||
| $this->builder = new PromptBuilder( $registry, $prompt ); | ||
| } catch ( Exception $e ) { | ||
| $this->builder = new PromptBuilder( $registry ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| <?php | ||
| /** | ||
| * Tests for wp_supports_ai(). | ||
| * | ||
| * @group ai-client | ||
| * @covers ::wp_supports_ai | ||
| */ | ||
|
|
||
| class Tests_WP_Supports_AI extends WP_UnitTestCase { | ||
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
|
justlevine marked this conversation as resolved.
Outdated
|
||
| public function tear_down() { | ||
| // Remove the WP_DISABLE_AI constant if it was defined during tests. | ||
| remove_all_filters( 'wp_supports_ai' ); | ||
|
|
||
| parent::tear_down(); | ||
| } | ||
|
|
||
| /** | ||
| * Test that wp_supports_ai() defaults to true. | ||
| * | ||
| * @ticket 64591 | ||
| */ | ||
| public function test_defaults_to_true() { | ||
| $this->assertTrue( wp_supports_ai() ); | ||
| } | ||
|
|
||
| /** | ||
| * Tests that the wp_supports_ai filter can disable/enable AI features. | ||
| */ | ||
| public function test_filter_can_disable_ai_features() { | ||
| add_filter( 'wp_supports_ai', '__return_false' ); | ||
| $this->assertFalse( wp_supports_ai() ); | ||
|
|
||
| // Try a later filter to re-enable AI and confirm that it works. | ||
| add_filter( 'wp_supports_ai', '__return_true' ); | ||
| $this->assertTrue( wp_supports_ai() ); | ||
| } | ||
| } | ||

Uh oh!
There was an error while loading. Please reload this page.