Skip to content

Fix - Non-taxable product no longer zeroes shared standard tax rate (WOOTAX-240) - #2979

Merged
bartech merged 6 commits into
trunkfrom
wootax-240-mixed-carts-of-taxable-and-non-taxable-products-dont
Aug 7, 2026
Merged

Fix - Non-taxable product no longer zeroes shared standard tax rate (WOOTAX-240)#2979
bartech merged 6 commits into
trunkfrom
wootax-240-mixed-carts-of-taxable-and-non-taxable-products-dont

Conversation

@bartech

@bartech bartech commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes WOOTAX-240.

When a cart mixes a taxable product and a non-taxable one (Tax Status = None, Tax Class = Standard), automated taxes stop calculating for the whole cart — the "Standard Rates" tax rate row is overwritten to 0%, and it stays zeroed (adding another taxable product doesn't restore it; removing the non-taxable product does).

Root cause

get_itemized_tax_rates() writes one WooCommerce tax rate row per TaxJar breakdown line item, and selects which row by the product's Tax Class — not its Tax Status:

$tax_class = $product->get_tax_class();   // 'Standard' ('') even when Tax Status = None
...
$this->create_or_update_tax_rate( $location, round( $tax_rate * 100, 4 ), $tax_class, ... );

The non-taxable product is sent to TaxJar as exempt (code 99999), so its breakdown line returns 0% — but it still maps back to the Standard class. create_or_update_tax_rate() therefore updates the same rate row the taxable product just populated (same class + location) down to 0%. The zeroed row persists in wp_woocommerce_tax_rates, which is why the problem sticks.

Note: the ticket's proposed change to get_line_items() (! $product->is_taxable()'none' === $tax_status) is a no-opWC_Product::is_taxable() already returns false for tax_status = 'none', so the exempt product is already sent as 99999. The defect is downstream, in how the itemized rate rows are written.

Fix

Skip rate-row writes for line items whose product has Tax Status None. WooCommerce already applies no tax to such products (override_cart_item_tax_rates() falls back to core, which taxes nothing for a non-taxable item), so the shared class row is left intact for genuinely taxable items. Zero-rate class products are unaffected — they own a separate zero-rate row.

Tests

Adds test_get_itemized_tax_rates_non_taxable_line_does_not_zero_standard_rate, which drives get_itemized_tax_rates() with a mixed taxable/exempt breakdown and asserts (a) the exempt line writes no rate row and (b) the taxable line's Standard row stays at 7.25%. It fails on trunk (exempt line writes/zeroes the shared row) and passes with this fix.

Full WP_Test_WC_Connect_TaxJar_Integration suite: 68 tests, 166 assertions green. PHPCS clean on the changed files.

Update — guard widened to "Shipping only" (review follow-up)

The guard now skips rate-row writes for any non-taxable Tax Status ('taxable' !== $status), not just 'None'. get_backend_line_items() sends every non-taxable status to TaxJar as exempt (99999), so a "Shipping only" product returned the same 0% breakdown line and reproduced the zeroing bug on the admin order screen. ProductTaxStatus defines exactly three statuses and set_tax_status() normalises empty → taxable, so the new condition is precisely "none or shipping".

Backward compatibility / scope: this is a behaviour change on both paths, not just the backend one where the bug reproduces. A "Shipping only" product on the cart previously had a rate row written with a real (non-zero) rate; it is now skipped. This is safe — WC_Cart_Totals gates item tax on is_taxable() and WC_Order_Item::calculate_taxes() on ProductTaxStatus::TAXABLE, so core applies no item tax to either status, and the shipping tax rate row is written separately after this loop, so shipping tax is unaffected. No hook, signature, or option contract changes; no new global or WC() state reads; nothing site- or path-scoped, so multisite and non-root installs are unaffected.

Additional coverage: get_backend_line_items() sends a "Shipping only" product as exempt (input side), and its 0% line leaves the Standard row at 7.25% (output side). The latter was verified to fail against the previous 'none' === guard.

A pre-existing parent/variation Tax Class divergence found during this review is tracked separately in WOOTAX-310.

Update — taxability recorded at emit time, not re-derived (review follow-up)

Review surfaced a third route to the same zeroing: woocommerce_product_is_taxable can filter a product to non-taxable while its Tax Status still reads taxable. get_line_items() sets the exempt code from the filtered is_taxable(), while the guard read the raw status — so the 0% line passed through and zeroed the shared Standard row. Confirmed against live TaxJar by the reviewer.

Rather than widen the guard a third time, the request side now records which line items WooCommerce will not tax ($non_taxable_line_items, keyed by TaxJar line item id) and the response side skips exactly those. The two halves share one decision instead of each re-deriving one from a different predicate.

The two paths deliberately record different predicates, because core gates them differently:

Path Core's item-tax gate Location
Cart $item->product->is_taxable() — filtered class-wc-cart-totals.php 677 / 735 / 796
Order ProductTaxStatus::TAXABLE === $this->get_tax_status() — raw class-wc-order-item.php 251

WC_Order_Item::calculate_taxes() never consults is_taxable(), so the order path has no filter divergence to close and recording the raw status is the accurate mirror there.

This is not implemented as "skip anything sent as 99999" — zero-rate class products are emitted as exempt for an unrelated reason and must keep updating their own zero-rate row, so only the non-taxability reason is recorded.

Two earlier review points fall out as a side effect: the deleted-product case is now moot (the lookup never dereferences a possibly-gone product), as is any variation/parent-id mismatch at this seam, since the key is the emitted composite rather than a re-resolved product id.

Backward compatibility: the new property is private; no signature, hook, or option contract changes; no new global or WC() state reads; nothing site- or path-scoped, so multisite and non-root installs are unaffected. The only behaviour change is which rate rows are written, and only for line items core applies no item tax to.

Tests: the two output-side tests previously fabricated breakdown keys and called get_itemized_tax_rates() with no matching request — a shape that structurally cannot catch an emit/response divergence, which is why this survived two review rounds with a green suite. Both now drive the request side first and read the real emitted key back. The "Shipping only" case also moved to the backend order path, which is where that status is actually emitted as exempt; it had been asserting a cart scenario that cannot occur. New: test_get_itemized_tax_rates_filtered_non_taxable_line_does_not_zero_standard_rate, reproducing the filter case and asserting both the input side (99999 emitted despite Tax Status "taxable") and the output side (shared row stays 7.25%) — verified red against the previous guard.

One gotcha for anyone re-running these: is_taxable() also requires wc_tax_enabled(), which is off in the bare WC test bootstrap, so the cart tests set woocommerce_calc_taxes.

WP_Test_WC_Connect_TaxJar_Integration: 80 tests, 206 assertions green. Full suite: 235 tests, 521 assertions green. PHPCS --report=source byte-identical to baseline on both changed files.

Changelog retargeted: 3.6.11 shipped from trunk while this PR was open, so the entry moved to a new unreleased = 3.6.12 - 2026-xx-xx = section in both changelog.txt and readme.txt rather than being merged into the released 3.6.11 block.

@bartech
bartech force-pushed the wootax-240-mixed-carts-of-taxable-and-non-taxable-products-dont branch from 4f399e0 to be37e0f Compare July 15, 2026 12:45
@bartech bartech self-assigned this Jul 15, 2026
@bartech
bartech marked this pull request as ready for review July 15, 2026 12:52

@CezaryDrewniak CezaryDrewniak left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

Test file PHPCS: 0 errors, 5 warnings, but all 5 are on pre-existing lines (197, 339, 885), none on the new test's lines (1869–1951).

PR claim Verified
New test passes ✅ 1 test, 3 assertions, green
Full suite green ✅ 68 tests, 166 assertions (matches PR description exactly)
PHPCS clean on changed files ✅ Production class: 0 violations on lines 1735–1750. Test file: 0 violations on lines 1869–1951. The 5 pre-existing warnings are at lines 197, 339, 885 (unrelated, in older tests)
Root cause analysis correct ✅ Cursor's get_line_items() change is a no-op (is_taxable() already false for tax_status='none'); real defect is downstream in get_itemized_tax_rates() row-write path
Fix placement correct continue placed after $tax_class is determined, before create_or_update_tax_rate(), leaving the shared Standard row intact for genuinely taxable items
No regression for zero-rate class products ✅ They own a separate rate row keyed by tax_class='zero-rate'; the skip only triggers on tax_status='none'
Changelog/readme updated ✅ Both changelog.txt:7 and readme.txt:77 add the new entry in the right format under = 3.6.8 - 2026-xx-xx =

The fix is minimal, correctly targeted at the actual defect (not the ticket's misdiagnosed surface), well-tested, and clean

Comment thread classes/class-wc-connect-taxjar-integration.php Outdated
@bartech

bartech commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the careful read @Abdalsalaam — I verified all three cases against WooCommerce core. Case 1 is real and I've implemented your suggestion (8232331). Cases 2 and 3 don't hold up; details below.

✅ Implemented — case 1, "Shipping only" status

Confirmed, and it's the whole reason to take the change. get_backend_line_items() sends any non-taxable status as 99999, so a Tax Status = "Shipping only" product returns a 0% breakdown line that the 'none' === guard let straight through — the identical zeroing failure, on the admin order screen. The path is ungated: calculate_backend_totals()calculate_tax()get_itemized_tax_rates(). You're also right that the cart path is immune, since line 972 excludes 'shipping' from the exempt branch.

Your safety argument holds on both paths — core never taxes these items, so the skipped row goes unused:

Path Gate Location
Cart $item->product->is_taxable() (≡ 'taxable' === get_tax_status()) WC_Cart_Totals 677 / 735 / 796
Order ProductTaxStatus::TAXABLE === $this->get_tax_status() WC_Order_Item::calculate_taxes() 251

And 'taxable' !== is exactly "none or shipping": ProductTaxStatus defines precisely three constants, and set_tax_status() normalises empty → taxable and rejects anything else. No fourth-value risk.

Two deviations from the suggestion as written:

  1. Trimmed the comment — dropped the ticket reference, since Linear IDs don't belong in runtime source (they live in the branch/commit/PR, where Linear resolves them anyway).
  2. Added tests. The existing test only covered 'none', so it passed either way and would not have caught a revert. There are now two: one asserting the backend path sends a "Shipping only" product as exempt (input side), one asserting its 0% line leaves the Standard row at 7.25% (output side). I verified the second fails against the old 'none' === guard and passes with the fix. Suite: 70 tests, 171 assertions green; PHPCS unchanged vs. baseline on both files.

Scope note for reviewers: you framed this as backend-only, but the guard runs on both paths — so a "Shipping only" product on the cart now also has its rate row skipped (previously written with a real rate). Safe, because core's is_taxable() gate discards it and the shipping tax row is written separately after this loop, but it is a cart-path behaviour change and I've noted it in the PR description.

❌ Rejected — case 2, variations

Not reproducible. WC_Product_Variation_Data_Store_CPT::read_product_data() line 481:

// Pull data from the parent when there is no user-facing way to set props.
$product->set_tax_status( $parent_data['tax_status'] );

Variations always inherit the parent's tax status — there is no per-variation Tax Status field. So "a variation with Tax Status None under a taxable parent" cannot exist, and wc_get_product( $product_id ) in the guard reads the same value $item->get_tax_status() did.

There is a genuine parent/variation divergence next door, but it's the $tax_class resolution on the line above — for a variation, $product_id is the parent, so a variation with its own Tax Class gets the wrong rate row. That predates this PR and is out of scope here; filed separately as WOOTAX-310.

❌ Rejected — case 3, deleted product

The $product && short-circuit does structurally bypass the guard, but the state that would make it matter is unreachable. WC_Order_Item_Product::get_tax_status() (core, line 482):

$product = $this->get_product();
return $product ? $product->get_tax_status() : ProductTaxStatus::TAXABLE;

If the product is gone, the item is reported taxable → no 99999 → TaxJar returns a real rate → there is no 0% line to clobber anything. The Woo 2.6 branch has the same $product ? … : 'taxable' fallback at line 1052. Reaching the described state would require a 99999 emission alongside a failed wc_get_product() — i.e. a live variation with a dead parent, which can't happen since the variation read pulls parent post meta.

The $product && check stays, though: it's what prevents a fatal on false->get_tax_status().

❌ Rejected — the proposed "complete fix"

Recording the emitted status in a backend_tax_statuses array alongside backend_tax_classes adds a second request-scoped mutable state array to a class that already has that smell, and it only buys the unreachable case above. Sticking with the minimal guard.

@bartech
bartech requested a review from Abdalsalaam July 27, 2026 09:26
// writing that 0% overwrites the shared row used by genuinely taxable
// products in the same class, zeroing tax for the whole cart. Skip the
// write; WooCommerce applies no item tax to either status anyway.
if ( $product && 'taxable' !== $product->get_tax_status() ) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The fix works for the case in the ticket, but there's one more route to the same zeroing that I think we should close here.

Taxability is filterable, so a product can be sent to TaxJar as exempt while still reporting Tax Status "taxable". The guard checks get_tax_status(), while get_line_items() sets the exempt code from ! $product->is_taxable(). When those disagree, the 0% line still wipes the shared Standard row.

Repro, with two Standard class products both set to Taxable:

add_filter( 'woocommerce_product_is_taxable', function ( $taxable, $product ) {
	return 123 === $product->get_id() ? false : $taxable;
}, 10, 2 );

Add the first product, note the rate under Standard Rates, then add the filtered one. Cart tax drops to 0 and every Standard row goes to 0%. I confirmed this against live TaxJar.

I think the fix is to stop re-deriving taxability on the response side. If get_line_items() and get_backend_line_items() record the keys they treat as non-taxable, get_itemized_tax_rates() can skip exactly those. WDYT?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed, and thanks — this is a real hole and the diagnosis is right. Implemented your
suggested design.

The divergence

WC_Product::is_taxable() (abstract-wc-product.php:1832) is filtered:

return apply_filters( 'woocommerce_product_is_taxable', $this->get_tax_status() === ProductTaxStatus::TAXABLE && wc_tax_enabled(), $this );

So the request side (get_line_items(), exempt code from ! is_taxable()) and the
response side (the guard, reading raw get_tax_status()) can disagree, exactly as you
describe — 0% line, shared Standard row zeroed. The previous guard was a third patch to
a predicate that shouldn't have been re-derived in the first place.

What changed

Request side records, response side reads:

  • private $non_taxable_line_items — keyed by TaxJar line item id
    (<product_id>-<cart_or_order_item_key>), the same key the breakdown echoes back.
  • get_line_items() records ! $product->is_taxable().
  • get_backend_line_items() records inside its existing 'taxable' !== $tax_status
    branch — i.e. exactly what it emitted as exempt.
  • get_itemized_tax_rates() skips on isset( $this->non_taxable_line_items[ $key ] ).
    The product lookup there no longer participates in the decision.

The two paths deliberately record different predicates, because core gates them
differently:

Path Core's item-tax gate Location
Cart $item->product->is_taxable() — filtered class-wc-cart-totals.php 677 / 735 / 796
Order ProductTaxStatus::TAXABLE === $this->get_tax_status() — raw class-wc-order-item.php 251

WC_Order_Item::calculate_taxes() never consults is_taxable(), so the order path has no
filter divergence to close and recording the raw status there is the accurate mirror.
Each side records what its own core gate will do.

One thing worth calling out: I did not implement this as "skip anything sent as
99999". Zero-rate-class products are emitted as 99999 for an unrelated reason and must
keep updating their own zero-rate row — so only the non-taxability reason is recorded.

Two earlier review points fall out for free: the deleted-product case @Abdalsalaam raised
is now moot (the lookup never dereferences a possibly-gone product), as is any
variation/parent-id mismatch at this seam, since the key is the emitted composite rather
than a re-resolved product id.

Tests

The two output-side tests fabricated breakdown keys and called get_itemized_tax_rates()
with no matching request — a shape that structurally cannot catch an emit/response
divergence, which is why this survived two rounds. Both now drive the request side first
and read the real key back. The "Shipping only" one also moved to the backend order
path
, which is where that status is actually emitted as exempt; it had been asserting a
cart scenario that can't occur.

New: test_get_itemized_tax_rates_filtered_non_taxable_line_does_not_zero_standard_rate,
reproducing your filter case, asserting both the input side (99999 emitted despite Tax
Status "taxable") and the output side (shared row stays 7.25%). Verified red against the
previous guard — it writes the row — while the "None" and "Shipping only" tests stay
green.

Gotcha for anyone re-running these: is_taxable() also requires wc_tax_enabled(), which
is off in the bare WC test bootstrap, so the cart tests set woocommerce_calc_taxes.

TaxJar class 80 tests / 206 assertions, full suite 230 / 510 green. PHPCS
--report=source byte-identical to baseline on both files.

BC

Nothing public moved: new property is private, no signature/hook/option changes, no new
global or WC() state reads, nothing site- or path-scoped. The only behaviour change is
which rate rows get written, and only for line items core does not tax.

Pushed as 834527da.

@bartech
bartech requested a review from dustinparker August 5, 2026 13:54
Comment on lines +992 to +994
if ( ! $product->is_taxable() ) {
$this->non_taxable_line_items[ $id . '-' . $cart_item_key ] = true;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The exempt branch at line 984 deliberately excludes Tax Status 'shipping' from the 99999 code, so on the cart path a "Shipping only" product is sent to TaxJar as genuinely taxable and its breakdown line returns a real, non-zero rate. But the new record at line 992 uses the unconditional ! $product->is_taxable(), which is false for any status other than 'taxable' — including 'shipping'. That line is therefore added to $non_taxable_line_items and its correct, non-zero rate is discarded at line 1787, where pre-fix it was written. This is the one case where the skip is not preventing a 0% clobber but discarding good data. The order path has no such asymmetry ('taxable' !== $tax_status at line 1081 is exactly the condition that sets 99999), so this cart/order divergence is unintended rather than the deliberate filtered-vs-raw split the PR documents. Concretely: for a "Shipping only" product in a NON-standard tax class with woocommerce_shipping_tax_class = 'inherit' (WC default), WC_Cart::get_cart_item_tax_classes_for_shipping() includes it because is_shipping_taxable() is true for 'shipping' status, so WC resolves the shipping tax class to that non-standard class and calls WC_Tax::get_shipping_tax_rates() against it. Pre-fix the item write kept that row current (it carries tax_rate_shipping, line 1923); post-fix nothing writes it, because TaxJar's separate shipping write at line 1828 uses tax class '' (standard) only. The PR body's safety argument covers item tax and the standard class but not this path.

Suggested change
if ( ! $product->is_taxable() ) {
$this->non_taxable_line_items[ $id . '-' . $cart_item_key ] = true;
}
if ( 'shipping' !== $product->get_tax_status() && ! $product->is_taxable() ) {
$this->non_taxable_line_items[ $id . '-' . $cart_item_key ] = true;
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed and applied verbatim — 5af84040. This is a regression my own fix introduced, and your framing of it is the part that made it land: this is the one case where the skip discards good data instead of preventing a 0% clobber. I had been reasoning about the guard purely as a clobber-preventer, so a false positive looked free. It isn't.

I checked each supporting claim against the code rather than taking the chain on trust, since the harm path is several hops long:

Claim Verified
Exempt branch excludes 'shipping', so the line is emitted as taxable 'shipping' !== $product->get_tax_status() && ( ! $product->is_taxable() || 'zero-rate' == … )
! $product->is_taxable() is true for 'shipping' is_taxable() is 'taxable' === get_tax_status() && wc_tax_enabled()
The item write is what carries tax_rate_shipping create_or_update_tax_rate(), 'tax_rate_shipping' => $freight_taxable
TaxJar's own shipping write is standard-class only the shipping-breakdown loop passes '' as the tax class

The divergence is in the recording, not the emission

Worth stating explicitly because it changes what the fix should be. The two paths do emit differently for this status — the cart path sends "Shipping only" as taxable, the order path sends it as 99999 — and that difference is pre-existing and deliberate. What diverged was that the order path records on 'taxable' !== $tax_status, which is exactly the condition that sets its 99999, so it records what it emitted; the cart path recorded on a condition that had drifted from what it emitted. Aligning the cart path restores the same rule on both sides rather than making the two paths identical.

One alternative I considered and rejected

'99999' === $tax_code && ! $product->is_taxable() is semantically identical here and has the appeal of reading the emitted value directly, so it cannot drift if the exempt branch changes. I did not take it: $tax_code is derived from end( explode( '-', $tax_class ) ), so a tax class whose slug ends in -99999 would collide. Vanishingly unlikely, but your version has no exotic failure mode and mirrors the branch above in the same idiom, so it is the better line.

Tests — both sides of the boundary

  • test_get_line_items_does_not_record_shipping_only_status_as_non_taxable — the regression test. Fails against the previous condition with '10-<cart_key>' => true recorded.
  • test_get_line_items_still_records_none_status_as_non_taxable — the boundary test. Passes in both states by design; it pins that the exclusion narrows by exactly one status, so a line genuinely emitted as exempt stays recorded and its 0% cannot come back.

The reason this got through is worth recording: the cart path had no coverage for this status at all. Both existing "Shipping only" tests are on the order path, where the recording happened to be correct — so the suite was green on the path where the rule held and silent on the path where it didn't.

Full suite 237 tests / 527 assertions green.

@bartech
bartech requested a review from Abdalsalaam August 5, 2026 16:28
WC_Helper_Product::delete_product( $product->get_id() );
}

public function test_get_backend_line_items_sends_shipping_only_status_as_exempt() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

phpcs.xml.dist imports .phpcs.php.xml, which loads WordPress-Docs globally (.phpcs.php.xml:11). Its tests/ exclusions cover only Generic.Commenting, WordPress.Files.FileName, WordPress.WP.GlobalVariablesOverride, WooCommerce.Commenting.CommentHooks and the two Squiz.Commenting.FileComment.* sniffs, so Squiz.Commenting.FunctionComment.Missing still applies to tests/. This is the only method of ~70 in the file without a docblock, so composer run check-all should error, contradicting the PR body's claim that PHPCS --report=source is byte-identical to baseline. The intended docblock already exists but is stranded at lines 2264-2269 above the wrong method; once this suggestion is applied, delete that stranded block.

Suggested change
public function test_get_backend_line_items_sends_shipping_only_status_as_exempt() {
/**
* Tax Status "Shipping only" is exempt from item tax, and the backend order path
* sends it to TaxJar as exempt (code 99999) — unlike the cart path, which excludes
* that status from the exempt branch. This is the input side of the zeroing bug:
* an exempt code is what makes TaxJar return the 0% breakdown line.
*/
public function test_get_backend_line_items_sends_shipping_only_status_as_exempt() {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed, and the reasoning holds at every step — I checked each of the three claims separately rather than taking the conclusion.

1. The ruleset claim. .phpcs.php.xml does load WordPress-Docs globally, and its tests/ exclusions are exactly the five you listed (Generic.Commenting, WordPress.Files.FileName, WordPress.WP.GlobalVariablesOverride, WooCommerce.Commenting.CommentHooks, and the two Squiz.Commenting.FileComment.*). Squiz.Commenting.FunctionComment is not among them, so it does apply to tests/. Config-reading alone doesn't settle it though — whether the sniff is actually pulled in is decided inside WordPress-Docs — so I ran it, and it fires:

2350 | ERROR | Missing doc comment for function
     |       | test_get_backend_line_items_sends_shipping_only_status_as_exempt()
     |       | (Squiz.Commenting.FunctionComment.Missing)

2. The stranded docblock. Lines 2264-2269 held the backend test's docblock sitting immediately above the cart test's own docblock. PHPCS binds a docblock to the immediately following token, so the orphan never attached to anything — the cart test kept its own comment and looked fine, and the method 60 lines below silently lost its docblock. Exactly as you described.

3. The PR body claim. You're right that it contradicts it, and this is the part I'd have missed with a sloppier check. Measured --report=source across both changed PHP files, against merge-base 590070bf:

violations sources FunctionComment.Missing
baseline 590070bf 192 31 3
PR head 5af84040 193 31 4
after 70b5d287 192 31 3 — byte-identical to baseline

Worth noting for anyone re-running it: the source count stays 31 on all three rows. The class file already carries 3 pre-existing FunctionComment.Missing, so the regression showed up only as a count bump, not a new source line. A "did a new sniff appear?" check would have come back clean — which is plausibly how the original claim was made in good faith. It needs a count-level diff.

Changed in 70b5d287: moved the docblock onto the method it documents and deleted the stranded copy. Pure 6-line move, no code touched; phpcbf was a no-op on it. Full suite green — 237 tests, 527 assertions.

On the usual regression test: deliberately none here, rather than an oversight. This is a comment-only relocation with no runtime behaviour to pin, so a PHPUnit test would assert nothing real, and nothing narrows a condition so there's no boundary case either. The two-directional check is the PHPCS measurement above: the violation is present without the change and absent with it, against a fixed baseline. Happy to add something if you see a behavioural angle I don't.

One flag unrelated to this change: npm run test-client currently fails in my environment on missing wpcom / react-pure-render modules, which is local node_modules breakage rather than anything on this branch (this commit touches one PHP file). PHP side is clean.

@bartech
bartech requested a review from Abdalsalaam August 6, 2026 08:56
* clause narrows the condition by exactly one status and no more.
*/
public function test_get_line_items_still_records_none_status_as_non_taxable() {
$product = WC_Helper_Product::create_simple_product();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Unlike its siblings at :2121 and :2191, this test never calls update_option('woocommerce_calc_taxes','yes'), and set_up() (:44) does not enable taxes. WC_Product::is_taxable() is "'taxable' === get_tax_status() && wc_tax_enabled()", so with taxes off it is false for every product. Both assertions ('99999' emitted at :2329, key recorded at :2334) therefore hold identically for a plain 'taxable'-status product, so the test cannot pin what its docblock claims at :2314 — that the "'shipping' !==" clause narrows the condition by exactly one status and no more.

Suggested change
$product = WC_Helper_Product::create_simple_product();
// is_taxable() is false for every product while taxes are off, so without this the
// assertions below hold for any Tax Status and cannot pin the 'shipping' exclusion.
update_option( 'woocommerce_calc_taxes', 'yes' );
$product = WC_Helper_Product::create_simple_product();

@dustinparker dustinparker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This fixes the bug. A cart mixing a taxable product with a Tax Status "None" product keeps calculating tax now, the shared Standard rate rows stay intact instead of being overwritten to 0%, and stores already carrying a zeroed row recover on their next calculation rather than staying stuck on it.

One thing before merge: I agree with @Abdalsalaam about test_get_line_items_still_records_none_status_as_non_taxable. With woocommerce_calc_taxes off, is_taxable() is false for every product, so those assertions hold for any Tax Status and the test can't pin the exclusion its docblock describes. His suggested one-liner covers it.

bartech added 6 commits August 7, 2026 15:09
…WOOTAX-240)

In a cart mixing a taxable product and a non-taxable one (Tax Status =
"None", Tax Class = "Standard"), taxes stopped calculating for the whole
cart. get_itemized_tax_rates() writes one WooCommerce tax rate row per
TaxJar breakdown line item, keyed by the product's Tax Class. The
non-taxable product is sent to TaxJar as exempt (code 99999) so its
breakdown line comes back at 0%, but its Tax Class is still Standard — so
create_or_update_tax_rate() overwrote the shared Standard rate row (already
populated by the taxable product) with 0%, zeroing tax for every standard
item. The zeroed row persisted in wp_woocommerce_tax_rates, which is why
adding another taxable product did not restore it.

Skip rate-row writes for line items whose product has Tax Status "None".
WooCommerce already applies no tax to such products, and this leaves the
shared class rate intact for genuinely taxable items. Zero-rate *class*
products are unaffected (they own a separate rate row).

Note: the tax status check in get_line_items() is not the cause — a
tax_status "none" product is already correctly sent as code 99999. The
defect was downstream, in how the itemized rate rows are written.

Adds a regression test driving get_itemized_tax_rates() with a mixed
taxable/exempt breakdown; it fails on trunk (exempt line writes/zeroes the
shared row) and passes with this fix.
The guard added for the mixed-cart zeroing bug only skipped rate-row
writes for Tax Status "None". The backend order path emits the exempt
code 99999 for any status other than "taxable", so a "Shipping only"
product still returned a 0% breakdown line that overwrote the shared
Standard rate row — the same failure, on the admin order screen.

Guard on `'taxable' !== $tax_status` instead. ProductTaxStatus defines
exactly three statuses and set_tax_status() normalises empty to
"taxable", so this is precisely "none or shipping". Skipping the write
is safe for both: WC_Cart_Totals gates item tax on is_taxable() and
WC_Order_Item::calculate_taxes() on ProductTaxStatus::TAXABLE, so
neither status is taxed by core. Shipping tax is unaffected — that rate
row is written separately after this loop.

Adds coverage for both sides of the case: that the backend path sends a
"Shipping only" product as exempt, and that its 0% line leaves the
Standard row intact.
The response-side guard read the raw tax status while get_line_items()
set the exempt code from is_taxable(), which is filtered through
woocommerce_product_is_taxable. When the two disagreed, the 0% TaxJar
breakdown line still overwrote the shared tax class rate row.

Record the decision where it is made and read it back by line item key,
so both halves share one predicate. The cart path records is_taxable()
and the order path records the raw status, matching how WC_Cart_Totals
and WC_Order_Item::calculate_taxes() each gate item tax. Line items sent
as exempt for the zero-rate class are not recorded and keep updating
their own rate row.

Drive the regression tests through the request side rather than
fabricating breakdown keys, which could not catch the divergence, and
move the "Shipping only" case to the backend order path where that
status is actually emitted as exempt.
The recording added on the cart path used an unconditional ! is_taxable(), which
is false for every Tax Status other than "taxable" -- including "shipping". The
exempt branch three lines above deliberately excludes that status, so a
"Shipping only" product is sent to TaxJar as genuinely taxable and its breakdown
line comes back with a real, non-zero rate. Recording it made
get_itemized_tax_rates() skip the write and throw that rate away.

This is the one case where skipping is not preventing a 0% clobber but
discarding good data. WooCommerce applies no item tax to such a product, so the
skip looks harmless, but the row it would have written carries
tax_rate_shipping: with the default woocommerce_shipping_tax_class = 'inherit',
WC_Cart::get_cart_item_tax_classes_for_shipping() includes the product (its
shipping is taxable), WooCommerce resolves the shipping tax class to that
product's class, and WC_Tax::get_shipping_tax_rates() looks the rate up there.
TaxJar's separate shipping write only ever covers the standard class, so for a
"Shipping only" product in a non-standard class nothing kept that row current.

The condition now mirrors the exempt branch, so the cart path records exactly
what it emitted. The backend order path already did: there the same
'taxable' !== $tax_status test is what sets the 99999 code, so recording on it
is correct rather than inconsistent. The divergence was between the two paths'
*recording*, not their emission -- the emission difference is pre-existing and
deliberate.

Two tests, covering both sides of the boundary:

- test_get_line_items_does_not_record_shipping_only_status_as_non_taxable --
  fails against the previous condition, which recorded '10-<key>' => true.
- test_get_line_items_still_records_none_status_as_non_taxable -- passes in both
  states, pinning that the exclusion narrows by exactly one status and that a
  line genuinely emitted as exempt stays recorded.

The cart path had no coverage for this status, which is why it slipped through;
the only "Shipping only" tests were on the order path.

Full suite 237 tests / 527 assertions green.
The docblock for test_get_backend_line_items_sends_shipping_only_status_as_exempt()
sat above the cart-path test instead, leaving the backend test with no docblock.
PHPCS binds a docblock to the immediately following token, so the orphan became a
floating comment and Squiz.Commenting.FunctionComment.Missing fired on the method
60 lines below it.

WordPress-Docs loads globally in .phpcs.php.xml and its tests/ exclusions do not
cover Squiz.Commenting.FunctionComment, so the sniff does apply to tests/.

Measured across both changed PHP files, --report=source:
  merge-base 590070b : 192 violations / 31 sources
  before this commit  : 193 / 31 (FunctionComment.Missing 3 -> 4)
  after this commit   : 192 / 31, byte-identical to baseline
The rebase onto trunk placed the WOOTAX-240 entry into the released
3.6.8 section instead of 3.6.12, in both changelog.txt and readme.txt.
The three-way merge anchored on the similar "unbounded growth of
WooCommerce tax rate rows" line that 3.6.8 already carried, and the
3.6.12 header the original commit had created now came from trunk.

No test covers changelog placement, so this was invisible to CI.
@bartech
bartech force-pushed the wootax-240-mixed-carts-of-taxable-and-non-taxable-products-dont branch from 70b5d28 to b4c5907 Compare August 7, 2026 13:11
@bartech
bartech merged commit 028ceb6 into trunk Aug 7, 2026
9 checks passed
@bartech
bartech deleted the wootax-240-mixed-carts-of-taxable-and-non-taxable-products-dont branch August 7, 2026 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants