Tweak - Match TaxJar cache entries across cart and order paths (WOOTAX-258)

[bartech]

Description

The TaxJar transient cache key was an md5 of the raw request body, so two requests that
would receive the same answer from TaxJar routinely missed each other. Two independent
causes, fixed together.

1. The line item id was context-specific. get_line_items() built it from
WooCommerce's cart item key, get_backend_line_items() from the numeric order item ID, so
the same basket produced two different bodies and the order path missed every time. Both
builders now derive id from the tax-relevant inputs alone — product, tax code, quantity,
unit price, discount, tax location — as <product_id>-<fingerprint>-<occurrence>. TaxJar
treats id as an opaque echo field, so nothing about the request semantics changes. The
occurrence counter keeps two genuinely identical order lines distinct so neither loses its
rate. The product ID stays the first - segment, which get_itemized_tax_rates(),
override_cart_item_tax_rates() and override_order_item_taxes() all depend on.

2. The key was byte-sensitive. A differently-cased city, a stray double space, or "5"
versus "5.00" each split the cache. The key now comes from a canonical projection of the
body — whitespace collapsed, case folded, amounts given one representation, object keys
sorted, lists sorted. The body sent to TaxJar is unchanged; only key derivation moved.
Numeric normalization is applied by field name rather than by sniffing values, specifically
so a leading-zero ZIP is never reinterpreted (01234 must not collapse onto 1234).

Reviewer notes

• get_line_items() and get_backend_line_items() now return arrays keyed by cart item
  key / order item ID instead of lists, because calculate_totals() and
  calculate_backend_totals() need to get from their own key to the canonical one.
  calculate_tax() gained an array_values() so the body still encodes as a JSON array
  rather than an object.

• get_line_item() needed no change — it already searched by ['id'].

• Known conflict: tests/php/test-class-wc-connect-taxjar-integration.php conflicts at
  the trailing append point with anything else that appends tests to the same class. No
  logic overlaps — resolution is "keep all blocks".

  • Fix - Validate TaxJar response before applying tax rate override (WOOTAX-74) #2978 is resolved: merged to trunk on 2026-07-27 and merged into this branch, with
    both blocks kept. Suite green at 247 afterwards.
  • Fix - Non-taxable product no longer zeroes shared standard tax rate (WOOTAX-240) #2979 is still open and will conflict the same way. Same resolution applies. The class
    file, changelog.txt and readme.txt all auto-merge with it.

• Out of scope, raised separately: the two builders disagree on product_tax_code in
  two independent, pre-existing ways. Both would need a payload change to fix, so neither
  belongs in a caching PR.

  1. Zero-rate tax classes (WOOTAX-309): the cart routes the decision through
     is_taxable(), the order path reads get_tax_status().

  2. Multi-word tax class names (WOOTAX-320): the cart takes the last - segment
     (is_numeric( end( $parts ) )), the order path takes the second
     (is_numeric( $parts[1] )). The order rule only holds when the class name is exactly
     one word, so it breaks on any slug with an internal hyphen.

     An earlier revision of this description called this "three or more segments", which
     understated it — the trigger is any tax class whose display name is more than one
     word, including WooCommerce's built-in Reduced rate and Zero rate:

     tax_class slug	cart	order
     standard-12345	12345	12345	✓
     clothing-40030	40030	40030	✓
     reduced-rate-12345	12345	""	✗
     zero-rate-99999	99999	""	✗
     digital-goods-31000	31000	""	✗

     Filed as WOOTAX-320, and it is worse than a cache miss: the order path sends no
     product tax code at all, so TaxJar prices that request at the general rate. Where the
     code carries a category exemption or reduced rate, an admin order recalculation can
     return a different figure than the customer was charged. Pre-existing since 2017
     (706c6b10 / 78114e8a), untouched by this PR.

  Consequence for this PR: cross-path cache hits are not achieved for zero-rate items,
  nor for stores whose tax class display name is more than one word. Every other store gets
  the full benefit.

Backward compatibility

