Lokasi ngalangkungan proxy:   [ UP ]  
[Ngawartoskeun bug]   [Panyetelan cookie]                
Skip to content

fix: accept top-level fields in v4 payment gateway PUT endpoint#63714

Merged
mordeth merged 5 commits intotrunkfrom
fix/payment-gateway-v4-top-level-fields
Mar 18, 2026
Merged

fix: accept top-level fields in v4 payment gateway PUT endpoint#63714
mordeth merged 5 commits intotrunkfrom
fix/payment-gateway-v4-top-level-fields

Conversation

@mordeth
Copy link
Copy Markdown
Contributor

@mordeth mordeth commented Mar 16, 2026

Motivation

The v4 payment gateway GET endpoint returns enabled, title, description, and order as top-level fields (e.g., { "id": "bacs", "enabled": false, ... }). However, the PUT endpoint only accepted these fields nested inside a required values parameter ({ "values": { "enabled": "yes" } }).

This shape mismatch between GET and PUT prevents @wordpress/core-data from tracking dirty state correctly. When core-data fetches a gateway record (persisted state has top-level enabled: false), then the UI edits it, the edit must use the same shape as the persisted record for dirty tracking to work. With the old API, edits had to use values.enabled (different path), so entities appeared permanently dirty — the save button never disabled when changes were reverted.

Changes

Controller (Controller.php):

  • Route args for the PUT endpoint are now derived from the schema (get_endpoint_args_for_item_schema) instead of a hardcoded required values object
  • update_item accepts enabled, title, description, and order as top-level params (matching the GET shape) in addition to nested values.*
  • Top-level params take precedence when both are present
  • The values parameter is no longer required — sending just { "enabled": true } is valid
  • Backwards compatible: existing callers sending { "values": { "enabled": "yes" } } continue to work

Tests (PaymentGatewaysSettingsControllerTest.php):

  • Replaced test_update_payment_gateway_missing_values (no longer errors) with:
    • test_update_payment_gateway_with_top_level_enabled — verifies top-level enabled works
    • test_top_level_fields_take_precedence_over_values — verifies precedence when both are sent

Test plan

  • Verify existing v4 payment gateway tests pass
  • PUT /wc/v4/settings/payment-gateways/bacs with { "enabled": true } returns 200 and enables the gateway
  • PUT /wc/v4/settings/payment-gateways/bacs with { "values": { "enabled": "yes" } } still works (backwards compat)

Refs WOOPRD-3082

The GET response returns `enabled`, `title`, `description`, and `order`
as top-level fields, but the PUT endpoint only accepted these nested
inside a required `values` parameter. This shape mismatch prevents
`@wordpress/core-data` from tracking dirty state correctly — edits
never match the persisted record shape, so entities appear permanently
dirty.

Accept these fields at the top level (matching the GET shape) while
maintaining backwards compatibility with `values.*` for existing
callers. Top-level params take precedence when both are present.
The `values` parameter is no longer required; endpoint args are now
derived from the schema.

Refs WOOPRD-3082
@github-actions github-actions Bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Mar 16, 2026
@mordeth mordeth self-assigned this Mar 16, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 16, 2026

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 16, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Controller now accepts top-level fields (enabled, title, description, order) alongside the existing values object for the v4 payment-gateway settings PUT endpoint; top-level fields take precedence. Validation/sanitization (standard vs. special fields) and deferred order persistence were added; tests expanded for new input shapes.

Changes

