Skip to content

Commit 2481bd8

Browse files
committed
Fix - Scope nexus address validation to the fields the value object reads.
Addresses review feedback on the TaxJar request seam. Two regressions, both introduced by validating the nexus address through the shared address schema. The origin state is now required for the US only. Address::validate() defaults to requiring country and state and errors on present-but-blank, where the loop it replaced only errored when the key was absent. The nexus address this plugin builds for the merchant's own store always carries a 'state' key, and WC()->countries->get_base_state() is '' for 19 of the 31 countries in get_supported_countries() -- GB, FR, NL, BE, DK, SE, PL, PT and the rest of the stateless EU. Every tax calculation for those merchants therefore dropped the store's own nexus block from the request and wrote a forced error log plus a persistent admin error notice. The US-only boundary is measured, not assumed. Probed against the live TaxJar API (scripts under .issues/WOOTAX-313/work/): GB, FR, NL, DK, DE, ES, IT, IE, CA and AU all return an identical rate whether the nexus carries its state or an empty string, because VAT and GST are rated from the destination. A US nexus with a blank state returns HTTP 200 with has_nexus:false and zero tax -- it fails silently, so the check has to stay there. Keying the requirement off whether WooCommerce models states for the country would have kept wrongly rejecting DE, ES, IT, IE, GR, HU, HR, RO, BG, CA and AU, which matters because TaxJar supports those markets for legacy accounts even though it documents US/CA only. The non-scalar guard no longer inspects unknown keys. It iterated every key of the filter-supplied address, so a custom key holding an array rejected the whole nexus address -- silently changing which nexus TaxJar was asked about, and logging "field must be a string" for a field that is never cast to string. Only the six fields the value object actually reads are checked; everything else is merged back untouched, which is what the documented array-in / array-out contract promises. Adds eleven tests. Nine non-US store countries and the unknown-key case fail before this change; the US blank-state case bounds the relaxation and passes either way.
1 parent 10234f8 commit 2481bd8

2 files changed

Lines changed: 186 additions & 3 deletions

File tree