Two protected methods on WC_Connect_TaxJar_Integration change shape. The class is in the
global namespace, so per our BC policy these count as externally exposed even though nothing
in this repo subclasses it.

Surface	Before	After
get_line_items() / get_backend_line_items() return value	list (0..n)	map keyed by cart item key / order item ID
line item id format	<product_id>-<cart_key_or_item_id>	<product_id>-<fingerprint>-<occurrence>

Who could notice: a subclass that overrides either method and indexes the parent result
positionally (parent::get_line_items()[0]), or any code that parses id past the product
prefix to recover a cart key or order item ID.

Why it is judged low risk: the <product_id>- prefix is unchanged, which is the only
part of id that this plugin's own consumers (get_itemized_tax_rates(),
override_cart_item_tax_rates(), override_order_item_taxes()) rely on. array_values() in
calculate_tax() keeps the outgoing request body a JSON array, so the wire format TaxJar
sees is unchanged. No public method signature, hook, or hook argument is added, removed, or
reordered.

Not deprecation-eligible: these are return-shape changes inside existing methods, not
renames, so there is no old symbol to keep alive alongside a new one. Flagging rather than
deprecating is the available path here.

Related issue(s)

WOOTAX-258

Steps to reproduce & screenshots/GIFs

Watch smartcalcs_request() — that is the only method that makes a real API call.
maybe_calculate_totals() call counts are not a useful metric, since it early-returns in
most contexts.

Cache now hits (fails on trunk):

1. Enable automated taxes with a US store address.
2. Add a product to the cart and complete checkout. Note one smartcalcs_request() call.
3. Within the hour (cache_time = HOUR_IN_SECONDS), open the order in wp-admin and save
   the order items. Before: a second API call. After: no second call.
4. Re-enter the same destination address with different casing or an extra internal space.
   Before: a fresh API call. After: served from cache.

Must still miss the cache (regression check):

5. Change the destination ZIP, the state, the quantity, or apply a coupon that changes the
   discount. Each must produce a fresh smartcalcs_request() call.
6. Confirm calculated tax totals and per-line rate assignment are identical to trunk in
   every case above, including a mixed cart with a non-taxable item and a two-location
   cart (WOOTAX-250 grouping).
7. An order holding the same product twice at the same price — both lines must receive
   their own rate.