Cohort / File(s) Summary
Payment Gateways Controller
plugins/woocommerce/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/Controller.php
Adds acceptance of top-level fields (enabled, title, description, order) while keeping optional values; top-level fields override nested values. Implements per-field parsing/sanitization, schema-driven validation for standard vs. special fields, deferred gateway order persistence, gateway form initialization, and returns updated item via get_item_response/rest_ensure_response.
Payment Gateways Controller Tests
plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php
Renames one test and adds multiple tests covering no-params (no-op) updates, top-level field updates (enabled, title, description, order), legacy values handling (e.g., 'yes'), precedence of top-level fields over nested values, and persistence assertions.
Changelog
plugins/woocommerce/changelog/63714-fix-payment-gateway-v4-top-level-fields
Documents acceptance of top-level fields in the v4 payment gateway settings PUT endpoint and notes backward-compatible support for the values object.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Client
participant Controller
participant Schema
participant Gateway
participant Settings
Client->>Controller: PUT /settings/payment-gateways/{id} with top-level fields and/or values
Controller->>Gateway: initialize gateway form/schema
Controller->>Schema: validate & sanitize standard fields
Schema-->>Controller: validation result
Controller->>Schema: validate & sanitize special fields (if present)
Schema-->>Controller: validation result
Controller->>Settings: persist standard settings (enabled/title/description)
Controller->>Gateway: apply/update order (deferred/persist)
Controller->>Gateway: update gateway-specific values
Gateway-->>Settings: persist gateway-specific data
Controller->>Client: return updated item (get_item_response / rest_ensure_response)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: accepting top-level fields in the v4 payment gateway PUT endpoint to match the GET response shape.
Description check ✅ Passed The description is clearly related to the changeset, providing motivation (dirty state tracking issue), concrete changes, and test plan that align with the code modifications.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/payment-gateway-v4-top-level-fields
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php (1)

225-257: Assert persisted state here, not just the mutated gateway instance.

Lines 238-239 read the same in-memory gateway object that update_item() just changed, so this can still pass if the option write regresses. This block also only covers enabled, leaving the new top-level title, description, and order branches unexercised. Please re-read stored state for the assertion and add at least one string or numeric top-level case.

Based on learnings: Test behavior and interfaces, not implementation details, to ensure tests remain resilient when internal code changes. As per coding guidelines: Add unit or E2E tests where applicable.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php`
around lines 225 - 257, The test currently only inspects the in-memory gateway
object changed by update_item() (via
WC()->payment_gateways->payment_gateways()['bacs']), so change it to re-read
persisted settings from the DB and assert those values: after dispatching the
PUT in test_top_level_fields_take_precedence_over_values(), call get_option(
sprintf( 'woocommerce_%s_settings', 'bacs' ) ) (or the appropriate options key
for the gateway) and assert the persisted 'enabled' matches the top-level true;
also add at least one additional top-level field test (e.g., set 'title' => 'New
Title' and/or 'order' => 5 in the request) and assert the persisted option
contains that string/number to cover title/order branches rather than only
checking the mutated gateway instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@plugins/woocommerce/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/Controller.php`:
- Around line 224-229: The code is persisting woocommerce_gateway_order early
(using update_option when $order_value is set) before validating the rest of the
payload; change the flow so that you only mutate and call
update_option('woocommerce_gateway_order', $gateway_order) after all validations
pass and no WP_Error will be returned—collect the new $gateway_order in memory
(from $order_value / $values_to_update for $id) and defer calling update_option
until the end of the controller’s validation/processing path (apply same change
for the similar block handling lines referenced 247-264), ensuring update_option
is only invoked after successful validation of $values and the full request.

---

