Skip to content

Commit d1fc2e0

Browse files
committed
Address review-body nits: defensive output coercion and test cleanup
- 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.
1 parent 9f5fc08 commit d1fc2e0

4 files changed

Lines changed: 99 additions & 14 deletions

File tree

abilities/class-ability-create-alert.php

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,12 +181,22 @@ public function execute( $input = null ) {
181181
);
182182
}
183183

184+
// Normalize alert_meta to stdClass when empty so the response satisfies
185+
// the declared object output schema. Reachable in theory if a future
186+
// Alert::save() change ever leaves alert_meta unwritten -- today the
187+
// call always persists the merged trigger keys, but the coerce is cheap
188+
// defense and keeps the JSON output consistent with get-alerts.
189+
$persisted_meta = get_post_meta( $post_id, 'alert_meta', true );
190+
$alert_meta = is_array( $persisted_meta ) && ! empty( $persisted_meta )
191+
? $persisted_meta
192+
: new \stdClass();
193+
184194
return array(
185195
'id' => (int) $post_id,
186196
'status' => (string) get_post_status( $post_id ),
187197
'title' => (string) get_the_title( $post_id ),
188198
'alert_type' => get_post_meta( $post_id, 'alert_type', true ),
189-
'alert_meta' => (array) get_post_meta( $post_id, 'alert_meta', true ),
199+
'alert_meta' => $alert_meta,
190200
);
191201
}
192202
}

abilities/class-ability-get-alerts.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,18 @@ public function execute( $input = null ) {
114114

115115
$alerts = $this->plugin->alerts->get_alerts( $statuses );
116116

117+
// Output enum is fixed (STATUS_ENABLED / STATUS_DISABLED). Alerts::get_alerts()
118+
// only fetches those two statuses today, but a third party could trash an
119+
// alert post or wire up a custom status. Defensive filter keeps the
120+
// response schema-valid for any downstream changes.
121+
$allowed_statuses = array( Alerts::STATUS_ENABLED, Alerts::STATUS_DISABLED );
122+
117123
$out = array();
118124
foreach ( $alerts as $alert ) {
125+
if ( ! in_array( (string) $alert->status, $allowed_statuses, true ) ) {
126+
continue;
127+
}
128+
119129
// Alert::$alert_meta defaults to array(); an empty PHP array() also
120130
// JSON-encodes as a list ([]), which violates the declared object
121131
// output schema. Normalize empty/non-array values to a real object

tests/phpunit/abilities/test-class-ability-update-settings.php

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,4 +151,72 @@ public function test_unknown_keys_are_dropped_when_mixed_with_valid() {
151151
$stored = (array) get_option( $option_key );
152152
$this->assertArrayNotHasKey( 'malicious_key', $stored );
153153
}
154+
155+
/**
156+
* On network-activated multisite, writes must land in wp_stream_network
157+
* (the authoritative store that is_enabled() and the admin UI read from),
158+
* NOT in the per-site wp_stream option. The ability runs in REST where
159+
* is_network_admin() is always false, so without Settings::update_all_setting_values()
160+
* routing the write would silently target the per-site option and ghost-
161+
* save. Mirrors the read-side coverage in
162+
* Test_Abilities::test_is_enabled_reads_network_option_when_network_activated.
163+
*
164+
* @group ms-required
165+
*/
166+
public function test_write_targets_network_option_when_network_activated() {
167+
if ( ! is_multisite() ) {
168+
$this->markTestSkipped( 'Requires multisite.' );
169+
}
170+
171+
wp_set_current_user( $this->admin_user_id );
172+
173+
$network_key = $this->plugin->settings->network_options_key;
174+
$per_site_key = $this->plugin->settings->option_key;
175+
$original_network = get_site_option( $network_key, false );
176+
$original_site = get_option( $per_site_key, false );
177+
178+
// Force network-activated so update_all_setting_values() chooses the
179+
// site-option write path.
180+
add_filter( 'wp_stream_is_network_activated', '__return_true' );
181+
182+
// Reset both stores to a known baseline.
183+
update_site_option( $network_key, array( 'general_records_ttl' => 30 ) );
184+
update_option( $per_site_key, array( 'general_records_ttl' => 30 ) );
185+
186+
try {
187+
$result = $this->ability->execute(
188+
array(
189+
'settings' => array( 'general_records_ttl' => 90 ),
190+
)
191+
);
192+
193+
$this->assertIsArray( $result, 'Update must succeed.' );
194+
195+
$stored_network = (array) get_site_option( $network_key, array() );
196+
$stored_per_site = (array) get_option( $per_site_key, array() );
197+
198+
$this->assertSame(
199+
90,
200+
(int) ( $stored_network['general_records_ttl'] ?? 0 ),
201+
'Write must land in the network option on network-activated multisite.'
202+
);
203+
$this->assertSame(
204+
30,
205+
(int) ( $stored_per_site['general_records_ttl'] ?? 0 ),
206+
'Per-site option must NOT be touched on network-activated multisite.'
207+
);
208+
} finally {
209+
remove_filter( 'wp_stream_is_network_activated', '__return_true' );
210+
if ( false === $original_network ) {
211+
delete_site_option( $network_key );
212+
} else {
213+
update_site_option( $network_key, $original_network );
214+
}
215+
if ( false === $original_site ) {
216+
delete_option( $per_site_key );
217+
} else {
218+
update_option( $per_site_key, $original_site );
219+
}
220+
}//end try
221+
}
154222
}

tests/phpunit/test-class-abilities.php

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -217,20 +217,17 @@ public function test_load_abilities_instantiates_each_slug() {
217217

218218
$this->assertCount( 11, $abilities->abilities );
219219

220-
foreach ( $abilities->abilities as $name => $ability ) {
221-
$this->assertInstanceOf( __NAMESPACE__ . '\\Ability', $ability );
222-
$this->assertSame( $name, $ability->get_name() );
223-
}
224-
}
225-
226-
public function test_load_abilities_populates_all_slugs() {
227-
$abilities = new Abilities( $this->plugin );
228-
229-
$abilities->load_abilities();
230-
231-
$this->assertCount( 11, $abilities->abilities );
220+
// Every slug declared by the loader must produce an Ability instance
221+
// keyed under "stream/{slug}", and each instance must report the
222+
// matching get_name(). Combines the population check (key presence)
223+
// with the instantiation check (instanceof + get_name) in one pass.
232224
foreach ( $abilities->get_ability_slugs() as $slug ) {
233-
$this->assertArrayHasKey( 'stream/' . $slug, $abilities->abilities );
225+
$expected_name = 'stream/' . $slug;
226+
$this->assertArrayHasKey( $expected_name, $abilities->abilities );
227+
228+
$ability = $abilities->abilities[ $expected_name ];
229+
$this->assertInstanceOf( __NAMESPACE__ . '\\Ability', $ability );
230+
$this->assertSame( $expected_name, $ability->get_name() );
234231
}
235232
}
236233

0 commit comments

Comments
 (0)