Automated: composer test → 247 tests, 532 assertions, green (measured after merging
trunk, so the total includes #2978). This PR adds 12 test methods to
tests/php/test-class-wc-connect-taxjar-integration.php (62 → 74 versus trunk), which data
providers expand to 21 PHPUnit cases. The three "normalization ignores X" tests each first
assert the two raw bodies are not byte-identical, so they document the pre-fix baseline
and cannot pass vacuously. PHPCS reports 0 new violations on the changed files, verified by
diffing --report=json against the trunk version rather than reading the report.

Two of those close gaps raised in review — both cover paths where a regression would be
silent rather than loud:

• test_calculate_tax_encodes_keyed_line_items_as_json_list captures the encoded body and
  asserts line_items is a zero-indexed list. calculate_backend_totals() passes the
  order-item-ID-keyed map straight through without group_items_by_location() re-indexing
  it, so array_values() is the only thing preventing a JSON object on every admin-side
  request — and the pre-existing calculate_tax() test returns at the cross-state guard
  before reaching it. The assertion checks the raw JSON as well as the decoded array,
  because json_decode() hides the array/object distinction.
• test_smartcalcs_cache_request_serves_equivalent_bodies_from_one_entry exercises the
  PR's central claim end to end: two equivalent-but-byte-different bodies, one
  smartcalcs_request() invocation. It asserts specifically on the
  tj_tax_<md5(signature)> transient and on the absence of the tj_tax_<zip>_<state>
  key — that second key only ever caches 400 zip-to-state mismatches, and asserting on
  call count alone would let it mask a broken signature.

Both were verified to fail against deliberately mutated guards (array_values() removed;
get_cache_signature() reverted to returning the raw body) rather than assumed to be
meaningful because they pass.

Checklist

• unit tests

• changelog.txt entry added
• readme.txt entry added

[CezaryDrewniak]

WOOTAX-258 Review

Test matrix

#	Claim	How verified	Result
1	Full suite green	composer test	236 tests, 495 assertions, OK
2	TaxJar class suite	phpunit --filter WP_Test_WC_Connect_TaxJar_Integration	86 tests, 191 assertions, OK
3	No new PHPCS sniff types	phpcs --report=json diff PR vs HEAD~1	30 unique sniffs each, empty diff
4	Body still serializes as JSON array	array_values() demo	[{"id":...},...], not {...}
5	product_id stays as first - segment	test_canonical_line_item_id_keeps_product_id_prefix	OK
6	id-based response lookup intact	test_zero_amount_response_persists_real_itemized_rates	OK
7	Leading-zero ZIP not collapsed	test_cache_signature_preserves_leading_zero_zip	OK
8	usort deterministic	standalone PHP reordering test	identical canonical JSON
9	override_* filters still match by product_id-	test_override_cart_item_tax_rates_returns_taxjar_rates_when_matched + test_override_order_item_taxes_applies_taxjar_rates_when_matched	OK, 2 tests / 8 assertions
10	get_line_items new contract	test_get_line_items_is_keyed_by_cart_item_key + test_canonical_line_item_id_matches_across_cart_and_order_paths	OK, 2 tests / 5 assertions
11	WCS_Subscriptions price applied before canonicalization	grep call order in get_line_items	lines 988 → 1023, confirmed
12	BC note absent from PR description	git log -1 --format=%B | grep -iE 'BC|backward|protected|subclass'	no match
13	@since 3.4.0 on new private methods	grep -nE '@since 3\.4\.0'	5 hits at lines 1117, 1159, 1189, 1226, 1249
14	Pre-existing is_taxable() vs get_tax_status() divergence	standalone PHP reproduction	confirmed; PR description flags it
15	Newly found pre-existing multi-part tax class divergence	standalone PHP reproduction (see below)	confirmed; not in PR description

Findings worth surfacing

1. No regressions introduced. Every code path traced (line items builders, get_line_item, get_itemized_tax_rates, override_cart_item_tax_rates, override_order_item_taxes, override_woocommerce_tax_rates, calculate_taxes_by_location, calculate_tax, smartcalcs_cache_request, smartcalcs_request) is either unchanged or strengthened. The body sent to TaxJar is byte-identical apart from the opaque id field.

2. Pre-existing tax_code divergence is broader than the PR description suggests. The PR calls out the is_taxable() (cart) vs get_tax_status() (order) mismatch for zero-rate classes. There's a second, separate pre-existing bug: the cart path uses is_numeric( end( $parts ) ) at classes/class-wc-connect-taxjar-integration.php:974 (last element) while the order path uses is_numeric( $parts[1] ) at classes/class-wc-connect-taxjar-integration.php:1063 (second element). For tax classes with 3+ segments, they disagree:

tax_class cart order match? "" "" "" yes "standard" "" "" yes "reduced-rate" "" "" yes "standard-12345" "12345" "12345" yes "standard-rate-12345" "12345" "" NO "a-b-c-99" "99" "" NO

This means the PR's "cache hits across cart and order" benefit is partial for stores that use multi-segment tax classes (a 3-segment class with a numeric suffix is the most common offender). The PR cannot fix it without changing the request payload, but it should be mentioned alongside the zero-rate case in the "out of scope" section so reviewers don't expect full cross-path hits on such stores.

3. Wrong @since on the five new private methods. They are stamped @since 3.4.0 at classes/class-wc-connect-taxjar-integration.php:1117, 1159, 1189, 1226, 1249. The in-progress version is 3.6.10 (per changelog.txt). The line 1002 hit is pre-existing and not from this PR. No functional impact (private methods aren't part of the public surface), but it's misleading in the docblock.

4. Backward-compatibility note missing from the PR description. Per the AGENTS.md guidance, the return shape change on the two protected methods get_line_items() and get_backend_line_items() (now keyed by cart item key / order item id, with the canonical TaxJar id under 'id') needs a BC note for any subclass that overrides them. No subclass exists in this repo, but external code could.

Recommendation

Ship-able. Two non-blocking asks:

• Add a one-line BC note to the PR description about the protected return-shape change.
• Mention the multi-part tax class divergence in the "out of scope" paragraph.
• Fix the 5 wrong @since stamps to 3.6.10.

[iyut]

One thing I'd flag here: this quietly changes the shape of what get_line_items() and get_backend_line_items() return — they're keyed by cart-item-key / order-item-ID now instead of plain lists, and the id goes from <product_id>-<key> to <product_id>-<fingerprint>-<occurrence>.

In practice I don't think it'll bite anyone — the <product_id>- prefix still holds, so the override/itemized-rate lookups are fine, and array_values() keeps the outgoing body a JSON array. But since this is a global WC_Connect_* class, our BC policy treats even these protected methods as fair game for outside code (a subclass doing parent::get_line_items()[0], or someone parsing the id past the product prefix, would notice). Could you add a quick BC note to the PR description covering the return-shape and id-format change? Just so it's on the record.

[iyut]

Nice work on this one. The cross-path id normalization is the right call — fixing it at the source instead of just patching the cache key means the cart→order case actually lines up now, and I like that the canonical signature handles the whitespace/case/number-format stuff properly rather than the strtoupper shortcut. Test coverage is genuinely solid too; the "not byte-identical" baseline assertions are a nice touch so those can't silently pass.

Only thing I'd ask before merge: could you drop a short BC note in the description? The return shape of get_line_items() / get_backend_line_items() changed (keyed arrays now) and the id format changed too — low real-world risk since the <product_id>- prefix still holds, but it's a global WC_Connect_* class so it's worth having on record. Left an inline comment on the spot as well.

Approving — thanks for the thorough writeup and for splitting the zero-rate product_tax_code thing out to WOOTAX-309 instead of scope-creeping it in here.

[Abdalsalaam]

calculate_backend_totals() passes the order-item-ID-keyed map from get_backend_line_items() straight into calculate_tax() (lines 633 and 647) without going through group_items_by_location(), which is what re-indexes on the cart path (line 815). array_values() at line 1845 is therefore the only thing keeping line_items encoded as a JSON array rather than a JSON object like {"12":{...}} for every admin-side TaxJar request. Grepping tests/php/test-class-wc-connect-taxjar-integration.php for smartcalcs_request, smartcalcs_cache_request, set_transient, get_transient and request-body assertions returns nothing, and the only calculate_tax() test returns at the cross-state guard (line 1834) before line 1845 executes. A regression here would silently malform every admin TaxJar request with no failing test.

Suggested change

	$body['line_items'] = array_values( $line_items );
	Add a PHPUnit test that partial-mocks smartcalcs_cache_request, captures the encoded $json, and asserts json_decode( $json, true )['line_items'] is a zero-indexed list when calculate_tax() is handed a string-keyed line_items array.

[Abdalsalaam]

smartcalcs_cache_request() has no test coverage at all. The 19 new tests compare get_cache_signature() outputs and canonical ids in isolation, verifying the ingredients but never the result. The PR's central claim -- that opening the order in wp-admin within cache_time produces no second smartcalcs_request() call -- rests entirely on manual verification and would not be caught by CI if it regressed.

Suggested change

	$cache_key = 'tj_tax_' . hash( 'md5', $this->get_cache_signature( $json ) );
	Add a test that mocks smartcalcs_request to count invocations, calls smartcalcs_cache_request() twice with two equivalent-but-byte-different bodies, and asserts exactly one HTTP call plus a populated 'tj_tax_' . md5( signature ) transient.

[bartech]

Thanks all — every ask here was actionable. Summary of what changed, and one that turned
out to be already fixed.

@Abdalsalaam — both test gaps closed

Implemented as specified, and both now sit on paths where a regression would have been
silent rather than loud.

test_calculate_tax_encodes_keyed_line_items_as_json_list — captures the encoded body
and asserts line_items is a zero-indexed list. Your reading was right: calculate_backend_totals()
passes the order-item-ID-keyed map straight through without group_items_by_location()
re-indexing it, so array_values() is the only thing standing between us and
{"12":{...}} on every admin-side request. Confirmed the pre-existing calculate_tax()
test uses a CA→NY address and returns at the cross-state guard, so it never reached that
line. The test asserts on the raw JSON as well as the decoded array, because
json_decode() collapses the array/object distinction that is the whole point.

test_smartcalcs_cache_request_serves_equivalent_bodies_from_one_entry — two
equivalent-but-byte-different bodies, one smartcalcs_request() invocation, plus the
populated transient.

One addition beyond your suggestion, because counting calls alone would not have been
enough: smartcalcs_cache_request() checks tj_tax_<zip>_<state> before the signature
key. I traced where that key is written — only on a 400 zip↔state mismatch — so a 200-path
test is not confounded by it. But if that ever changed, a call-count assertion would pass
while the signature was broken. The test therefore asserts the tj_tax_<md5(signature)>
transient is populated and that the zip/state key is absent.

Both were mutation-verified rather than assumed meaningful because they pass. Removing
array_values() fails the first with array_keys returning 12, 34; reverting
get_cache_signature() to returning the raw body fails the second with a call count of 2.

@CezaryDrewniak — @since was already fixed, and my correction was wrong

Good catch, and it was real — but the fix landed before I got to it, and my version would
have been incorrect. Worth spelling out since the numbers are confusing:

• I read changelog.txt, saw 3.6.10 as in-progress, and stamped @since 3.6.10.
• 3.6.10 had actually shipped on 2026-07-27, and 42671539 retargeted this PR's
  changelog entry to 3.7.0.
• edff7469 had already stamped all five methods @since 3.7.0.

3.7.0 is correct. My commit is superseded and dropped in the merge resolution; the five
docblocks read @since 3.7.0 on the current tip.

Multi-segment tax classes — verified and added to the out-of-scope section. The cart
takes the last - segment (is_numeric( end( $parts ) )), the order path takes the second
(is_numeric( $parts[1] )); they agree on standard-12345 but not on
standard-rate-12345, where cart yields 12345 and order yields "". You are right that
it narrows the benefit, and the description now says so explicitly rather than implying
full cross-path hits for every store.

@CezaryDrewniak @iyut — BC note added

Added a Backward compatibility section to the description covering both surfaces: the
return-shape change on get_line_items() / get_backend_line_items() (list → keyed map)
and the id format change (<product_id>-<cart_key_or_item_id> →
<product_id>-<fingerprint>-<occurrence>), who could notice, and why it is judged low
risk. Agreed it belongs on the record given these are protected methods on a
global-namespace WC_Connect_* class — the "nothing in this repo subclasses it" argument
does not cover third-party code.

Noted explicitly that the parameter-required path is not an option here: these are
return-shape changes inside existing methods, so there is no old symbol to deprecate
alongside a new one. Flagging is the available path.

Merge status

#2978 merged to trunk on 2026-07-27 and is now merged into this branch. The only conflict
was the predicted trailing append point in the test file; resolved "keep all blocks", no
logic overlap. #2979 is still open and will conflict identically — same resolution applies.

Suite on the current tip: 247 tests, 532 assertions, green. PHPCS: no new violation
types, verified by diffing --report=source before/after rather than reading the report.

Spun out

Filed WOOTAX-319 for something I hit while writing the cache test: the constructor declares
?StoreNoticesNotifier $notifier = null, but _error() and smartcalcs_cache_request()
dereference it unconditionally, so constructing with four arguments — which the signature
says is fine — fatals. Pre-existing, not touched here, and our own bootstrap always passes
a notifier so it is not a live crash.

[Abdalsalaam]

get_line_items() records exemptions at line 1024 as $id . '-' . $cart_item_key, but get_itemized_tax_rates() reads that map at line 2023 using $line_item_key from $taxes['line_items'], which is now keyed by the canonical <product_id>-<fingerprint>-<occurrence> id TaxJar echoes back. The two key shapes can never match, so the continue never fires and the 0% breakdown line of a non-taxable product is written over the shared Standard-class rate row via create_or_update_tax_rate() at line 2044. This silently reverts the unreleased 3.6.12 fix recorded two lines above this PR's own changelog entry. The property docblock at line 94 already states the required invariant ('Keyed by TaxJar line item id'), which the code now violates. Existing tests at tests/php/test-class-wc-connect-taxjar-integration.php:3739 and :3824 assert this and will fail.

Suggested change

	return $this->assign_canonical_line_item_ids( $line_items );
	$line_items = $this->assign_canonical_line_item_ids( $line_items );
	// The exempt record was keyed while ids were still context-specific; move it onto
	// the canonical ids, which is what get_itemized_tax_rates() looks up.
	$non_taxable = array();
	foreach ( array_keys( $this->non_taxable_line_items ) as $legacy_key ) {
	$item_key = substr( $legacy_key, (int) strpos( $legacy_key, '-' ) + 1 );
	if ( isset( $line_items[ $item_key ] ) ) {
	$non_taxable[ $line_items[ $item_key ]['id'] ] = true;
	}
	}
	$this->non_taxable_line_items = $non_taxable;
	return $line_items;

[Abdalsalaam]

get_backend_line_items() records exemptions at line 1118 as $id . '-' . $item_key (order item ID), which is likewise unreachable from the canonical-id lookup at line 2023. Order-side recalculation of a mixed order therefore zeroes the shared Standard rate row the same way. Existing test tests/php/test-class-wc-connect-taxjar-integration.php:3999 (assertArrayNotHasKey on $shipping_only_key) asserts this and will fail.

Suggested change

	return $this->assign_canonical_line_item_ids( $line_items );
	$line_items = $this->assign_canonical_line_item_ids( $line_items );
	// The exempt record was keyed while ids were still context-specific; move it onto
	// the canonical ids, which is what get_itemized_tax_rates() looks up.
	$non_taxable = array();
	foreach ( array_keys( $this->non_taxable_line_items ) as $legacy_key ) {
	$item_key = substr( $legacy_key, (int) strpos( $legacy_key, '-' ) + 1 );
	if ( isset( $line_items[ $item_key ] ) ) {
	$non_taxable[ $line_items[ $item_key ]['id'] ] = true;
	}
	}
	$this->non_taxable_line_items = $non_taxable;
	return $line_items;

[bartech]

Filed WOOTAX-322 for the duplication this introduces — picking it up right away.

Your suggestion is applied verbatim at both call sites in 6db27cf0, and the full suite is green (399 tests / 975 assertions). I kept the block exactly as you wrote it rather than substituting my own shape, since it's your review's fix.

The follow-up is that the eight-line remap is now duplicated between get_line_items() and get_backend_line_items(). That's duplication of a coupling invariant, so a third caller of assign_canonical_line_item_ids() that omits it silently reintroduces the exact regression you caught here — with no failing test unless someone writes one for that path specifically. The ticket proposes hoisting the remap into assign_canonical_line_item_ids() itself, which already has both $key and $product_id in scope and so also drops the substr( …, strpos( …, '-' ) + 1 ) parsing. That parsing only works today because product IDs, order item IDs and cart item keys happen to contain no -, which is an undocumented assumption worth not depending on.

Also in this push, and not part of your review: three tests from #2979 indexed get_line_items() positionally ($line_items[0]). This PR keys that array by cart item key and pins it in test_get_line_items_is_keyed_by_cart_item_key, so those tests now take the single element instead. Those three were the Undefined array key 0 errors — separate from the two assertion failures your review predicted, and that part is mine, not reviewed.