-
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 1 commit
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 |
|---|---|---|
|
|
@@ -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.