Register Stream operations as WordPress Abilities#1859
Conversation
Add abstract Ability base class, Abilities loader with WP 6.9 + setting gating, and a new "Enable Abilities API" toggle under the existing Advanced settings section. The loader hooks wp_abilities_api_init and will register concrete abilities once they are added in subsequent commits. Falls back silently on WordPress < 6.9.
Implements the six read-only abilities under stream/* namespace: get-records, get-record, get-settings, get-alerts, get-connectors, and get-exclusion-rules. Each ability has hand-written JSON Schemas, delegates to existing Stream APIs, and ships with a PHPUnit test covering name, schema, permission gating, and execution. Tests skip themselves on WordPress < 6.9 via the shared Abilities_TestCase base class.
Add three abilities that mutate Stream state through the existing internal APIs: stream/create-alert (creates a wp_stream_alerts CPT post with alert_type and alert_meta), stream/update-settings (partial-merge update to the wp_stream option), and stream/create-exclusion-rule (appends to the parallel-array exclude_rules option columns Stream already uses). Each ability is gated behind the manage_options capability. Hand-written JSON schemas describe inputs and outputs for AI consumers; create-alert requires the four trigger fields, create-exclusion-rule requires at least one filter property, and update-settings requires a non-empty settings map. Each ability ships with PHPUnit coverage that verifies permissions, schema shape, and end-to-end execution against the option or post store.
Add the two destructive abilities required by the ticket: stream/purge-records (filtered DELETE against the Stream records table with a cascading meta delete that mirrors Admin::erase()) and stream/delete-alert (force-delete a wp_stream_alerts post by ID). purge-records refuses to run unless confirm: true is supplied AND at least one filter (older_than_days, connector, context, action) is set, preventing an accidental full table wipe. The row count is computed before the DELETE so the response is meaningful even though the multi-table DELETE returns the combined affected rows. delete-alert returns a 404 WP_Error when the ID is unknown or refers to a non-alert post type, which makes the ability safely idempotent. Both abilities ship with PHPUnit coverage that exercises permissions, schema validation, the happy path, the refusal paths, and (for purge-records) the meta cascade.
Cover the two infrastructure pieces left untested by the per-ability suites: tests/phpunit/test-class-ability.php exercises the Ability abstract base via an in-file Fake_Ability_For_Test subclass (verifies get_meta() emits category and show_in_rest, conditionally adds an annotations key, and that the default permission_callback denies subscribers and grants admins; also asserts register() makes the ability retrievable via wp_get_ability() when the API is available). tests/phpunit/test-class-abilities.php covers the loader: is_available() tracks the WP_Ability class presence, is_enabled() reflects the advanced_enable_abilities_api option, the constructor only hooks wp_abilities_api_init when both gates pass, get_ability_slugs() lists all eleven slugs, load_abilities() instantiates each, and register_abilities() does not double-load on a second invocation. Resolves: XWPENG-13
PatelUtkarsh
changed the title
- Register 'stream' category on wp_abilities_api_categories_init so abilities with category=stream pass core's category-existence check - Move 'category' from meta to top-level args in Ability::register(), matching the wp_register_ability() contract in WP 6.9 - Replace get-record's broken DB::get_records(['record' => $id]) call (Query class never implemented the singular 'record' arg) with a direct $wpdb single-row lookup - Snapshot/restore $plugin->settings->options in Abilities_TestCase so in-memory mutations from write-ability tests don't leak across tests - Update tests to satisfy the doing_action() guards on wp_register_ability() and wp_register_ability_category()
Add a per-ability 'instructions' annotation: a 1-2 sentence note for AI agents about when and how to call each ability, distinct from the description (which describes what it does). Add tests/phpunit/abilities/test-rest-integration.php covering all three ability types end-to-end: dispatches actual WP_REST_Requests through WP_REST_Server and asserts 200/403/404/405 paths plus the list-abilities endpoint exposes all 11 stream/* abilities. Catches breakage in the real REST stack that direct execute() tests miss. Add idempotent: true to purge-records annotations. WP core's REST router only routes to DELETE when destructive AND idempotent are both true; without idempotent the controller expects POST. Refactor test action-firing to use the documented core test pattern of pushing onto $wp_current_filter rather than registering callbacks through add_action(). Cleaner, no global hook pollution, matches the convention used in WordPress core's own abilities-api tests. Make Abilities::register_abilities() defensive: skip per-ability register() calls when the ability is already registered, preventing spurious _doing_it_wrong notices when load_abilities() runs more than once in the same process.
WP core's WP_Ability::invoke_callback() spreads zero arguments into the execute callback when the ability declares no input_schema (see wp-includes/abilities-api/class-wp-ability.php:506-512). Our previous 'execute($input)' signature required one argument, so any GET request to a no-input-schema ability raised a fatal ArgumentCountError and returned HTTP 500 to the caller. Add '$input = null' as the default on the abstract Ability::execute() plus all 11 concrete subclasses and the test fake. Null matches WP core's own conventions (their invoke_callback and check_permissions both default $input to null). Abilities that DO declare an input_schema continue to receive the parsed value verbatim from core, so the default sits unused for those. Caught by live e2e testing against WP 6.9.1 (Phase 4 of XWPENG-13-e2e.md): get-settings, get-connectors, and get-exclusion-rules previously fataled.
- Authorization: read abilities use 'view_stream'; base default uses
WP_STREAM_SETTINGS_CAPABILITY. Abilities registers a user_has_cap
filter for REST contexts where Admin (and its filter) isn't loaded,
so allowed roles can call read abilities consistently with the UI.
- update-settings: allowlist {section}_{field} keys from registered
fields, run incoming values through Settings::sanitize_settings(),
reject payloads with no recognized keys.
- create-exclusion-rule: schema gains format:ip and maxLength bounds;
execute() sanitizes via sanitize_text_field(), validates IPs with
FILTER_VALIDATE_IP, validates connector against registered slugs,
rejects all-empty payloads.
- purge-records: use rows_affected from the DELETE itself (no stale
pre-count); run orphan-meta sweep after; fix MySQL alias syntax.
- get-record: kept direct query (Query::query has a real array_shift
bug with record__in) but adds explicit blog_id scoping on multisite
so cross-site record leakage cannot occur.
The 5 read-only abilities (get-records, get-record, get-alerts, get-connectors, get-exclusion-rules) all carried an identical permission_callback() returning current_user_can( 'view_stream' ) with the same rationale docblock. Move it into a shared trait so the authorization rule lives in one place. Each ability file require_once's the trait directly so per-test loaders (which require ability files individually) keep working without any autoloader changes. Net -35 LOC. Single-site and multisite Ability suites unchanged: 316 tests pass with the same skipped/incomplete counts as before.
- orderby: add enum bound to Query::query()'s actual sortable columns, and change the default from 'date' (not a real Stream column; silently fell back to ID) to 'created'. This makes the silent fallback impossible at the schema layer for REST callers and surfaces the contract for direct PHP callers. - user_id__in / connector__in: add maxItems: 100 so a caller cannot force an unbounded IN(...) clause from a single request. Tests cover schema shape, REST schema validation (orderby=date now rejected, 101 items rejected), and a behavioral regression that seeds two records with out-of-order created/ID and asserts orderby=created ASC actually orders by created -- not by ID, which is what the old silent fallback was doing.
…or-context Mirror the admin form's create-alert flow (classes/class-alerts.php:766- 806) so API-created alerts behave identically to UI-created ones: - Validate alert_type against $plugin->alerts->alert_types (the registered notifier slugs). Schema can't enum these because wp_stream_alert_types is a filter -- a hardcoded enum would lock out 3rd-party notifiers. Reject unknown slugs with stream_unknown_alert_type / status 400 BEFORE inserting the post. - Split 'connector-context' input into trigger_connector + trigger_context meta keys, exactly like the admin form does. Without the split, Alert_Trigger_Context::check_record() silently let any connector through because trigger_connector was never populated -- alerts created via the API were effectively connector-agnostic. - Build an Alert model from the split meta and use $alert->get_title() for post_title, so the admin list shows a meaningful title instead of 'Auto Draft'. Tests cover the title regression, the connector-dash-context split, and the alert_type rejection path (including no-side-effects: no post is inserted when validation fails).
…ed multisite On a network-activated multisite install, the Abilities API toggle is saved to the wp_stream_network site option via Network::update_site_option(). However, Settings::get_options() only reads from get_site_option() when is_network_admin() is true; in REST and frontend contexts $plugin->settings->options reflects the (typically empty) per-site option. As a result, is_enabled() returned false in REST even when the network admin had enabled the API, making the entire Abilities API silently unreachable on network-activated sites. Read the network option directly via get_site_option($settings->network_options_key) when is_multisite() && $plugin->is_network_activated(), and fall back to the existing in-memory per-site options otherwise (preserves single-site and per-site-activated behavior). Adds two regression tests: - test_is_enabled_reads_network_option_when_network_activated (@group ms-required) flips the wp_stream_is_network_activated filter and proves is_enabled() follows the network option even when in-memory options say disabled. - test_is_enabled_reads_per_site_options_when_not_network_activated proves the network option is ignored when the plugin isn't network-activated.
Add //end try comments to satisfy Squiz.Commenting.LongConditionClosingComment on the new is_enabled() multisite tests, and lift the inline 'not a registered notifier' comment above the array literal so it doesn't trip Squiz.Commenting.PostStatementComment in the unknown-alert_type test.
The comment claimed format:ip is a hint not enforced by rest_validate_value_from_schema(), which is wrong. WP core's rest_is_ip_address() in wp-includes/rest-api.php DOES validate the format and rejects bogus IPs at the schema layer with ability_invalid_input before our execute() runs. Reframe the in-method check as defense-in-depth for direct PHP callers who invoke $ability->execute() outside the REST stack.
There was a problem hiding this comment.
Pull request overview
Integrates Stream with the WordPress 6.9 Abilities API by introducing an ability base class + loader, registering 11 Stream operations under the stream/ namespace (gated behind an Advanced setting), and adding a comprehensive PHPUnit suite to validate schemas, permissions, and REST routing.
Changes:
- Added
Ability(abstract base) andAbilities(loader/gating + category registration) and wired the loader intoPlugin::init(). - Implemented multiple
stream/*abilities (read/write/destructive) backed by existing Stream internals, plus sharedview_streampermission handling for read-only abilities. - Added PHPUnit infrastructure and per-ability tests, updated coverage configuration, and extended PHPCS configuration for Stream’s custom capability.
Review Findings (issues to address)
1) Critical issues
stream/purge-recordscan delete other sites’ logs on multisite when Stream is not network-activated- Why it matters: Stream’s tables are shared (base prefix) and rely on
blog_idfor separation; without ablog_idconstraint this endpoint can purge records across the network. - Suggested fix: Add a
stream.blog_id = %dconstraint whenis_multisite()and$this->plugin->is_multisite_not_network_activated()are true (mirroringAdmin::erase_stream_records()/ scheduled purge behavior).if ( $this->plugin->is_multisite_not_network_activated() ) { $where[] = 'stream.blog_id = %d'; $params[] = get_current_blog_id(); }
- WP VIP ref: Multisite scoping via
get_current_blog_id(); safe SQL construction via$wpdb->prepare().
- Why it matters: Stream’s tables are shared (base prefix) and rely on
2) High issues
stream/get-alertscan returnalert_metain a shape that violates its declared output schema- Why it matters: When
alert_metais missing,get_post_meta(..., true)returns''; casting to(array)yields a numerically-indexed array which JSON-encodes as a list, not an object, breaking schema expectations and clients. - Suggested fix: Normalize
alert_metato an associative array/object: only return the meta value if it’s already an array, otherwise return an empty array. - WP VIP ref: Data consistency for REST outputs;
get_post_meta()return-type handling.
- Why it matters: When
3) Medium/Low issues
-
test_orderby_created_actually_orders_by_created_not_id()doesn’t reliably detect anorderbyfallback-to-ID regression- Why it matters: The test data currently makes
createdorder align with insertion/ID order, so it can pass even iforderby=createdis ignored. - Suggested fix: Insert records such that ID ordering conflicts with
createdordering (e.g., insert the “newer timestamp” record first and the “older timestamp” record second) and assert against that conflict. - WP VIP ref: PHPUnit test correctness (ensuring assertions can fail for the intended regression).
- Why it matters: The test data currently makes
-
Abilities API toggle description lists an endpoint shape that conflicts with this PR’s REST integration tests
- Why it matters: The admin-facing setting description references
/wp-abilities/v1/stream/*, while tests exercise/wp-abilities/v1/abilities/stream/{slug}/run; this can mislead admins during manual verification. - Suggested fix: Update the UI description text to match the actual route structure used by the Abilities API implementation being targeted.
- WP VIP ref: Admin UI clarity / accurate operational documentation.
- Why it matters: The admin-facing setting description references
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/phpunit/test-class-ability.php | Unit tests for the abstract Ability base behavior (meta, permissions, registration). |
| tests/phpunit/test-class-abilities.php | Unit tests for loader gating, multisite enablement behavior, and slug list. |
| tests/phpunit/fake-ability.php | Concrete fake ability used by base-class tests. |
| tests/phpunit/abilities/test-rest-integration.php | REST-level integration tests for routing/method enforcement and discovery. |
| tests/phpunit/abilities/test-class-ability-update-settings.php | Tests for partial settings updates, key allowlisting, and memory refresh. |
| tests/phpunit/abilities/test-class-ability-purge-records.php | Tests for destructive purge safety (confirm + filters) and cascade cleanup. |
| tests/phpunit/abilities/test-class-ability-get-settings.php | Tests for settings read ability output and permissions. |
| tests/phpunit/abilities/test-class-ability-get-records.php | Tests for records querying, schema allowlists/bounds, and ordering behavior. |
| tests/phpunit/abilities/test-class-ability-get-record.php | Tests for single-record fetch behavior and not-found handling. |
| tests/phpunit/abilities/test-class-ability-get-exclusion-rules.php | Tests for exclusion-rule pivoting and internal-key stripping. |
| tests/phpunit/abilities/test-class-ability-get-connectors.php | Tests for connector discovery output shape. |
| tests/phpunit/abilities/test-class-ability-get-alerts.php | Tests for alert listing and status filtering. |
| tests/phpunit/abilities/test-class-ability-delete-alert.php | Tests for deletion safety, idempotency, and post-type validation. |
| tests/phpunit/abilities/test-class-ability-create-exclusion-rule.php | Tests for rule append semantics, sanitization, and validation. |
| tests/phpunit/abilities/test-class-ability-create-alert.php | Tests for alert creation, title generation, and alert-type validation. |
| tests/phpunit/abilities/abilities-testcase.php | Shared Abilities test base (WP 6.9 gating, schema assertions, helpers). |
| tests/bootstrap.php | Loads the new Abilities test base. |
| phpunit.xml | Adds abilities/ directory to coverage include paths. |
| phpunit-multisite.xml | Adds abilities/ directory to multisite coverage include paths. |
| phpcs.xml.dist | Configures PHPCS to recognize view_stream as a custom capability. |
| classes/class-settings.php | Adds the “Enable Abilities API” Advanced setting (WP 6.9+ only). |
| classes/class-plugin.php | Wires Abilities loader into plugin initialization. |
| classes/class-ability.php | Introduces the Ability abstract base and registration helper. |
| classes/class-abilities.php | Introduces the Abilities loader (gating, category registration, instantiation). |
| abilities/trait-view-stream-permission.php | Shared view_stream permission callback for read-only abilities. |
| abilities/class-ability-update-settings.php | Implements stream/update-settings ability. |
| abilities/class-ability-purge-records.php | Implements stream/purge-records ability (destructive). |
| abilities/class-ability-get-settings.php | Implements stream/get-settings ability. |
| abilities/class-ability-get-records.php | Implements stream/get-records ability with schema allowlists and paging. |
| abilities/class-ability-get-record.php | Implements stream/get-record ability with multisite scoping guard. |
| abilities/class-ability-get-exclusion-rules.php | Implements stream/get-exclusion-rules ability with pivoted output. |
| abilities/class-ability-get-connectors.php | Implements stream/get-connectors ability (connector/context/action discovery). |
| abilities/class-ability-get-alerts.php | Implements stream/get-alerts ability (status-filtered listing). |
| abilities/class-ability-delete-alert.php | Implements stream/delete-alert ability (destructive + idempotent). |
| abilities/class-ability-create-exclusion-rule.php | Implements stream/create-exclusion-rule ability with sanitization/validation. |
| abilities/class-ability-create-alert.php | Implements stream/create-alert ability with alert-type validation and title generation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rby test, UI text
- purge-records: scope DELETE by blog_id on non-network-activated multisite
to prevent cross-site record deletion; mirrors Admin::erase_stream_records().
- get-alerts: coerce missing alert_meta to {} instead of [""] so the response
matches the declared object output schema.
- test_orderby_created_actually_orders_by_created_not_id: invert insertion
order so ID-order conflicts with created-order; the test would now fail if
the implementation silently fell back to ORDER BY ID.
- Settings UI: drop the specific /wp-abilities/v1/stream/* path from the
toggle description (the actual route is owned by core's Abilities API).
- Add regression tests for the missing alert_meta normalization and for the
per-blog purge isolation (multisite-only).
…e, UTC cutoff, settings UX, hook isolation
- get-record / purge-records: scope by blog_id = get_current_blog_id() on
any multisite request that is not is_network_admin() (REST is never
network-admin). Replaces the previous is_multisite_not_network_activated()
predicate, which left network-activated installs unprotected against a
per-site admin reading or purging another site's records via REST.
Mirrors Network::network_query_args() default scoping.
- purge-records: compute the older_than_days cutoff as a UTC DateTime in
PHP and bind as %s, mirroring Admin::purge_scheduled_action(). The
previous DATE_SUB(NOW(), INTERVAL %d DAY) used the MySQL server timezone
while Stream's created column is UTC, so it could over- or under-purge
on hosts where the server timezone is not UTC.
- Settings: hide the "Enable Abilities API" toggle on per-site settings
screens when Stream is network-activated. The setting is read from the
network option in that mode, so a per-site checkbox would have been a
silent no-op.
- Tests: snapshot and restore $wp_filter['wp_abilities_api_init'] in
Test_Abilities::setUp/tearDown so the existing remove_all_actions()
calls inside individual tests don't pollute the global hook registry
for subsequent tests in the same process.
- Tests: lock the new behaviors with regression coverage:
* get-record returns stream_record_not_found for foreign-blog IDs on
multisite (must not leak via guessing).
* purge-records does not cross blog boundaries on multisite (rename
drops the _when_not_network_activated suffix).
* purge-records older_than_days uses a UTC cutoff: rows seeded with
explicit UTC timestamps purge correctly regardless of server tz.
* Settings field is visible on non-network-activated installs.
| // Delete stream rows first and capture rows_affected so the response reflects | ||
| // the actual count (rather than a stale pre-DELETE COUNT). $params is guaranteed | ||
| // non-empty here by the count( $where ) === 1 guard above. MySQL requires the | ||
| // "DELETE alias FROM tbl AS alias" form when the WHERE references an alias. | ||
| // phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared,WordPress.DB.DirectDatabaseQuery | ||
| $delete_sql = "DELETE stream FROM {$wpdb->stream} AS stream WHERE {$where_sql}"; | ||
| // phpcs:ignore WordPress.DB.DirectDatabaseQuery,WordPress.DB.PreparedSQL.NotPrepared | ||
| $wpdb->query( $wpdb->prepare( $delete_sql, $params ) ); | ||
| $deleted = (int) $wpdb->rows_affected; | ||
|
|
||
| if ( 0 === $deleted ) { | ||
| return array( 'deleted' => 0 ); | ||
| } | ||
|
|
||
| // Sweep orphaned meta rows whose parent record was just deleted. Idempotent | ||
| // and safe to run unconditionally; the LEFT JOIN scopes the cleanup to | ||
| // orphans across the whole streammeta table, which also catches any prior | ||
| // orphans without growing this query's blast radius beyond a single sweep. | ||
| // phpcs:ignore WordPress.DB.DirectDatabaseQuery,WordPress.DB.PreparedSQL.InterpolatedNotPrepared,WordPress.DB.PreparedSQL.NotPrepared | ||
| $wpdb->query( | ||
| "DELETE meta FROM {$wpdb->streammeta} AS meta | ||
| LEFT JOIN {$wpdb->stream} AS stream ON stream.ID = meta.record_id | ||
| WHERE stream.ID IS NULL" | ||
| ); | ||
|
|
| 'properties' => array( | ||
| 'deleted' => array( | ||
| 'type' => 'integer', | ||
| 'description' => 'Number of stream records deleted (meta rows are cascaded by record_id).', |
| // the actual count (rather than a stale pre-DELETE COUNT). $params is guaranteed | ||
| // non-empty here by the count( $where ) === 1 guard above. MySQL requires the | ||
| // "DELETE alias FROM tbl AS alias" form when the WHERE references an alias. |
| * Verifies that abilities registered through Abilities API actually serve HTTP | ||
| * requests at /wp-abilities/v1/abilities/stream/{slug}/run with correct status | ||
| * codes and method routing. Complements the per-ability unit tests, which only | ||
| * exercise execute() in isolation. |
…atement Replace the post-DELETE orphan sweep over the entire streammeta table with a single multi-table DELETE that removes matching stream rows and their meta in one statement, mirroring Admin::purge_scheduled_action(). Capture the parent count up-front so the response still reports records-deleted independent of how many meta rows were attached. Also drop a stale comment that referenced a guard pattern no longer in the code, and update the output-schema description so it matches the implementation (no more 'cascade' wording, since there is no FK).
| // Refresh in-memory copy so subsequent abilities see the change. | ||
| $this->plugin->settings->options = $merged; | ||
|
|
||
| return $merged; |
| 'properties' => array( | ||
| 'settings' => array( | ||
| 'type' => 'object', | ||
| 'description' => 'Partial settings map keyed by {section}_{field} (e.g. general_records_ttl). Unknown keys are rejected; values are normalized through Stream\'s settings sanitizer. Omitted keys are preserved.', |
| $options['exclude_rules'] = $rules; | ||
| update_option( $option_key, $options ); | ||
|
|
||
| // Refresh in-memory copy. | ||
| $this->plugin->settings->options = $options; | ||
|
|
||
| return array( | ||
| 'index' => $index, | ||
| 'rule' => $rule, | ||
| ); |
| return array( | ||
| 'type' => 'object', | ||
| 'additionalProperties' => true, | ||
| 'description' => 'Settings keyed by {section}_{field} (e.g. general_records_ttl, advanced_enable_abilities_api).', |
…hten schema descriptions After update_option() the raw option array is sparse (omits defaults). Both update-settings and create-exclusion-rule were assigning that sparse array directly to $plugin->settings->options, leaving default-only keys missing for any later code in the same request. Refresh the in-memory copy via Settings::get_options() so defaults are merged in. Also align two schema descriptions with actual behavior: - update-settings now says unknown keys are *ignored* (the request only fails when no key matches a registered setting). This matches the array_intersect_key() filtering in execute(). - get-settings no longer promises advanced_enable_abilities_api in every response; that field is only registered on WP 6.9+ and, on network-activated multisite, only from network admin.
When a wp_stream_alerts post has no alert_meta postmeta row, the ability
was emitting an empty PHP array(), which JSON-encodes as [] and violates
the declared 'type: object' output schema.
Replace the empty array with a stdClass instance so wp_json_encode()
emits {} as the output schema requires. Also strengthen the regression
test to assert against the JSON-encoded payload (the previous PHP-level
array() comparison was passing despite the wire-format bug).
Squiz.Commenting.FunctionComment.WrongStyle was flagging the three test methods that had only inline section comments above them, and Generic.Commenting.DocComment.MissingShort was flagging the @expectedIncorrectUsage-only docblock on test_unknown_ability_returns_404.
…empotent category, doc fix Four issues from the latest Copilot review on this branch: - update-settings: PHP booleans on checkbox keys were silently coerced to '' by Settings::sanitize_setting_by_field_type() (it gates on is_numeric()). JSON clients naturally send true/false for checkbox-typed settings, so the round-trip would store '' instead of 0/1. Walk the registered fields to identify checkbox keys and normalize bools to 0/1 before sanitization. Add a regression test that round-trips both true and false. - purge-records: the multi-table DELETE result was discarded, so a database-side failure (lock-wait timeout, deadlock, etc.) would still return the pre-counted 'deleted' as if the purge had succeeded. Check $wpdb->query()'s return value and surface a 500 WP_Error on false. - class-abilities: register_category() now bails when the category is already registered, mirroring the idempotency pattern in register_abilities() and avoiding a core _doing_it_wrong notice when multiple loader instances exist (which the test harness already works around). - get-records: the orderby description claimed unknown values fall back to ID in Query::query(), but the schema enum rejects them at REST validation. Tighten the description to match.
bartoszgadomski
left a comment
bartoszgadomski
left a comment
There was a problem hiding this comment.
@PatelUtkarsh Thank you for working on this exciting new feature!
Please see my feedback below - it's mostly related to separation of concerns and de-duplicating logic.
Keep abilities thin and delegate to the existing data-flow classes so the admin UI and the Abilities API stay on one code path. - Alerts: add STATUS_ENABLED / STATUS_DISABLED constants and a get_alerts() listing method; consume both from get-alerts ability. - Alert: add delete() method; consume from delete-alert ability. - create-alert ability now delegates the insert + meta save to Alert::save() instead of duplicating wp_insert_post / update_post_meta. - Connectors: add get_all() and get_slugs() helpers; consume from get-connectors and create-exclusion-rule abilities. - Record: add static get_by_id($id, $blog_id) for single-row + meta fetch; consume from get-record ability (multisite scoping kept in the ability layer, where the REST/network-admin distinction belongs). - Settings: add get_setting_value() that transparently reads the network option on network-activated multisite; Abilities loader uses it instead of its own multisite branching.
|
Thanks for the thorough review! @bartoszgadomski Addressed all the feedback in 8e4637a. The main theme was routing abilities through existing classes |
bartoszgadomski
left a comment
bartoszgadomski
left a comment
There was a problem hiding this comment.
@PatelUtkarsh Thank you for implementing the requested changes!
I have a few more findings - note these are AI generated, so please double-check whether there's no any false alarms. Beside the inline comments shared below for critical issues, here are other findings:
Medium
advanced_enable_abilities_api is unreachable through stream/update-settings on network-activated multisite
Settings::get_fields() only appends the toggle when ! is_network_activated() || is_network_admin(). REST is never is_network_admin(), so on a network-activated install the field is missing from get_fields() everywhere except the network-admin settings screen. update-settings builds its allowlist from get_fields(), so the key gets array_intersect_key-stripped and the call fails with stream_no_valid_settings. This is arguably intentional (force the toggle through the network admin UI), but it should be either documented or — better — whitelisted explicitly so an MCP client can still flip it. Same gap applies to get-settings, where the toggle's current value will never appear in the response on network-activated installs.
stream/get-connectors returns a frontend-only subset, breaking stream/create-exclusion-rule validation
Connectors::load_connectors() honors register_frontend = false and skips those connectors when ! is_admin() — REST satisfies ! is_admin(). The following connectors set register_frontend = false: blogs, taxonomies, settings, editor, menus, installer, jetpack, mercator. Two visible consequences:
get-connectorsreturns a list that's a strict subset of the connectors an admin sees in the UI, which is misleading for AI clients trying to enumerate what's tracked on the site.create-exclusion-rulevalidatesconnectoragainst$plugin->connectors->get_slugs(). Trying to excludeconnector: "settings"orconnector: "editor"from the REST API will hitstream_unknown_connector, even though those connectors are real, log records, and are valid in the UI.
The cleanest fix is to either force register_frontend = true for connectors that should be discoverable via abilities, or to drive the validation list from a non-context-dependent source.
update-settings re-triggers the TTL purge hook
Settings::__construct() hooks update_option_$option_key → updated_option_ttl_remove_records. update-settings writes through update_option, so any REST call that changes general_records_ttl (or even just touches the option) will fire that hook in the REST request. Likely desired, but worth confirming — historically that hook scheduled an async cascade purge, and it now runs in user-facing REST latency. Either reuse the existing path explicitly or document.
Lower priority / nits
-
Ability_Get_Alerts::execute()casts(string) $alert->statusand returns it. The schema enums to[STATUS_ENABLED, STATUS_DISABLED], butAlert::__constructactually keeps whatever the post status is (e.g.trash). Since the listing only fetcheswp_stream_enabled/wp_stream_disabledfromAlerts::get_alerts(), this is fine today — but if a third-party plugin trashes an alert and you later expandget_alerts()to include other statuses, the output will violate the enum. Cheap defensive coercion or anif ( ! in_array( $status, ..., true ) ) continue;would future-proof this. -
Ability_Create_Alertoutput schema vs. reality: the response buildsalert_typeviaget_post_meta(...)which returns''(notnull) when meta is missing. Schema istype: [string, null], so an empty string is fine — butalert_metais cast(array) get_post_meta(...)and ifAlert::save()ever leaves meta unsaved (e.g. a failedupdate_post_meta),(array) ''becomes[ 0 => '' ], a list. Same JSON-shape bug class as theget-recordmeta shape. Currently unreachable becausesave()always writes a non-emptyalert_meta, but worth a defensive normalize before returning. -
Trait_View_Stream_Permissionlives inabilities/trait-view-stream-permission.phpand isrequire_once'd at the top of every ability file that uses it. With the loader'sinclude_onceand PHPUnit's per-testrequire_once, this works, but it's also fragile: rename the file or change the autoload prefix and the abilities silently lose their permission callback (and fall back to theWP_STREAM_SETTINGS_CAPABILITYdefault — which would over-restrict reads, not under-restrict). Consider moving the trait toclasses/so the existing PSR-4-ish autoloader picks it up viaTrait_View_Stream_Permission→class-trait-view-stream-permission.php, or rename to live under the existing autoload conventions. -
Abilities::register_category/register_abilities/filter_user_capscallbacks are registered asarray( $this, '...' ). The workspace coding rule (Avoid Anonymous Functions in PHP) discourages instance-method callbacks in favor of static or named functions, but the rest of Stream consistently usesarray( $this, '...' )(e.g.Settings::__construct(),Admin::__construct()). New code is therefore consistent with the repo but inconsistent with the stated standard. Worth a project-level decision before this lands; not a blocker. -
Test_Ability_Update_Settings::test_partial_update_preserves_other_keysand friends pass only because they exercise the single-site code path. Once the multisite write issue is addressed, you'll want a@group ms-requiredtest that proves a write lands inwp_stream_networkon network-activated installs (mirroringtest_is_enabled_reads_network_option_when_network_activated). -
Test_Abilities::test_load_abilities_instantiates_each_slugandtest_load_abilities_populates_all_slugsare functionally duplicates — both assert count 11 + iterate the slugs. Worth collapsing. -
Ability_Get_Settingsdoesn't override the base permission_callback, so it usesWP_STREAM_SETTINGS_CAPABILITY. The PR description says read abilities useview_streamand only write/destructive abilities use the settings cap; the doc is slightly off —get-settingsis read but gated on the settings cap. Reasonable choice (settings can include role allowlists), but worth documenting why this one read ability is treated differently. -
Ability::register()includes afinalqualifier, which is great for the contract, but it means a subclass that ever needed to inject e.g. a precondition (register_only_if_woocommerce_loaded) has nowhere to hook. Add apre_register()hook method or removefinallater if needed; minor.
| $args['records_per_page'] = self::DEFAULT_PER_PAGE; | ||
| } | ||
|
|
||
| $records = $this->plugin->db->get_records( $args ); |
There was a problem hiding this comment.
@PatelUtkarsh get-record and purge-records were given an explicit is_multisite() && ! is_network_admin() blog guard, but get-records was missed. The reasoning that protects the others applies here too, and the wp_stream_query_args filter that normally injects blog_id is never registered. A user with view_stream on any blog can call stream/get-records and receive records from every other blog.
Apply the same scoping as get-record / purge-records:
| $records = $this->plugin->db->get_records( $args ); | |
| if ( is_multisite() && ! is_network_admin() ) { | |
| $args['blog_id'] = get_current_blog_id(); | |
| } | |
| $records = $this->plugin->db->get_records( $args ); |
There was a problem hiding this comment.
Good catch! Fixed in 71d6443. Added the same is_multisite() && ! is_network_admin() guard get-record and purge-records have.
| $sanitized = $this->plugin->settings->sanitize_settings( $filtered ); | ||
| $merged = array_merge( $current, $sanitized ); | ||
|
|
||
| update_option( $option_key, $merged ); |
There was a problem hiding this comment.
@PatelUtkarsh is_enabled() method was fixed to read via Settings::get_setting_value() so it consults wp_stream_network on network-activated installs. The writes weren't; In REST on a network-activated install, Settings::get_option_key() returns wp_stream (the per-site key), but the authoritative store is wp_stream_network (written by Network::network_admin_save_options() via update_site_option).
Result: an admin who flips a setting via the API thinks it was saved, but Abilities::is_enabled(), Admin::purge_scheduled_action(), and the in-network-admin UI all still read the old value. This should route through a setter on Settings that knows about the network case.
There was a problem hiding this comment.
Good catch! Fixed in 71d6443. Added Settings::update_all_setting_values() and get_all_setting_values() that route through update_site_option/get_site_option on network-activated multisite.
| } | ||
|
|
||
| $options['exclude_rules'] = $rules; | ||
| update_option( $option_key, $options ); |
There was a problem hiding this comment.
Same concern applies here.
| return $allcaps; | ||
| } | ||
|
|
||
| $role_access = isset( $this->plugin->settings->options['general_role_access'] ) |
There was a problem hiding this comment.
Same multisite gap on the read side: the role-access dynamic cap filter reads $plugin->settings->options['general_role_access']. On network-activated multisite REST contexts, $plugin->settings->options is the (typically empty) per-site option. So users who would be granted view_stream via the network admin's Role Access setting won't get it through the abilities endpoint - read abilities silently return 403 for any non-administrator. Use Settings::get_setting_value( 'general_role_access', array() ) here, mirroring what was done for the toggle.
There was a problem hiding this comment.
Good catch! Fixed in 71d6443. filter_user_caps() now reads via Settings::get_setting_value().
| return null; | ||
| } | ||
|
|
||
| $row['meta'] = (array) get_metadata( 'record', $row['ID'] ); |
There was a problem hiding this comment.
When the record has no meta, get_metadata() returns [], which JSON-encodes as [], violating the declared meta: { type: object } in the output schema. The same 5117ef1 commit that fixed alert_meta should be applied here - coerce empty to new \stdClass() (or is_array() && ! empty() ternary).
There was a problem hiding this comment.
Fixed in 71d6443. Coerced empty meta to new \stdClass() in Ability_Get_Record::execute(). Keeps JSON-shape concerns in the ability layer and the model neutral.
…uting - Add Settings::get_all_setting_values() and update_all_setting_values() that route through the network option on network-activated multisite. Previously, update-settings and create-exclusion-rule wrote to the per-site option in REST contexts, while is_enabled() and the admin UI read from the network option. Writes ghost-saved. - Route Abilities::filter_user_caps() through get_setting_value() so general_role_access is honored on network-activated REST contexts. Previously, editors granted view_stream via the network admin's Role Access setting silently got 403 from read abilities. - Scope get-records to the current blog when is_multisite() and not in network admin. Mirrors guards already in get-record and purge-records. The wp_stream_query_args filter that normally injects this is only registered inside Admin, which doesn't load in REST. - Coerce empty record meta to stdClass in Ability_Get_Record so the response satisfies the declared meta: object output schema. Same fix class as the earlier alert_meta normalization. - Stop hiding the Abilities API toggle from network admin UI on network-activated installs (keep it hidden only from the per-site settings screen, where saving would be a no-op). REST/CLI clients can now flip it via update-settings since writes are routed correctly.
Connectors::load_connectors() skips connectors whose register_frontend is false when the request isn't wp-admin. REST is ! is_admin(), so connectors like settings, editor, menus, taxonomies, blogs, installer, jetpack, and mercator were absent from stream/get-connectors and unknown to stream/create-exclusion-rule's validation enum -- even though those connectors are real, configurable in the wp-admin UI, and log records on the site. Add Connectors::get_all_including_admin_only() and get_all_slugs_including_admin_only(), which walk the full connector class list and return metadata for every dependency-satisfied connector, ignoring the is_admin() gate. They never call register() and do not mutate the live $this->connectors registry, so hooks fire exactly as before. Route the two affected abilities through the new methods. Verified against the live xwp-demo deployment: stream/get-connectors was returning 6 entries; with this fix it returns all 14+ that an admin sees in the UI, and stream/create-exclusion-rule now accepts connector slugs like "settings".
…ultisite E2E surfaced a read/write mismatch: update-settings and create-exclusion-rule correctly wrote to wp_stream_network on network-activated installs, but get-settings and get-exclusion-rules still read from $plugin->settings->options (per-site, populated via Settings::get_options() which gates on is_network_admin() and falls back to get_option() in REST). After a write, the read endpoint returned the old per-site value instead of the just- persisted network value. Route both read abilities through Settings::get_all_setting_values() and make sure the in-memory cache that update_all_setting_values() refreshes also reads from the network option (Settings::get_options() can't be trusted on network-activated REST because is_network_admin() is always false there). Also return from update-settings via get_all_setting_values() so the response reflects the authoritative store.
- Filter Ability_Get_Alerts results to the declared status enum (STATUS_ENABLED / STATUS_DISABLED) so a third-party-trashed alert or future broadening of Alerts::get_alerts() doesn't leak a non-enum status into the response and violate the output schema. - Normalize empty alert_meta to stdClass in Ability_Create_Alert output, mirroring the coerce in Ability_Get_Alerts. Currently unreachable because Alert::save() always writes the merged trigger keys, but the coerce is cheap defense and keeps the JSON output consistent. - Add Test_Ability_Update_Settings::test_write_targets_network_option_when_network_activated -- mirror of the read-side coverage in Test_Abilities::test_is_enabled_reads_network_option_when_network_activated, proves update-settings writes land in wp_stream_network on network- activated multisite and do not touch the per-site option. - Merge duplicate Test_Abilities::test_load_abilities_instantiates_each_slug and test_load_abilities_populates_all_slugs into a single test that covers both population and instantiation in one pass.
Each of the 5 read abilities (get-records, get-record, get-alerts, get-connectors, get-exclusion-rules) previously did its own require_once for trait-view-stream-permission.php at the top of the class file. A new read ability that forgot the require would silently fall back to Ability::permission_callback() and gate on WP_STREAM_SETTINGS_CAPABILITY instead of view_stream. Move the require_once into the two chokepoints that actually load ability files: Abilities::load_abilities() for production and Abilities_TestCase::setUp() for PHPUnit. New abilities now get the trait available automatically -- no per-file require to forget. Verified via PHP introspection (trait_exists, class_uses on all 5 ability classes) and a live REST smoke test that admin GETs return 200 and subscriber GETs return 403 across all five read abilities.
|
Thanks for the second pass! All five inline points addressed in 71d6443. @bartoszgadomski Medium
Nits addressed in d1fc2e0:
Deferring:
|
Resolves XWPENG-13.
Integrates Stream with the WordPress 6.9 Abilities API. Eleven Stream operations are registered as abilities under the
stream/namespace, exposed via the core Abilities REST controller at/wp-abilities/v1/abilities/stream/{slug}/runonce a site owner explicitly opts in.Approach
Mirrors Stream's existing connector/alert plugin extension layout: an abstract base class, a loader gated on environment + opt-in setting, and one file per ability that delegates to existing Stream internals (no business logic duplicated).
Permission model — read abilities use Stream's
view_streamcapability (via a shared trait) so editors and other roles configured under "Role Access" can call them, matching the admin UI. Write and destructive abilities require the Stream settings capability (WP_STREAM_SETTINGS_CAPABILITY, filterable; defaults tomanage_options).Schemas are hand-written with AI-readable
descriptionstrings rather than auto-derived, decoupling the public API from internal arg names.Abilities registered:
stream/get-recordsview_streamstream/get-recordview_streamstream/get-settingsstream/get-alertsview_streamwp_stream_alertsposts + metastream/get-connectorsview_streamstream/get-exclusion-rulesview_streamstream/create-alertwp_stream_alertspoststream/update-settingsstream/create-exclusion-rulestream/purge-recordsstream/delete-alertwp_stream_alertspostRouting follows the WP core Abilities REST controller defaults: read-only abilities are served as
GET, write abilities asPOST, and destructive + idempotent abilities asDELETE. Non-matching methods return405 Method Not Allowed.Gating
register()checksfunction_exists( 'wp_register_ability' )defensively. No admin notice is shown (per direction).advanced_enable_abilities_apicheckbox in the existing Advanced section of Stream settings. Defaults to0. Only renders on WP 6.9+. Honors network option on network-activated multisite.Safety
view_stream; write/destructive abilities use the Stream settings capability (filterable).stream/purge-recordsrequiresconfirm: trueAND at least one filter (older_than_days,connector,context,action); a confirm-only payload returns a400WP_Errorrather than truncating the table. On any multisite request that is not running inside Network Admin (REST never is), the purge is scoped toblog_id = get_current_blog_id()so it cannot wipe other sites' records.stream/get-recordapplies the same per-blog scoping so aview_streamuser on one site can't read records from other sites by guessing IDs.stream/delete-alertis idempotent — the second call returns the same404WP_Erroras a missing ID. Refuses to delete posts that are not ofwp_stream_alertstype.stream/create-alertvalidatesalert_typeagainst registered alert types and rejects unknown types.stream/get-recordsschema tightened:orderbyis an enum,*__inarrays havemaxItemscaps.stream/create-exclusion-rulevalidates IP format at the application layer.older_than_dayscutoffs are computed as a UTCDateTimein PHP (matchingAdmin::purge_scheduled_action()) so behavior is independent of MySQL server timezone.Manual REST QA
After merging and enabling the toggle on a WP 6.9+ site:
Checklist
contributing.md).Release Changelog
stream/namespace, gated behind a new "Enable Abilities API" advanced setting (default off). Read abilities respect Stream'sview_streamcapability; write abilities require the Stream settings capability.