[WOOTAX-322] Refactor - Hoist the canonical-id exemption remap - #2990
Conversation
…he helper assign_canonical_line_item_ids() rewrites each TaxJar line item id, which is exactly what invalidates $non_taxable_line_items: callers record that map while ids are still context-specific, but get_itemized_tax_rates() reads it back under the canonical id. Landing WOOTAX-258 and WOOTAX-240 together left the map keyed one way and the lookup the other, so the non-taxable guard stopped firing and WOOTAX-240 silently regressed. The shipped fix repairs the map in the caller, duplicated verbatim in get_line_items() and get_backend_line_items(). That leaves a coupling invariant living one level below where it belongs: a third caller that omits the block reintroduces the same regression, silently and with nothing failing. Move the remap into assign_canonical_line_item_ids(), which already holds both $key and $product_id in scope. The map is no longer observable in the stale keying, and a future caller inherits the invariant instead of remembering it. This also drops the substr( $key, strpos( $key, '-' ) + 1 ) parsing, correct today only because product IDs, order item IDs and cart item keys happen to contain no '-'. Behaviour-preserving. Both callers already reset the map before building, so replacing it wholesale inside the helper is safe, and an entry whose line item never made it into $line_items (the order path skips zero-priced lines) is still dropped. Two tests call the helper directly, with no caller involved, to pin the remap at the seam a third caller would rely on. The five behavioural guards from WOOTAX-240 and WOOTAX-258 are unchanged.
CezaryDrewniak
left a comment
CezaryDrewniak
left a comment
There was a problem hiding this comment.
Reviewed the full change and traced every consumer of the re-keyed map (get_itemized_tax_rates, both callers, the cache seam) — the refactor is behavior-preserving as described, the key reconstruction is exactly equivalent, and the drop semantics match the old caller-side remap. Tests pass (401/978), and no PHPCS violation lands on a changed line.
Agree with Abdalsalaam's comment, though: the seam test only pins re-keying, not accumulation. If $non_taxable[ $canonical_id ] = true on line 1216 were ever rewritten as a reset, the suite would stay green (no test produces two exempt line items), and a cart with two exempt products in one tax class would regress straight back to WOOTAX-240. I'd fold a second exempt line item into test_assign_canonical_line_item_ids_rekeys_non_taxable_map (or add a third test) and assert both canonical keys survive — that's exactly the invariant the "test at the seam" thesis is supposed to pin.
Small note for a follow-up (not blocking): while checking for similar single-element blind spots I found three pre-existing ones outside this PR — override_woocommerce_tax_rates has no test at all, the $product_id- prefix matcher has no collision test (product 1 vs 10), and pre_recalculation_tax_snapshots is only ever tested with one order. Worth separate tickets, not this PR.
…d seam
The seam test asserted that the non-taxable map comes back keyed by canonical id,
but not that every exempt line survives. Because the record sits inside an
isset() guard, only matching iterations touch the accumulator, so with a single
exempt line item "add to the map" and "replace the map" behave identically.
Rewriting classes/class-wc-connect-taxjar-integration.php:1216 from
$non_taxable[ $canonical_id ] = true;
to
$non_taxable = array( $canonical_id => true );
passed the entire suite (401/401), because no fixture anywhere produced two
exempt line items. In production a cart with two exempt products would leave
only the last one guarded, and the first one's 0% TaxJar breakdown line would
overwrite the shared tax-class rate row - the WOOTAX-240 regression this seam
exists to prevent.
Fold a second exempt line item into the test and assert both canonical keys
survive, so the accumulation is pinned rather than incidentally satisfied. No
production change: the helper was already correct.
Reported by @Abdalsalaam and @CezaryDrewniak in review of #2990.
|
Thanks — the accumulation gap is fixed in 1f01aa1, and I reproduced your mutation before accepting it rather than taking it on trust.
Your three follow-ups — filed, and all three verified firstAgreed they're out of scope here. I checked each against the code before filing, since a ticket asserting an unverified gap is its own kind of debt. All three are real; one needed its framing corrected.
Three for three surviving the full suite is the more interesting result than any individual one — the single-element fixture is a pattern in this file, not an isolated miss. Happy to pick these up next, or leave them for whoever grabs the queue. |
CezaryDrewniak
left a comment
CezaryDrewniak
left a comment
There was a problem hiding this comment.
Verified the follow-up end to end — good to merge.
The seam test now does what the PR's thesis claims: I applied the reset mutation ($non_taxable = array( $canonical_id => true )) to :1216 and it fails with the first exempt key missing from the map — exactly the WOOTAX-240 failure shape (first exempt line loses its guard, its 0% breakdown would clobber the shared rate row). Reverted, suite is green at 401/978, both seam tests pass, and PHPCS shows zero new violations (test file report identical to baseline). Production code confirmed byte-identical across the two commits, and I traced every consumer of the map and the canonical ids — nothing else is affected.
3 participants
Description
Follow-up to #2983, filed from @Abdalsalaam's review there.
assign_canonical_line_item_ids()rewrites each TaxJar line item'sid, and that rewrite isexactly what invalidates
$non_taxable_line_items: callers record that map while ids are stillcontext-specific, but
get_itemized_tax_rates()reads it back under the canonical id. LandingWOOTAX-258 and WOOTAX-240 together left the map keyed one way and the lookup the other, so the
non-taxable guard stopped firing and WOOTAX-240 silently regressed.
The fix shipped in #2983 repairs the map in the caller — an identical eight-line block duplicated
verbatim in
get_line_items()andget_backend_line_items(). That leaves a coupling invariantliving one level below where it belongs: a third caller of
assign_canonical_line_item_ids()thatomits the block reintroduces the same regression, silently, with nothing failing. That is the same
failure signature as the original bug.
This PR moves the remap into
assign_canonical_line_item_ids(), which already holds both$keyand$product_idin scope:The map is no longer observable in the stale keying, and a future caller inherits the invariant
instead of having to remember it. Each call site keeps a one-line comment noting the side effect.
Why this is behaviour-preserving
array()before building (:983cart,:1098order), soreplacing it wholesale inside the helper cannot clobber the other path's state.
$line_items(
isset( $line_items[ $item_key ] )). The new form iterates$line_items, so such an entrysimply never appears — same result. This case is real, not theoretical:
get_backend_line_items()records the exempt status before the
if ( $unit_price )guard decides whether the line isemitted at all, so a zero-priced exempt item is recorded and then omitted.
$product_id . '-' . $keyreconstructs the caller's key exactly, so the lookup is equivalent tothe old parse for every input the two callers can produce.
One strict difference, unreachable today
The removed
substr( $legacy_key, strpos( $legacy_key, '-' ) + 1 )mis-parses any key containing a-. The new form does not parse at all, so it is robust where the old one was not. No currentcaller can produce such a key — cart item keys are md5 hashes, order item IDs and product IDs are
integers — so this is unobservable in practice.
I deliberately did not add a test pinning it: asserting it would assert a behaviour change in a
refactor that is meant not to have one. Flagging it here instead, since it is the undocumented
assumption the ticket set out to remove.
Backward compatibility
No BC impact.
assign_canonical_line_item_ids()isprivateand$non_taxable_line_itemsis aprivateproperty, both introduced in the current unreleased 3.6.12 — neither has ever beenreachable from outside this class. No hook is added, removed, reordered or retimed; no new read of a
global or of
WC()state is introduced; no site-scoped option or path/URL construction is touched,so multisite and non-root install layouts are unaffected.
On the changelog checkboxes
Left unchecked deliberately, rather than overlooked.
assign_canonical_line_item_ids()is@since 3.6.12and$non_taxable_line_itemscame from the WOOTAX-240 fix, also in the 3.6.12block — which is still unreleased (
2026-xx-xxplaceholder;Stable tag: 3.6.11). Both the codebeing changed and the state it replaces are unreleased, so there is no user-visible delta to report
and an entry would document internal churn in a version that never shipped. Happy to add one if you
would rather the block record it.
Related issue(s)
Closes WOOTAX-322
Follow-up to #2983 (WOOTAX-258). Guards the fix for WOOTAX-240.
Steps to reproduce & screenshots/GIFs
No user-visible change, so there is nothing to screenshot — this is a behaviour-preserving refactor
and the test suite is the evidence. To verify:
composer test— expect 401 tests / 978 assertions green. Baseline ontrunkis 399/975;the delta is exactly the two new tests added here.
untouched in the diff, which is the point:
Standard-class product with a non-taxable (Tax status "None") one and confirm the Standard rate
row is not zeroed.
PHPCS: zero new violations.
--report=sourceover the two changed files is identical totrunkat 30 sniff types / 179 violations.
Checklist
changelog.txtentry addedreadme.txtentry addedOn the two new tests
Both call the
privatehelper directly via reflection, with no caller involved. That isdeliberate: a test entering through
get_line_items()/get_backend_line_items()would stay greenif someone re-duplicated the block back into both callers, so it would pin the two known paths
rather than the contract. Testing at the seam is what makes a hypothetical third caller safe.
test_assign_canonical_line_item_ids_rekeys_non_taxable_map— seeds the map in the legacy<product_id>-<key>shape and asserts it comes back keyed by canonical id, with the legacy key gone.test_assign_canonical_line_item_ids_drops_non_taxable_entries_without_a_line_item— pins the dropsemantics that make replacing the map wholesale safe.