Nitpick comments:
In
`@plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php`:
- Around line 225-257: The test currently only inspects the in-memory gateway
object changed by update_item() (via
WC()->payment_gateways->payment_gateways()['bacs']), so change it to re-read
persisted settings from the DB and assert those values: after dispatching the
PUT in test_top_level_fields_take_precedence_over_values(), call get_option(
sprintf( 'woocommerce_%s_settings', 'bacs' ) ) (or the appropriate options key
for the gateway) and assert the persisted 'enabled' matches the top-level true;
also add at least one additional top-level field test (e.g., set 'title' => 'New
Title' and/or 'order' => 5 in the request) and assert the persisted option
contains that string/number to cover title/order branches rather than only
checking the mutated gateway instance.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 2d0de554-f343-45b5-944f-85748b0de18f

📥 Commits

Reviewing files that changed from the base of the PR and between bf365c7 and d757e49.

📒 Files selected for processing (2)
  • plugins/woocommerce/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/Controller.php
  • plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php

Using get_endpoint_args_for_item_schema() caused REST API to add
validate/sanitize callbacks to the values parameter. The multi-type
additionalProperties schema (string|number|array|boolean) triggered
sanitization that corrupted field values, causing strtolower() to
receive arrays instead of strings.

Replace with explicit arg definitions without sanitize callbacks since
the controller handles all validation and sanitization internally.

Also defer woocommerce_gateway_order update_option until after all
validation passes, preventing partial writes on validation errors.

Add tests for top-level title and order fields with DB persistence
assertions.

Refs WOOPRD-3082
@mordeth mordeth added this to the 10.7.0 milestone Mar 17, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php (1)

219-304: Add a regression for the legacy 'yes' payload too.

The new coverage only exercises boolean payloads, but this change also promises that {"values":{"enabled":"yes"}} remains supported. A focused test for that historical wire format will protect the backwards-compatible path that motivated the route-arg change.

🧪 Example test
+	public function test_update_payment_gateway_with_legacy_enabled_string() {
+		$request = new WP_REST_Request( 'PUT', self::ENDPOINT . '/bacs' );
+		$request->set_param(
+			'values',
+			array(
+				'enabled' => 'yes',
+			)
+		);
+		$response = $this->server->dispatch( $request );
+
+		$this->assertSame( 200, $response->get_status() );
+		$this->assertTrue( $response->get_data()['enabled'] );
+
+		$saved_settings = get_option( 'woocommerce_bacs_settings' );
+		$this->assertSame( 'yes', $saved_settings['enabled'] );
+	}

As per coding guidelines, code changes must ensure backwards compatibility and add unit or E2E tests where applicable.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php`
around lines 219 - 304, Add a new unit test that verifies legacy "yes" string
payload in the values map is accepted and persisted: create a test (e.g.
test_values_enabled_accepts_legacy_yes) that issues a PUT to the
PaymentGatewaysSettingsController endpoint (use the same pattern as
test_top_level_fields_take_precedence_over_values) but sets
$request->set_param('values', array('enabled' => 'yes')), dispatches via
$this->server->dispatch($request), asserts a 200 response and that the response
data/enabled and persisted option
(get_option('woocommerce_bacs_settings')['enabled']) remain 'yes'; follow
existing tests for request construction and assertions to match style and
cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@plugins/woocommerce/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/Controller.php`:
- Around line 236-247: The title and description branches are bypassing schema
sanitization by consuming $params and unsetting $values_to_update entries;
instead of assigning/sanitizing here, copy any top-level $params['title'] and
$params['description'] into $values_to_update['title'] and
$values_to_update['description'] (if present) so that
validate_and_sanitize_settings() handles validation/sanitization; remove the
sanitize_text_field/wp_kses_post assignments and the unset(...) calls so the
existing validate_setting_text_field path (and function
validate_and_sanitize_settings) is used to normalize and persist these fields on
$gateway.

---

Nitpick comments:
In
`@plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php`:
- Around line 219-304: Add a new unit test that verifies legacy "yes" string
payload in the values map is accepted and persisted: create a test (e.g.
test_values_enabled_accepts_legacy_yes) that issues a PUT to the
PaymentGatewaysSettingsController endpoint (use the same pattern as
test_top_level_fields_take_precedence_over_values) but sets
$request->set_param('values', array('enabled' => 'yes')), dispatches via
$this->server->dispatch($request), asserts a 200 response and that the response
data/enabled and persisted option
(get_option('woocommerce_bacs_settings')['enabled']) remain 'yes'; follow
existing tests for request construction and assertions to match style and
cleanup.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: f0d34a3e-c5c3-4350-83d8-5b77907253b6

📥 Commits

Reviewing files that changed from the base of the PR and between d757e49 and 0d171df.

📒 Files selected for processing (3)
  • plugins/woocommerce/changelog/63714-fix-payment-gateway-v4-top-level-fields
  • plugins/woocommerce/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/Controller.php
  • plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php

All parameters are now optional, so a PUT with no params should return
200 and leave settings unchanged. The old test asserting 400 was removed
when required params were dropped, but the replacement behavior had no
coverage.

Refs WOOPLUG-6371
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php (1)

290-331: Add a top-level description test to complete field coverage

The PR behavior includes top-level description, but this changed test set only adds top-level coverage for enabled, title, and order. Please add one test for top-level description (and ideally a precedence case vs values.description) to fully lock the contract.

Proposed test addition
+	/**
+	 * Test updating a payment gateway with top-level description field.
+	 */
+	public function test_update_payment_gateway_with_top_level_description() {
+		$request = new WP_REST_Request( 'PUT', self::ENDPOINT . '/bacs' );
+		$request->set_param( 'description', '<strong>Pay</strong> via transfer' );
+		$response = $this->server->dispatch( $request );
+
+		$this->assertSame( 200, $response->get_status() );
+		$data = $response->get_data();
+		$this->assertStringContainsString( 'Pay', $data['description'] );
+
+		$saved_settings = get_option( 'woocommerce_bacs_settings' );
+		$this->assertArrayHasKey( 'description', $saved_settings );
+	}

As per coding guidelines "Add unit or E2E tests where applicable."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php`
around lines 290 - 331, Add a new test method on
PaymentGatewaysSettingsControllerTest named something like
test_update_payment_gateway_with_top_level_description that mirrors
test_update_payment_gateway_with_top_level_title/order: send a WP_REST_Request
PUT to self::ENDPOINT . '/bacs' with set_param('description', '...'), assert
response status 200 and that $response->get_data()['description'] equals the
value, then verify persisted state via
get_option('woocommerce_bacs_settings')['description']; also add a second
assertion case to verify precedence where you send both top-level 'description'
and a nested values['description'] and assert the top-level field takes
precedence in the response and persisted option.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php`:
- Around line 290-331: Add a new test method on
PaymentGatewaysSettingsControllerTest named something like
test_update_payment_gateway_with_top_level_description that mirrors
test_update_payment_gateway_with_top_level_title/order: send a WP_REST_Request
PUT to self::ENDPOINT . '/bacs' with set_param('description', '...'), assert
response status 200 and that $response->get_data()['description'] equals the
value, then verify persisted state via
get_option('woocommerce_bacs_settings')['description']; also add a second
assertion case to verify precedence where you send both top-level 'description'
and a nested values['description'] and assert the top-level field takes
precedence in the response and persisted option.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: b5f500cc-fe3a-4e05-8e28-0ffc3f52296e

📥 Commits

Reviewing files that changed from the base of the PR and between 0d171df and d222a31.

📒 Files selected for processing (1)
  • plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php

The previous test compared get_option() before and after the PUT, but
the option may not exist before the first PUT call in a clean CI
environment — causing assertSame(false, array(...)) to fail.

Compare gateway object properties (enabled, title) instead, which are
always available. Also fix PHPCS equals sign alignment warnings.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php (1)

308-329: LGTM!

Proper test setup with delete_option( 'woocommerce_gateway_order' ) to ensure a clean state, followed by verification that the order value is persisted to the correct option.

Consider adding a test for the top-level description field for completeness, since it's one of the four top-level fields mentioned in the PR objectives. The existing test_update_payment_gateway_sanitizes_text_fields covers description through values.*, but a dedicated top-level test would provide full parity.

📝 Optional: Add test for top-level description
/**
 * Test updating a payment gateway with top-level description field.
 */
public function test_update_payment_gateway_with_top_level_description() {
    // Act.
    $request = new WP_REST_Request( 'PUT', self::ENDPOINT . '/bacs' );
    $request->set_param( 'description', 'Pay directly to our bank account.' );
    $response = $this->server->dispatch( $request );

    // Assert.
    $this->assertSame( 200, $response->get_status() );

    $data = $response->get_data();
    $this->assertSame( 'Pay directly to our bank account.', $data['description'] );

    // Verify persisted state.
    $saved_settings = get_option( 'woocommerce_bacs_settings' );
    $this->assertSame( 'Pay directly to our bank account.', $saved_settings['description'] );
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php`
around lines 308 - 329, Add a new unit test that mirrors
test_update_payment_gateway_with_top_level_order but targets the top-level
description field: create a WP_REST_Request PUT to self::ENDPOINT . '/bacs',
set_param('description', '<text>'), dispatch via $this->server->dispatch, assert
status 200, assert the response data contains the same description, and verify
persistence by checking get_option('woocommerce_bacs_settings')['description'];
place the new test method named
test_update_payment_gateway_with_top_level_description alongside the other
PaymentGatewaysSettingsControllerTest methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php`:
- Around line 308-329: Add a new unit test that mirrors
test_update_payment_gateway_with_top_level_order but targets the top-level
description field: create a WP_REST_Request PUT to self::ENDPOINT . '/bacs',
set_param('description', '<text>'), dispatch via $this->server->dispatch, assert
status 200, assert the response data contains the same description, and verify
persistence by checking get_option('woocommerce_bacs_settings')['description'];
place the new test method named
test_update_payment_gateway_with_top_level_description alongside the other
PaymentGatewaysSettingsControllerTest methods.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: dfa17dcc-4d22-472a-83cd-5d0d8be8e015

📥 Commits

Reviewing files that changed from the base of the PR and between d222a31 and 0856bd3.

📒 Files selected for processing (1)
  • plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php

Copy link
Copy Markdown
Contributor

@oaratovskyi oaratovskyi left a comment

Choose a reason for hiding this comment

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

Hey @mordeth , nice work on this one. Here's what stood out:

  • The core problem is well-understood — GET/PUT shape mismatch breaking core-data dirty tracking is a real pain, and this fix addresses it cleanly
  • Backwards compatibility is solidvalues still works, just no longer required. Existing callers won't break
  • Sanitization is correct — each field uses the right sanitizer (wc_bool_to_string, sanitize_text_field, wp_kses_post, absint)
  • Order update deferral is a genuine improvement — moving the write to after validation prevents partial DB state on failure
  • Two minor test coverage gapsdescription field and multi-field precedence testing (details below)

Verdict: APPROVE (with minor suggestions)

All four specialist reviewers (PR, security, WP architecture, PHP tests) found no correctness or security issues. The only feedback is about test coverage completeness.


Detailed findings (2 items)

Consider: Missing test for top-level description field

File: PaymentGatewaysSettingsControllerTest.php ~line 250
What: Tests exist for enabled, title, and order as top-level PUT params, but not for description. Since description uses wp_kses_post (which preserves safe HTML) while title uses sanitize_text_field (which strips all HTML), they exercise different sanitization paths.

Suggestion: What if we added a test_update_payment_gateway_with_top_level_description that sends a description at the top level and verifies both the API response and persisted woocommerce_bacs_settings['description']? Bonus points for also verifying <script> tags are stripped while <em> survives.

Consider: Precedence test only covers enabled

File: PaymentGatewaysSettingsControllerTest.php ~line 270
What: test_top_level_fields_take_precedence_over_values only tests the enabled conflict scenario. The same $params[field] ?? $values_to_update[field] pattern applies to title, description, and order — a typo in one block wouldn't be caught.

Suggestion: Extending the test to also send a conflicting title (top-level vs values.title) would cover string fields too. One extra field is enough.

@oaratovskyi
Copy link
Copy Markdown
Contributor

Checked it works both for new data format and old too :shipit:

- Assert persisted state (get_option) instead of in-memory gateway
  object in the top-level enabled test
- Add top-level description field test for complete field coverage
- Add legacy values.enabled='yes' regression test for backwards compat
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php (2)

225-242: Strengthen the no-op test by asserting persisted settings are unchanged.

This currently validates only in-memory gateway properties. Adding an option snapshot check will better catch persistence regressions for empty PUT requests.

♻️ Proposed test hardening
 public function test_update_payment_gateway_with_no_params() {
 	// Arrange.
 	$gateway        = WC()->payment_gateways->payment_gateways()['bacs'];
 	$enabled_before = $gateway->enabled;
 	$title_before   = $gateway->title;
+	$saved_before   = get_option( 'woocommerce_bacs_settings' );

 	// Act.
 	$request  = new WP_REST_Request( 'PUT', self::ENDPOINT . '/bacs' );
 	$response = $this->server->dispatch( $request );

 	// Assert.
 	$this->assertSame( 200, $response->get_status() );

 	// Verify gateway state was not changed.
 	$gateway_after = WC()->payment_gateways->payment_gateways()['bacs'];
 	$this->assertSame( $enabled_before, $gateway_after->enabled );
 	$this->assertSame( $title_before, $gateway_after->title );
+	$saved_after = get_option( 'woocommerce_bacs_settings' );
+	$this->assertSame( $saved_before, $saved_after );
 }

Based on learnings: Test behavior and interfaces, not implementation details, to ensure tests remain resilient when internal code changes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php`
around lines 225 - 242, The test test_update_payment_gateway_with_no_params only
asserts in-memory gateway properties; capture the persisted settings for the
BACS gateway before the PUT (e.g. $before_option =
get_option('woocommerce_bacs_settings')) and after the request, then assert they
are unchanged (assertSame($before_option, $after_option)); update the test to
take this option snapshot in addition to the existing WC()->payment_gateways
checks so persistence regressions are caught.

313-329: Consider covering precedence for all top-level fields, not only enabled.

The precedence rule applies to enabled, title, description, and order; adding the remaining cases would better lock the API contract.

As per coding guidelines: Ensure code is backwards compatible and add unit or E2E tests where applicable.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php`
around lines 313 - 329, Extend the existing
test_top_level_fields_take_precedence_over_values to also assert that top-level
title, description, and order override values in the nested values array: send a
PUT to self::ENDPOINT . '/bacs' with top-level params 'title', 'description',
and 'order' alongside a 'values' array containing conflicting
title/description/order, dispatch via $this->server->dispatch($request), then
assert response status 200 and that $data['title'], $data['description'], and
$data['order'] match the top-level inputs; finally fetch persisted settings via
get_option('woocommerce_bacs_settings') and assert the stored ['title'],
['description'], and ['order'] reflect the top-level values (same pattern used
for 'enabled').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php`:
- Around line 225-242: The test test_update_payment_gateway_with_no_params only
asserts in-memory gateway properties; capture the persisted settings for the
BACS gateway before the PUT (e.g. $before_option =
get_option('woocommerce_bacs_settings')) and after the request, then assert they
are unchanged (assertSame($before_option, $after_option)); update the test to
take this option snapshot in addition to the existing WC()->payment_gateways
checks so persistence regressions are caught.
- Around line 313-329: Extend the existing
test_top_level_fields_take_precedence_over_values to also assert that top-level
title, description, and order override values in the nested values array: send a
PUT to self::ENDPOINT . '/bacs' with top-level params 'title', 'description',
and 'order' alongside a 'values' array containing conflicting
title/description/order, dispatch via $this->server->dispatch($request), then
assert response status 200 and that $data['title'], $data['description'], and
$data['order'] match the top-level inputs; finally fetch persisted settings via
get_option('woocommerce_bacs_settings') and assert the stored ['title'],
['description'], and ['order'] reflect the top-level values (same pattern used
for 'enabled').

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: ad2202cd-e4e5-47c8-a557-c5b014675154

📥 Commits

Reviewing files that changed from the base of the PR and between 0856bd3 and 42da656.

📒 Files selected for processing (1)
  • plugins/woocommerce/tests/php/src/Internal/RestApi/Routes/V4/Settings/PaymentGateways/PaymentGatewaysSettingsControllerTest.php

@mordeth
Copy link
Copy Markdown
Contributor Author

mordeth commented Mar 18, 2026

Addressed the review feedback. Thanks for the reviews!

@mordeth mordeth merged commit 9057c2d into trunk Mar 18, 2026
65 of 67 checks passed
@mordeth mordeth deleted the fix/payment-gateway-v4-top-level-fields branch March 18, 2026 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants