[WOOTAX-288] Resolve PHPCS violations.

[bartech]

Description

Milestone 2 of the PHPCS work for this plugin — the cleanup counterpart to the M1 ruleset restructure in #2956. Resolves the PHPCS violations the M1 ruleset deliberately deferred, so that composer run check-all exits with no errors (warnings are allowed per the ticket's definition of done).

The change is overwhelmingly PHPCBF auto-fixes (docblocks, Yoda conditions, count() vs sizeof(), long-array syntax, alignment) plus a small set of narrowly-scoped ruleset exclusions. Two behavior-preserving points worth calling out for review:

• Reused WooCommerce core strings keep the woocommerce text domain. The dev-data REST controllers in classes/wc-api-dev/ (forks of core's /wc/v3/data controllers) and the tax-rate CSV export reuse strings verbatim from WooCommerce core and rely on core's translation catalog. They intentionally stay on the woocommerce domain, with WordPress.WP.I18n.TextDomainMismatch suppressed narrowly for just those strings, so translations aren't lost on non-English locales.
• No runtime behavior changes; the existing PHPUnit suite stays green.

⚠️ Stacked on #2956 — do not merge first. This PR is based on the M1 ruleset branch because the split ruleset it lints against only exists there. Once #2956 lands on trunk, this PR will be retargeted to trunk.

Related issue(s)

Closes https://linear.app/a8c/issue/WOOTAX-288

Steps to reproduce & screenshots/GIFs

PHPCS (the definition of done) — exits with no errors:

composer run check-all       # exit 0 — 0 errors, 168 warnings
composer run check-php
composer run check-security
composer run check-l18n

PHPUnit (no behavior change) — existing suite stays green:

composer test

Checklist

• unit tests — N/A: standards/config cleanup with no behavior change; no new tests. Existing suite stays green.
• changelog.txt entry added — N/A: PHPCS cleanup, no user-facing change.
• readme.txt entry added — N/A: PHPCS cleanup, no user-facing change.

[Abdalsalaam]

🤖 Automated AI pre-flight review — generated by Claude Code (ultra-review skill) on behalf of @Abdalsalaam.
This is NOT an approval and not a substitute for human review; it is an early automated pass to surface likely issues. Treat every finding as a suggestion to verify.

---

Verified — the diff-line-8972 change is just a plain end-of-line comment moved above the body (cosmetic, logic intact), not a scanner suppression. The only relocated scanner annotation is the nosemgrep in WooCommerceBlocksIntegration.php. I have everything needed to synthesize.

Ultra-Review — Automattic/woocommerce-services #2959

PR: [WOOTAX-288] Resolve PHPCS violations. (#2959) · author bartech
Base: trunk ← Head: wootax-288-address-phpcs-violations
Scope: 76 files, +3321 / −736 · Stack: WordPress/WooCommerce plugin (WooCommerce Shipping & Tax / woocommerce-services), PHP 7.4+ · WP plugin: yes
Issue: WOOTAX-288 — resolve the PHPCS violations the M1 ruleset (#2956) deferred, so composer run check-all exits with 0 errors.
Run mode: Offline, read-only, non-approving / advisory. No git/gh/build/live-test executed by design.
Reviewers dispatched: Correctness & Logic · Security · i18n · Conventions/Ruleset/Build/Tests · Supply-chain scanner annotations.

⚠️ This PR is stacked on #2956 ("do not merge first" per the description) and will be retargeted to trunk once #2956 lands. That ordering constraint is the author's; this review only assesses the diff content.

Verdict

SHIP WITH FIXES

• One real behavior regression hides among the cosmetic auto-fixes: (array) glob() is not false-safe and contradicts both the PR's "no runtime behavior changes" claim and the function's own documented invariant. Should be fixed before merge.
• One scanner-suppression annotation was relocated off its target line, which will likely resurface a previously-suppressed semgrep finding — ironic for a "make the linters pass" PR.
• Everything else (the 99% that is docblocks, Yoda flips, array syntax, wp_unslash/sanitization, the ruleset split, the i18n text-domain suppression) was independently verified as behavior-preserving and correctly scoped.

Problem Statement

Problem: PHPCS reports errors on the codebase; the M1 ruleset restructure deliberately deferred resolving them. M2 must clear the deferred violations so composer run check-all exits clean (0 errors; warnings allowed).
Context: Overwhelmingly PHPCBF auto-fixes (docblocks, Yoda conditions, count()/sizeof(), long→short array, alignment, }//end if markers) plus a small set of narrowly-scoped ruleset exclusions and a one phpcs.xml.dist → 4-file split. Two deliberate, behavior-preserving decisions are called out: (1) classes/wc-api-dev/ forks + tax-rate CSV export keep the woocommerce text domain to reuse core's translations; (2) no runtime behavior changes, PHPUnit stays green.
Acceptance criteria: composer run check-all → exit 0; existing PHPUnit suite stays green; no runtime behavior change.
Open questions: None stated.

Approach & Alternatives

Proposed approach. Run PHPCBF for the auto-fixable sniffs, hand-fix the rest, and split the monolithic phpcs.xml.dist into concern-specific rulesets (.phpcs.common.xml shared base; .phpcs.php.xml, .phpcs.l18n.xml, .phpcs.security.xml; aggregator phpcs.xml.dist) with new composer scopes (check-php, check-security, check-l18n, check-all). Genuinely-untranslatable or feature-gated cases are deferred via narrow, commented exclude-patterns and inline phpcs:ignores.

Assessment. This is the most direct path and the scoping discipline is good — exclusions are per-file lists + tests/ + classes/wc-api-dev/, not blanket category suppressions, and each carries a rationale comment. The risk in any "auto-fix everything" PR is that a manual edit made to satisfy a sniff quietly changes semantics; that is exactly what happened once (the glob() cast). The acceptance criterion "no runtime behavior change" is substantially met but not fully — see Majors.

Alternatives.

• Pure-PHPCBF + // phpcs:ignore for everything non-auto-fixable — zero behavior risk, but litters production code with ignores; the author rightly preferred real fixes for most cases.
• Split the i18n/security/php rulesets in a separate PR from the violation fixes — smaller, more reviewable diffs; trade-off is more PR churn. Acceptable as-is given the M1/M2 framing.

Meets acceptance criteria? check-all exit-0 and green PHPUnit are plausible and unverifiable offline. The "no runtime behavior change" claim is violated by the (array) glob() regression (Major #1) and weakened by the relocated nosemgrep (Minor #1).

Findings

Blockers (must fix before merge)

None.

Majors (should fix before merge)

1. (array) glob() is not false-safe — regresses the documented glob()-returns-false handling
classes/class-wc-connect-functions.php:394-397 (diff lines 1448-1451), function get_backed_up_tax_rate_files().

// before
$found_files = array_merge(
    glob( $backup_dir . '/taxjar-wc_tax_rates-*.csv' ) ?: array(),
    glob( $upload_dir['basedir'] . '/taxjar-wc_tax_rates-*.csv' ) ?: array()
);
// after
$found_files = array_merge(
    (array) glob( $backup_dir . '/taxjar-wc_tax_rates-*.csv' ),
    (array) glob( $upload_dir['basedir'] . '/taxjar-wc_tax_rates-*.csv' )
);

glob() returns false on a filesystem error. In PHP (array) false === array( 0 => false ) — a one-element array containing false, not an empty array. The old ?: array() mapped that error case to array(). Consequences when glob() errors:

• if ( empty( $found_files ) ) { return false; } (line 399) no longer triggers — [false] is non-empty — so the "no backups" early return is bypassed.
• basename( false ) (line 405) coerces to basename('') → '', so the function returns array( '', ... ) (empty-string "filenames") instead of false.

This directly contradicts the same function's own comment three lines above (lines 384-387): "Check for false explicitly: glob() returns false on filesystem error (not just an empty array), and empty(false) would be true…". It also contradicts the PR's "no runtime behavior changes" claim. The likely trigger for the edit was a no-short-ternary sniff; the false-safe + lint-clean fix is:

$backup_files = glob( $backup_dir . '/taxjar-wc_tax_rates-*.csv' );
$root_files   = glob( $upload_dir['basedir'] . '/taxjar-wc_tax_rates-*.csv' );
$found_files  = array_merge(
    is_array( $backup_files ) ? $backup_files : array(),
    is_array( $root_files ) ? $root_files : array()
);

Severity rationale: not a Blocker because it only manifests on a rare glob() filesystem error (off the golden path) and causes no data loss/crash — but it is a genuine, edge-case correctness regression in tax-rate-backup discovery that the codebase explicitly guards against elsewhere. (One reviewer rated this BLOCKER; downgraded to MAJOR under the honest-calibration rule.)

Minors (nice to fix)

• Relocated nosemgrep no longer covers its target. src/Integrations/WooCommerceBlocksIntegration.php:105-106 (diff 7234→7242). The // nosemgrep: audit.php.lang.security.file.inclusion-arg comment moved from the end of the ? require $script_asset_path : array(); line to its own line below that statement. Semgrep's inline ignore applies to the same line or the line immediately above the finding — placing it below the require means it now annotates the unrelated $script_dependencies line and no longer suppresses the file-inclusion finding. If semgrep runs in CI, the previously-suppressed (and genuinely safe) finding will resurface. Fix: put the comment back at the end of the require line, or on the line directly above it.

Nits (optional)

• classes/class-wc-connect-taxjar-integration.php:~6101: a thrown Exception message is now wrapped in esc_html( sprintf( … ) ) (to satisfy WordPress.Security.EscapeOutput). Harmless here (the message is country/state codes + a JSON bool), but escaping a not-yet-rendered exception message is a code smell; a targeted // phpcs:ignore would express intent more honestly than mutating the message text. json_encode→wp_json_encode in the same line is a correct improvement.
• classes/class-wc-connect-migration-survey.php:124 (diff 2233): the new isset(...) ? … : array() guard changes the not-set path from json_decode('') (→ null) to array(); both are falsy so the downstream if ( ! $survey_data ) error path is unchanged. Behavior-preserving and a slight robustness improvement — noted only for completeness.

Testing Instructions

(Offline run — not executed here; provided for the human reviewer / CI.)

Setup. Check out the branch on top of #2956; composer install; a WooCommerce store with the plugin active.

Happy path (acceptance criteria).

1. composer run check-all → expect exit 0, 0 errors (warnings allowed).
2. composer run check-php, composer run check-security, composer run check-l18n → each exit 0.
3. composer test (PHPUnit) → suite green.

Regression checks (the parts that changed behavior, not just formatting).

1. Tax-rate backup discovery (Major #1): unit-test WC_Connect_Functions::get_backed_up_tax_rate_files() with the backups directory made unreadable so glob() returns false. Assert it returns false (or []), not an array containing empty strings. This is the case the cast regresses.
2. Migration survey submit (handle_survey_submission): POST with and without survey_data; with malformed JSON; confirm Invalid survey data for empty/missing, and that valid payloads still record tracks with per-value sanitize_text_field.
3. Label reports date column: render the shipping-label report; confirm dates display identically to trunk (the date()→gmdate() swap is a UTC no-op in a WP runtime — verify no off-by-timezone shift if the host overrides PHP's default timezone).
4. i18n: load a non-English locale; confirm classes/wc-api-dev/ data endpoints (continents/locations) and the tax-rate CSV export headers are still translated via core's catalog, and the rest of the plugin still resolves under woocommerce-services.
5. Semgrep (Minor #1): if CI runs semgrep, confirm the file.inclusion-arg finding in WooCommerceBlocksIntegration::register_script() is still suppressed.

Edge cases. glob() filesystem error (case 1); survey_data present but false/empty JSON; superglobals containing backslashes/quotes (the new wp_unslash paths — range, from_order, carrier, taxjar country/state/postcode/city/street); locale with non-Latin scripts.

Not flagged but worth knowing (verified, no action needed)

• wp_unslash() additions (label-reports range; shipping-label from_order/carrier; taxjar wc_clean(wp_unslash(...)) for country/state/postcode/city/street; reroute nonce): all correct unslash-before-sanitize fixes; the values involved make the slash-handling change benign.
• date() → gmdate() (class-wc-connect-label-reports.php): no-op in a WP runtime (WP forces PHP default TZ to UTC) and strictly more correct. Not a regression.
• Security ruleset reorder (.phpcs.security.xml): only reordering + import refactor — EscapeOutput, ValidatedSanitizedInput(.InputNotSanitized), SafeRedirect, NonceVerification, the eval() ban, and ignore_warnings_on_exit=1 are all preserved. No sniff removed or weakened.
• i18n text-domain suppression is correctly scoped to classes/wc-api-dev/ (verified: exactly the two core-fork data controllers) and the CSV-header block; the standalone domain edits elsewhere (woocommerce_services→woocommerce-services, adding missing domains) are fixes, not losses.
• Comment-sniff exclusions are per-file + tests/ + wc-api-dev/, not blanket across classes/. Feature-gating rationale verified: the excluded controllers all load inside if ( ! self::is_wc_shipping_activated() ) (woocommerce-services.php:1594-1660), while the two explicitly-NOT-excluded controllers (shipping-carriers, shipping-label-eligibility) load unconditionally — the PR's distinction matches the code.
• WP version bump 6.7 → 6.9 (.phpcs.common.xml): intended and consistent with readme.txt (Requires at least: 6.9).
• Composer scripts / aggregator point at real standard files; the legacy phpcbf script and the husky bin/wc-phpcbf.sh hook still work. AGENTS.md update is accurate; no stale composer phpcs references remain.
• Test changes are cosmetic (docblocks, dirname(__FILE__)→__DIR__, visibility/spacing). The one removed assert is re-added with WPCS spacing; no markTestSkipped/weakened assertions/deleted expectations.
• PrefixAllGlobals whitelist (bare woocommerce, wcservices, taxjar, wcship, etc.) is a PHPCS-only naming-convention relaxation for this first-party plugin's public hooks/constants — no runtime or security effect.

Reviewer roll-up

• Correctness & Logic: 0 blockers, 1 major ((array) glob() regression), 0 minors. Confirmed date()→gmdate() is a safe no-op.
• Security: 0 findings. All added sanitization/escaping/wp_unslash/phpcs:ignores verified as correct fixes or justified false-positive suppressions; security ruleset change is inert.
• i18n: 0 findings. Text-domain suppression correctly scoped; domain edits are improvements.
• Conventions / Ruleset / Build / Tests: 0 findings. Ruleset split, feature-gating, version bump, composer scripts, and test changes all check out.
• Supply-chain scanner annotations: 1 minor (nosemgrep relocated off its target line).

Follow-ups (advisory — no writes performed)

1. Fix Major #1 ((array) glob() → is_array() guard) before merge.
2. Restore the nosemgrep comment to the require line (Minor #1).
3. Optional: replace the esc_html()-wrapped exception message with a scoped // phpcs:ignore (Nit).