classes/class-wc-connect-taxjar-integration.php

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1444,6 +1444,14 @@ function ( $line_item ) use ( $new_tax_rate ) {
14441444
* rules the rest of the integration uses, and — because `calculate_tax()` sends
14451445
* the normalised address — is checked in the shape it is sent.
14461446
*
1447+
* Country is always required. State is required for the US only; see the inline
1448+
* comment for the measurement behind that, and for why a blanket requirement
1449+
* rejects the plugin's own nexus across most of TaxJar's supported list.
1450+
*
1451+
* Validation is deliberately confined to the fields the value object reads. Extra
1452+
* keys supplied by a filter are neither validated nor rewritten — `calculate_tax()`
1453+
* passes them straight through to the request body.
1454+
*
14471455
* @param array $address Nexus address, as returned by `woocommerce_taxjar_nexus_address`.
14481456
*
14491457
* @return bool
@@ -1463,17 +1471,52 @@ private function is_nexus_address_valid( $address ): bool {
14631471
return false;
14641472
}
14651473

1466-
// The value object casts each field to string, so reject anything that cannot
1474+
// The value object casts these fields to string, so reject anything that cannot
14671475
// survive that cast rather than triggering an array-to-string conversion.
1468-
foreach ( $address as $field => $value ) {
1476+
//
1477+
// Only the fields the value object reads are checked. Any other key a filter
1478+
// added is passed through untouched below and is never cast, so it carries no
1479+
// conversion risk — rejecting the whole address over it would turn the
1480+
// documented array-in / array-out contract into a whitelist.
1481+
foreach ( array( 'id', 'country', 'state', 'zip', 'city', 'street' ) as $field ) {
1482+
if ( ! array_key_exists( $field, $address ) ) {
1483+
continue;
1484+
}
1485+
1486+
$value = $address[ $field ];
1487+
14691488
if ( null !== $value && ! is_scalar( $value ) ) {
14701489
$this->logger->error( 'Nexus Address ERRORS: [' . $field . '] field must be a string' . PHP_EOL . 'Nexus address removed from request body.' . PHP_EOL . wp_json_encode( $address ), 'WCS Tax' );
14711490

14721491
return false;
14731492
}
14741493
}
14751494

1476-
$errors = Address::from_nexus( $address )->validate()->get_error_messages();
1495+
$nexus = Address::from_nexus( $address );
1496+
1497+
/*
1498+
* The origin state is required for the US, and only for the US.
1499+
*
1500+
* US nexus is determined from the ORIGIN state; VAT and GST countries rate from
1501+
* the destination and ignore a blank origin state. Measured against the live
1502+
* TaxJar API rather than assumed -- GB, FR, NL, DK, DE, ES, IT, IE, CA and AU
1503+
* all return an identical rate whether the nexus carries its state or an empty
1504+
* string, while a US nexus with a blank state returns HTTP 200 with
1505+
* `has_nexus: false` and zero tax. It fails silently, which is exactly why the
1506+
* check has to stay for the US.
1507+
*
1508+
* Requiring it everywhere is what makes this wrong. The nexus address this
1509+
* plugin builds for its own store always carries a 'state' key, and
1510+
* WC()->countries->get_base_state() is '' for 19 of the 31 countries in
1511+
* get_supported_countries() -- so a blanket requirement rejected the store's own
1512+
* nexus on every tax calculation for those merchants, dropping the nexus block
1513+
* from the request and writing a forced error log plus a persistent admin error
1514+
* notice. For a filter-supplied nexus it is worse than noise: falling back to
1515+
* the store's from_* address changes which origin TaxJar rates against.
1516+
*/
1517+
$required = ( 'US' === $nexus->country() ) ? array( 'country', 'state' ) : array( 'country' );
1518+
1519+
$errors = $nexus->validate( $required )->get_error_messages();
14771520

14781521
if ( ! empty( $errors ) ) {
14791522
$this->logger->error( 'Nexus Address ERRORS: ' . implode( ', ', $errors ) . PHP_EOL . 'Nexus address removed from request body.' . PHP_EOL . print_r( $address, true ), 'WCS Tax' );

tests/php/test-class-wc-connect-taxjar-integration.php

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2567,4 +2567,144 @@ function ( $nexus_address ) {
25672567
$this->assertIsArray( $body );
25682568
$this->assertArrayNotHasKey( 'nexus_addresses', $body );
25692569
}
2570+
2571+
/**
2572+
* A non-scalar value under an unknown key must not reject the whole address.
2573+
*
2574+
* The non-scalar guard exists because the value object casts the fields it reads to
2575+
* string. Keys it does not read are never cast — they are merged back untouched —
2576+
* so they carry no conversion risk. Checking them anyway made a single custom key
2577+
* holding an array discard the entire nexus address, silently swapping which nexus
2578+
* TaxJar was asked about, and logged "[meta] field must be a string" for a field
2579+
* that is never treated as a string.
2580+
*/
2581+
public function test_non_scalar_unknown_nexus_key_is_passed_through() {
2582+
$body = $this->capture_taxjar_request_json(
2583+
$this->in_state_options(),
2584+
function ( $nexus_address ) {
2585+
$nexus_address['meta'] = array( 'source' => 'erp' );
2586+
2587+
return $nexus_address;
2588+
}
2589+
);
2590+
2591+
$this->assertIsArray( $body );
2592+
$this->assertArrayHasKey(
2593+
'nexus_addresses',
2594+
$body,
2595+
'A non-scalar value under an unknown key rejected the whole nexus address.'
2596+
);
2597+
$this->assertSame( array( 'source' => 'erp' ), $body['nexus_addresses'][0]['meta'] );
2598+
$this->assertSame( 'US', $body['nexus_addresses'][0]['country'] );
2599+
}
2600+
2601+
/**
2602+
* A store outside the US keeps its own nexus address even with a blank state.
2603+
*
2604+
* `to_nexus_array()` always emits a `state` key, and `WC()->countries->get_base_state()`
2605+
* returns `''` for 19 of the 31 countries in `get_supported_countries()` — GB, FR,
2606+
* NL, BE, DK, SE, PL, PT and the rest of the stateless EU. Requiring a non-blank
2607+
* state therefore rejected the nexus this plugin builds for the merchant's own
2608+
* store, on every tax calculation, and each rejection wrote a forced error log and
2609+
* a persistent admin error notice.
2610+
*
2611+
* The provider deliberately mixes countries WooCommerce gives no states (GB, FR, NL,
2612+
* DK) with ones it does (DE, ES, IT, CA, AU). Both groups belong here: measured
2613+
* against the live TaxJar API, every one of them rates a blank-state nexus
2614+
* identically to a populated one, because only the US derives nexus from the origin
2615+
* state. Keying the requirement off "does WooCommerce model states for this country"
2616+
* would pass the first group and wrongly reject the second.
2617+
*
2618+
* @dataProvider provider_non_us_store_countries
2619+
*
2620+
* @param string $country Two-letter country code.
2621+
* @param string $postcode A postcode valid for that country.
2622+
* @param string $to_state Destination state. Only CA needs one — `validate_taxjar_request()`
2623+
* requires a destination state for US and CA, which is a rule about
2624+
* where the customer is, not about the store's nexus.
2625+
*/
2626+
public function test_non_us_store_keeps_its_nexus_address_without_a_state( $country, $postcode, $to_state = '' ) {
2627+
$store = array(
2628+
'street' => '1 Store Way',
2629+
'city' => 'Somewhere',
2630+
'state' => '',
2631+
'country' => $country,
2632+
'postcode' => $postcode,
2633+
);
2634+
2635+
$options = $this->in_state_options();
2636+
$options['to_country'] = $country;
2637+
$options['to_state'] = $to_state;
2638+
$options['to_zip'] = $postcode;
2639+
$options['to_city'] = 'Somewhere';
2640+
2641+
$body = $this->capture_taxjar_request_json( $options, null, $store );
2642+
2643+
$this->assertIsArray( $body, sprintf( 'A %s store aborted the tax calculation.', $country ) );
2644+
$this->assertArrayHasKey(
2645+
'nexus_addresses',
2646+
$body,
2647+
sprintf( 'The store nexus was rejected for %s, where TaxJar does not need an origin state.', $country )
2648+
);
2649+
$this->assertSame( $country, $body['nexus_addresses'][0]['country'] );
2650+
$this->assertSame( '', $body['nexus_addresses'][0]['state'] );
2651+
}
2652+
2653+
/**
2654+
* Non-US TaxJar-supported countries, with and without WooCommerce states.
2655+
*
2656+
* The `has states` flag is recorded only to document that the split is deliberate —
2657+
* nothing in the assertion depends on it, which is the point.
2658+
*
2659+
* @return array<string, array{0: string, 1: string, 2: string}>
2660+
*/
2661+
public function provider_non_us_store_countries() {
2662+
return array(
2663+
// No states in WooCommerce.
2664+
'GB' => array( 'GB', 'SW1A 1AA', '' ),
2665+
'FR' => array( 'FR', '75001', '' ),
2666+
'NL' => array( 'NL', '1011 AB', '' ),
2667+
'DK' => array( 'DK', '1050', '' ),
2668+
2669+
// States in WooCommerce, but TaxJar rates them from the destination anyway.
2670+
'DE' => array( 'DE', '10117', '' ),
2671+
'ES' => array( 'ES', '28001', '' ),
2672+
'IT' => array( 'IT', '00184', '' ),
2673+
'AU' => array( 'AU', '2000', '' ),
2674+
2675+
// CA needs a destination state to clear `validate_taxjar_request()`; the
2676+
// store's own state stays blank, which is what this test is about.
2677+
'CA' => array( 'CA', 'M5H 2N2', 'ON' ),
2678+
);
2679+
}
2680+
2681+
/**
2682+
* The US keeps the strict requirement, because there a blank state loses the tax.
2683+
*
2684+
* This is the one country the relaxation must not reach. US nexus is derived from
2685+
* the origin state, and TaxJar does not reject a blank one — it returns HTTP 200
2686+
* with `has_nexus: false` and zero tax, so an accepted blank-state US nexus is a
2687+
* silent under-collection. Rejecting it here falls back to the store's `from_*`
2688+
* address, which carries the real state.
2689+
*
2690+
* This passes before the relaxation too — it bounds the change rather than
2691+
* demonstrating a fixed bug.
2692+
*/
2693+
public function test_blank_state_is_still_rejected_for_a_us_nexus() {
2694+
$body = $this->capture_taxjar_request_json(
2695+
$this->in_state_options(),
2696+
function ( $nexus_address ) {
2697+
$nexus_address['state'] = '';
2698+
2699+
return $nexus_address;
2700+
}
2701+
);
2702+
2703+
$this->assertIsArray( $body, 'A blank nexus state aborted the request instead of falling back.' );
2704+
$this->assertArrayNotHasKey(
2705+
'nexus_addresses',
2706+
$body,
2707+
'A US nexus address with a blank state was forwarded.'
2708+
);
2709+
}
25702710
}

0 commit comments

Comments
 (0)