Commit 1512d21
authored
[WOOTAX-313] Refactor - Migrate the TaxJar rate-table seam onto the Address value object (#2988)
* Add - Characterization tests for the TaxJar rate-table seam.
Golden-masters the `wp_woocommerce_tax_rates` round trip before
`create_or_update_tax_rate()` and `allow_street_address_for_matched_rates()`
are migrated onto the address value object. Duplicate rate rows are the
failure mode this seam produces, and they are silent — the right rate is
still charged, so nothing surfaces until the table has grown a row per
checkout. Every case therefore calls twice and asserts on the row delta.
Measuring the current behaviour corrected two of the three asymmetries the
refactor plan attributes to this seam:
- The postcode asymmetry (written through `wc_normalize_postcode()`, read
raw) is real but unreachable: `WC_Tax::find_rates()` applies the identical
`wc_normalize_postcode( wc_clean( … ) )` to its own argument, so core
reconciles it. ZIP+4 round-trips today.
- The state asymmetry is the reverse of what the plan describes. Core's
`prepare_tax_rate()` pushes `tax_rate_state` through `sanitize_key()`, so
the column holds a *more* normalised value than either reader asks for.
`'N Y'` round-trips only because `str_replace( ' ', '' )` coincides with
`sanitize_key()` for a space; `'N.Y.'` inserts a fresh row every time.
Three defects are pinned as-is rather than fixed here, so that the fix
appears as a deliberate change to this file:
- a state carrying anything outside `[a-z0-9_-]` duplicates rows on every
calculation;
- a city carrying anything `sanitize_text_field()` removes does the same,
because the write applies `wc_clean()` and the lookup does not;
- `allow_street_address_for_matched_rates()` normalises the state not at all,
so it cannot see rows that `create_or_update_tax_rate()` can — the
`woocommerce_matched_rates` path silently returns no rates for those
addresses while itemized cart totals stay correct.
Also pins the parts that must survive untouched: ordinal-position matching of
priorities, in-place rate updates, and the VAT branch writing no location rows.
* Refactor - Migrate the TaxJar rate-table seam onto the Address value object.
`create_or_update_tax_rate()` and `allow_street_address_for_matched_rates()`
now derive their `find_rates()` arguments and their `location_code` rows from
one `Address`, through a matched pair of projections the value object owns —
`to_find_rates_args()` and `to_rate_table_locations()`. Deriving them
separately is what let this seam look up values it had never written and
insert a fresh `wp_woocommerce_tax_rates` row for the same address on every
calculation.
Three defects the characterization tests pinned are fixed:
- A state carrying anything outside `[a-z0-9_-]` duplicated a rate row per
calculation. `WC_Tax::prepare_tax_rate()` singles `tax_rate_state` out for
`sanitize_key()` before storing it, and the lookup only stripped spaces —
an approximation that happened to be right for `'N Y'` and wrong for
`'N.Y.'` and for anything non-ASCII. `Address::state_compact()` now mirrors
the core function instead of approximating it, so the lookup asks for the
value core actually stored.
- A city carrying anything `sanitize_text_field()` removes did the same: the
write applied `wc_clean()` and the lookup did not. Sanitisation now happens
once, inside the shared projection.
- `allow_street_address_for_matched_rates()` normalised the city but not the
state, so it could not see rows `create_or_update_tax_rate()` had just
written. Rate rows existed and the `woocommerce_matched_rates` path — price
display, shipping tax, coupons — returned nothing for those addresses,
while itemized cart totals stayed correct.
Two of the plan's three target asymmetries turned out not to be live, and the
tests say so rather than the code pretending to fix them: `find_rates()`
applies the same `wc_normalize_postcode( wc_clean( … ) )` to its argument that
the write applies, so ZIP+4 always round-tripped; and core upper-cases the
city on both ends, so the plugin's asymmetric `strtoupper()` was unobservable.
Behaviour deltas beyond the fixes above:
- A comma-list postcode is now stored as its first segment rather than whole.
The row is keyed by the segment the rate was actually quoted for — the
TaxJar request body has carried only that segment since the request seam
moved onto the value object.
- `get_backend_address()` no longer upper-cases postcode, city and street.
That was this path's own historic behaviour, unmatched by `get_address()`,
and kept only because the city reaches the rate-table city column — which
is now upper-cased at both the write and the lookup. The admin recalculate
and the cart now put identically-cased values on the wire. This changes the
request JSON, so one generation of the `md5( $json )` request cache is
invalidated.
- `tax_rate_country` is trimmed before storage, so a whitespace-padded country
can no longer produce a row nothing matches.
- `create_or_update_tax_rate()` tolerates a missing or non-scalar location
field instead of emitting a notice or storing the literal "Array". It is
public and takes the location it is handed.
`WC_Connect_TaxJar_Integration::normalize_city()` now raises its deprecation
notice. It could not before: the last three in-repo callers were all on this
seam, and the notice would have fired on every checkout. The method itself
stays — it is `protected static`, so an out-of-repo subclass may call it.
* Tweak - Add changelog entries for the TaxJar rate-table seam fixes.
* Add - Pin the sanitize_key() collapse cases in state_compact().
Requested independently by both reviewers on the rate-table seam PR: the changelog
cites a non-Latin state as the motivating case, but no provider row exercised one,
so the behavior rested on reasoning rather than on a test.
Adds five rows. Three collapse to empty outright (Cyrillic, CJK, a bare
diacritic). Two collapse only partially -- 'IF' with a circumflex becomes 'F',
'AL' with a ring above becomes 'L' -- and those are the dangerous shape, because
the result is a shorter but still perfectly plausible state code. That is exactly
the duplication path this seam exists to close: the row is stored under the
collapsed value and looked up under the raw one, so it can never be found again
and a fresh row is inserted on every calculation.
Expected values were read off WordPress's own sanitize_key() rather than derived
by hand; the test's existing second assertion re-checks that agreement per row.
All five fail against the pre-migration str_replace() implementation.
* Tweak - Stamp the normalize_city() runtime notice with 3.6.12, not 3.6.11.
3.6.11 shipped on 2026-08-05 while this stack was open, so the version passed to
wc_deprecated_function() names a release that went out without the deprecation
in it. This PR's changelog entries land in the unreleased 3.6.12 block, which is
the release that will actually raise the notice.
The audience is the reason this matters. normalize_city() is protected static
and retained solely so an out-of-repo subclass does not break on load; the
notice is the only signal those consumers get, and it tells them which release
to check their migration against. Pointing at 3.6.11 sends them to a release
where nothing changed.
Matches the @deprecated tag, corrected on the base branch when it was added.
Tests are unaffected: setExpectedDeprecated() matches on the function name, not
the version string.
* Tweak - Narrow the rate-row growth claim to the states that actually duplicated.
The changelog entry named three triggers — a period, a space, and a non-Latin
character. Measured against core, only one of the three is right.
WC_Tax::prepare_tax_rate() stores tax_rate_state as strtoupper( sanitize_key() ),
and WC_Tax::get_matched_tax_rates() matches it with `tax_rate_state IN ( %s, '' )`.
So a row is unfindable — and duplicated on every calculation — only when the
stored value is non-empty *and* differs from what the lookup asked for.
A space never diverged: the old str_replace( ' ', '' ) lookup happened to agree
with sanitize_key() there, and only there. A state that collapses to '' outright
never diverged either, because the blank satisfies the second arm of the IN
clause whatever was asked for. What actually duplicated is a state that collapses
*partially*, to a shorter non-empty code: 'N.Y.' -> 'NY', 'ÎF' -> 'F'.
Rewords the entry to key on the mechanism rather than on scripts, and applies the
same correction to readme.txt, which carries the published verbatim copy. The
state_compact() docblock carried the same overbroad claim and gains the boundary.
* Add - Pin the sanitize_key() collapse boundary at the rate-table seam.
state_compact_provider() already pins what the *projection* returns for a
collapsing state. Nothing pinned what that collapse does to the rate table, which
is the claim the changelog entry actually makes — and the reason the entry was
able to drift.
Adds two rows to provide_rate_table_round_trips(), which feeds both the round-trip
test and the matched-rates lookup test, so two rows are four cases:
- 'state partially collapsing' (RO / 'ÎF', stored as 'F') is a regression test.
Against the pre-migration space-stripping lookup both its cases fail: the
round trip inserts a second row, and allow_street_address_for_matched_rates()
cannot see the row create_or_update_tax_rate() had just written.
- 'state wholly non-Latin' (RU / 'ЛЕН', stored as '') is a boundary
characterization and passes with or without the migration. The blank matches
the '' arm of core's `tax_rate_state IN ( %s, '' )`, so this shape never
duplicated. It also pins that such an address yields a country-wide rate.
Together they are the narrowing: the same "non-Latin character" trigger, opposite
outcomes, decided by whether anything survives the stripping. Both cities are
plain ASCII on purpose, so the state dimension is not confounded with the separate
byte-parity contract to_find_rates_args() keeps with core.1 parent b01e71f commit 1512d21
6 files changed
Lines changed: 952 additions & 65 deletions
File tree
- classes
- src/Tax
- tests/php
- Tax
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
| 12 | + | |
| 13 | + | |
12 | 14 | | |
| 15 | + | |
13 | 16 | | |
14 | 17 | | |
15 | 18 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
709 | 709 | | |
710 | 710 | | |
711 | 711 | | |
712 | | - | |
713 | | - | |
| 712 | + | |
| 713 | + | |
| 714 | + | |
| 715 | + | |
| 716 | + | |
| 717 | + | |
| 718 | + | |
| 719 | + | |
| 720 | + | |
| 721 | + | |
| 722 | + | |
| 723 | + | |
| 724 | + | |
| 725 | + | |
| 726 | + | |
714 | 727 | | |
715 | 728 | | |
716 | 729 | | |
717 | | - | |
718 | | - | |
719 | | - | |
720 | | - | |
721 | | - | |
722 | | - | |
723 | | - | |
724 | | - | |
725 | | - | |
726 | | - | |
727 | | - | |
728 | | - | |
729 | | - | |
730 | | - | |
| 730 | + | |
| 731 | + | |
| 732 | + | |
| 733 | + | |
| 734 | + | |
731 | 735 | | |
732 | | - | |
| 736 | + | |
| 737 | + | |
733 | 738 | | |
734 | 739 | | |
735 | 740 | | |
| |||
935 | 940 | | |
936 | 941 | | |
937 | 942 | | |
| 943 | + | |
| 944 | + | |
| 945 | + | |
| 946 | + | |
| 947 | + | |
| 948 | + | |
| 949 | + | |
938 | 950 | | |
939 | 951 | | |
940 | 952 | | |
941 | | - | |
942 | | - | |
943 | | - | |
944 | | - | |
945 | | - | |
946 | | - | |
947 | | - | |
948 | | - | |
949 | | - | |
950 | | - | |
951 | | - | |
952 | | - | |
953 | | - | |
954 | | - | |
955 | | - | |
956 | | - | |
957 | | - | |
958 | | - | |
| 953 | + | |
959 | 954 | | |
960 | 955 | | |
961 | 956 | | |
| |||
1859 | 1854 | | |
1860 | 1855 | | |
1861 | 1856 | | |
1862 | | - | |
1863 | | - | |
1864 | | - | |
| 1857 | + | |
| 1858 | + | |
| 1859 | + | |
| 1860 | + | |
| 1861 | + | |
1865 | 1862 | | |
1866 | 1863 | | |
1867 | 1864 | | |
| |||
1870 | 1867 | | |
1871 | 1868 | | |
1872 | 1869 | | |
| 1870 | + | |
| 1871 | + | |
1873 | 1872 | | |
1874 | 1873 | | |
1875 | 1874 | | |
| |||
1890 | 1889 | | |
1891 | 1890 | | |
1892 | 1891 | | |
1893 | | - | |
1894 | | - | |
1895 | | - | |
1896 | 1892 | | |
1897 | 1893 | | |
| 1894 | + | |
| 1895 | + | |
| 1896 | + | |
| 1897 | + | |
| 1898 | + | |
| 1899 | + | |
| 1900 | + | |
| 1901 | + | |
| 1902 | + | |
| 1903 | + | |
| 1904 | + | |
| 1905 | + | |
| 1906 | + | |
| 1907 | + | |
| 1908 | + | |
| 1909 | + | |
| 1910 | + | |
| 1911 | + | |
| 1912 | + | |
| 1913 | + | |
| 1914 | + | |
| 1915 | + | |
| 1916 | + | |
| 1917 | + | |
| 1918 | + | |
| 1919 | + | |
1898 | 1920 | | |
1899 | 1921 | | |
1900 | 1922 | | |
| |||
1918 | 1940 | | |
1919 | 1941 | | |
1920 | 1942 | | |
1921 | | - | |
1922 | | - | |
| 1943 | + | |
| 1944 | + | |
1923 | 1945 | | |
1924 | 1946 | | |
1925 | 1947 | | |
| |||
1931 | 1953 | | |
1932 | 1954 | | |
1933 | 1955 | | |
1934 | | - | |
1935 | | - | |
1936 | | - | |
1937 | | - | |
1938 | | - | |
1939 | | - | |
1940 | | - | |
1941 | | - | |
1942 | | - | |
| 1956 | + | |
1943 | 1957 | | |
1944 | 1958 | | |
1945 | 1959 | | |
| |||
1970 | 1984 | | |
1971 | 1985 | | |
1972 | 1986 | | |
1973 | | - | |
1974 | | - | |
| 1987 | + | |
| 1988 | + | |
| 1989 | + | |
| 1990 | + | |
1975 | 1991 | | |
1976 | 1992 | | |
1977 | 1993 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
79 | 79 | | |
80 | 80 | | |
81 | 81 | | |
| 82 | + | |
| 83 | + | |
82 | 84 | | |
| 85 | + | |
83 | 86 | | |
84 | 87 | | |
85 | 88 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
352 | 352 | | |
353 | 353 | | |
354 | 354 | | |
355 | | - | |
356 | | - | |
357 | | - | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
| 376 | + | |
| 377 | + | |
358 | 378 | | |
359 | 379 | | |
360 | 380 | | |
361 | 381 | | |
362 | | - | |
| 382 | + | |
363 | 383 | | |
364 | 384 | | |
365 | 385 | | |
| |||
601 | 621 | | |
602 | 622 | | |
603 | 623 | | |
604 | | - | |
605 | | - | |
| 624 | + | |
| 625 | + | |
| 626 | + | |
| 627 | + | |
| 628 | + | |
606 | 629 | | |
607 | 630 | | |
608 | 631 | | |
| |||
611 | 634 | | |
612 | 635 | | |
613 | 636 | | |
614 | | - | |
615 | | - | |
| 637 | + | |
| 638 | + | |
616 | 639 | | |
617 | 640 | | |
618 | 641 | | |
619 | 642 | | |
| 643 | + | |
| 644 | + | |
| 645 | + | |
| 646 | + | |
| 647 | + | |
| 648 | + | |
| 649 | + | |
| 650 | + | |
| 651 | + | |
| 652 | + | |
| 653 | + | |
| 654 | + | |
| 655 | + | |
| 656 | + | |
| 657 | + | |
| 658 | + | |
| 659 | + | |
| 660 | + | |
| 661 | + | |
| 662 | + | |
| 663 | + | |
| 664 | + | |
| 665 | + | |
| 666 | + | |
| 667 | + | |
| 668 | + | |
| 669 | + | |
| 670 | + | |
| 671 | + | |
| 672 | + | |
| 673 | + | |
| 674 | + | |
| 675 | + | |
| 676 | + | |
| 677 | + | |
| 678 | + | |
| 679 | + | |
| 680 | + | |
| 681 | + | |
| 682 | + | |
| 683 | + | |
| 684 | + | |
| 685 | + | |
| 686 | + | |
| 687 | + | |
| 688 | + | |
| 689 | + | |
| 690 | + | |
620 | 691 | | |
621 | 692 | | |
622 | 693 | | |
| |||
0 commit comments