feat: integrator string overrides + per-user locale (closes #66, #140)#143
Open
zackkatz wants to merge 8 commits into
Open
feat: integrator string overrides + per-user locale (closes #66, #140)#143zackkatz wants to merge 8 commits into
zackkatz wants to merge 8 commits into
Conversation
Implements the scoped i18n architecture (issues #66 and #140) in a shape designed to look like idiomatic WordPress code: - src/Strings.php: registry of overrideable string keys (class constants), Strings::get($key, $default, $context = []) accessor, Strings::load_translations($integrator_textdomain) opt-in entry point. Runtime textdomain routing means each Strauss-prefixed SDK image loads under its own textdomain — no cross-plugin collision. - src/Config.php: Config::validate_strings() walks the optional `strings` array and discards malformed overrides individually. Placeholder-safety check uses a behavioral sentinel test against vsprintf (not a regex over sprintf grammar) so it catches "missing placeholders" AND "extra placeholders" AND "wrong conversion type" in one shot. Bad entries log a warning; valid entries are kept. - src/Form.php: 5 highest-visibility user-facing strings migrated to the new accessor as proof points. The literal __() calls stay at the call site so `wp i18n make-pot` extraction still works. - src/SupportUser.php: support_user/locale config setting + trustedlogin/{ns}/support_user/locale filter. Format validation via regex (variant nested inside region group so de_de doesn't parse as "de + variant"). Locale lands in wp_insert_user($args) so wp_new_user_notification_email fires in the right language; defensive re-assert covers wp_pre_insert_user_data filters that strip unknown fields. Tests (49 new, all 179 still green; PHPStan clean): - test-strings.php: override resolution, placeholder enforcement (positive + negative cases including object overrides, escaped %%, missing/extra placeholders), closure invocation with $context positional args, runtime filter, unknown-key drop. - test-strings-translation.php: load_translations sets runtime textdomain, lookups route through it, override preempts translation, closure override can call __() against integrator textdomain, runtime filter sees user-locale context for context-aware overrides, change_locale callback runs without crashing when no .mo present. - test-support-user-locale.php: configured locale lands on the new user, get_user_locale() returns it, switch_to_user_locale() activates it, locale persists across simulated logins, defensive re-assert kicks in when wp_pre_insert_user_data strips the field, filter can override or clear, format validation rejects shell-injection / path-traversal / wrong-case-region attempts, accepts pt_PT_ao90 / ckb / ckb_IQ. This is a prototype on a feature branch — NOT a release. Open questions: 1. Should this ship as Approach A (English-only SDK with optional integrator overrides; SDK ships no .mo files yet) or Approach B (SDK builds + ships its own .mo files for a curated locale set)? The expert review pushed for B given the SDK's value prop of helping support agents serve non-English customers. This commit leaves both paths open: Strings::load_translations() works today as a routing primitive; shipping .mo files is a separate CI + translation-distribution decision. 2. Should Strings::get() be split into get() + get_plural() for _n() coverage, or keep one method with a closure-shape override for plurals? Current shape relies on closures, which delegate plural resolution to the integrator's own gettext.
All 117 unique user-facing strings across the SDK now route through
$this->strings->get(KEY, __('English', 'trustedlogin')). The __()
literals stay in place at every call site so wp i18n make-pot
extraction continues to work; runtime override resolution happens
inside the accessor.
Coverage:
Admin.php: 4 strings
Ajax.php: 2 strings
Client.php: 3 strings
Encryption.php: 2 strings
Endpoint.php: 2 strings
Form.php: 89 strings (the bulk — consent screen, headers,
error banners, success messages,
debug panel, brute-force lockdown)
Remote.php: 5 strings (handle_response error paths only —
see "static methods" below)
SecurityChecks.php: 2 strings
SiteAccess.php: 2 strings
SupportRole.php: 1 string
SupportUser.php: 2 strings
Per-class plumbing: each class that uses translations now has a
private `$strings` property initialized in the constructor from
`new Strings( $config )`. Pattern is consistent — Form, Admin, Ajax,
Client, Encryption, Endpoint, Remote, SecurityChecks, SiteAccess,
SupportRole, SupportUser all gained the property + assignment.
Strings.php now registers 117 constants + matching registry entries,
each declaring its sprintf placeholder count. Config::validate_strings
checks integrator overrides against this schema using a sentinel-
based behavioral vsprintf test (catches missing/extra/wrong-type
placeholders in one shot, no regex-over-sprintf-grammar maintenance
burden).
Static methods deliberately NOT migrated. Remote::check_response_code
and Remote::body_looks_like_html are static utility methods — they
can't reference $this->strings. Their 9 strings remain as plain
esc_html__() literals. They're still POT-extractable and translatable
via the SDK's own .mo files (Approach B); they just aren't
overridable through the strings config. Real users will rarely want
to override these anyway (they're HTTP-status-specific error
messages mostly visible in debug logs).
Constant names are auto-generated from the first 5-6 words of each
English string, with a 6-char hex suffix on collisions. They're
SemVer-stable: renames are breaking. A handful (USER_NOT_CREATED…,
VENDOR_HAS_SITE_ACCESS_THAT, SUPPORT_ACCESS_VENDOR_*) were renamed
by hand to either avoid PHP's "no leading digit" rule or to
disambiguate 60-char-truncated registry-comment collisions.
Tests: PHPUnit 179/179, 684 assertions, all green. PHPStan clean
under phpVersion: 70400. Existing test references to the old
hand-picked constants (SECURED_BY, REVOKE_ACCESS_BUTTON, etc.)
updated to the auto-generated equivalents (SECURED_BY_TRUSTEDLOGIN,
REVOKE_ACCESS, CREATED_1_S_AGO_BY_2, SUPPORT_ACCESS_IS_TEMPORARILY
_UNAVAILABLE_PLEASE).
Naming-curation pass is a reasonable follow-up — the auto-generated
keys work but some are unwieldy. Doing this in a separate PR keeps
THIS one mechanical and reviewable.
The instance-method shape was forcing every class to carry a private \$strings property and a constructor assignment, AND it couldn't be used from static methods (Remote::check_response_code had to leave 9 strings as plain esc_html__()). Static shape removes all of that: Strings::init( Config \$config ) // called once by Client::__construct Strings::get( \$key, \$default ) // called from anywhere, including static methods Strings::reset() // test-seam for tearDown What the refactor does: - src/Strings.php: \$config and \$overrides become static. init() binds them, reset() clears them. get() and resolve() drop \$this for self::. A missing init() degrades gracefully — overrides fall through to defaults and the runtime filter is skipped. - src/Client.php: one line — Strings::init( \$config ) right after Config validates. Subsequent SDK code uses Strings::get() without thinking about wiring. - 11 classes (Admin/Ajax/Client/Encryption/Endpoint/Form/Remote/ SecurityChecks/SiteAccess/SupportRole/SupportUser) lose their private \$strings property AND the constructor assignment \$this->strings = new Strings(\$config). Net 22 lines deleted. - 108 call sites: \$this->strings->get( → Strings::get(. Mechanical sed across the codebase. - src/Remote.php: the 9 strings in check_response_code() that had to remain as plain esc_html__() under the instance API are now migrated into the registry alongside the rest. Static methods participate fully. - Tests: \$strings = new Strings(\$config) → Strings::init(\$config), \$strings->get(...) → Strings::get(...). reset_strings_state() in tearDown collapses to a single Strings::reset() call. PHPStan: clean under phpVersion=70400 (after marking \$config as Config|null since reset() unsets it). PHPUnit: 179/179, 684 assertions. Diff stat: 14 files changed, 206 insertions(+), 241 deletions(-). Net -35 lines.
# Bug fixes (from code review)
## 1. EXTEND_S_ACCESS dropped a sprintf argument
src/Form.php line 1434 called sprintf with two args (vendor name +
human-readable duration) against a default '%s' format that only
consumed one. The duration silently vanished from the
"Extend Acme Access for 1 week" button. Fixed by:
- Renaming the constant: EXTEND_S_ACCESS → EXTEND_VENDOR_ACCESS_FOR_DURATION
- Bumping the registry from placeholders=1 to placeholders=2
- Updating the default from "Extend %s Access" to "Extend %1$s Access for %2$s"
## 2. Config::validate_strings defensive cache flush
Config::get_setting() caches per-key. validate_strings() mutates
$this->settings['strings'] in place. The current Client::__construct
call order is safe, but a future refactor that touches
get_setting('strings') from inside validate() would silently bypass
override pruning. Drop the cache slot at the tail of validate_strings.
## 3. change_locale closure captured a stale textdomain
Strings::load_translations() registered the change_locale hook only
once but the closure captured $textdomain by value. A second
load_translations('beta-plugin') would update self::$textdomain but
the previously-registered closure still loaded .mo against the FIRST
textdomain. Closure now reads self::$textdomain at fire time.
# Translator-comment placement fixes
Added tests/test-translator-comments-extracted.php — a meta-test
that runs `wp i18n make-pot` against src/ and asserts every
// translators: comment in source survives extraction into the POT.
The test caught FIVE existing placement bugs where the comment was
on the outer sprintf/esc_html wrapper line, too far from the inner
__() literal for the extractor to associate it:
- Form.php:380-381 (Contact/Email support text) — comment swapped
onto the wrong msgid because of inline-comment heuristics
- Form.php:845 (Site Health link) — multi-line wrap distance
- Form.php:970 — orphaned comment for 'Visit the %s website…' that
was hard-coded English (never translatable!). Fixed by wrapping
in __() with a new VISIT_VENDOR_WEBSITE registry entry.
- Form.php:1432 — the EXTEND comment I introduced in this PR
while fixing bug #1. Repositioned inside the Strings::get() arg.
All comments now sit on the line immediately preceding the __()
literal, either inline (`/* translators: */ __(...)`) or on the
line above when the __() is on its own line.
# Coverage tests (from audit)
Added ~22 tests across three files closing the audit-identified
gaps:
tests/test-strings.php (+15 tests, +setUp/tearDown with reset()):
- test_init_called_twice_replaces_bound_config
- test_get_without_init_returns_default_no_filter
- test_reset_clears_all_static_state
- test_zero_arg_closure_override_invoked
- test_throwing_closure_propagates_exception (documents current contract)
- test_malformed_override_injected_post_validation_falls_back
- test_runtime_filter_receives_four_args
- test_filter_fires_on_override_path
- test_strings_config_non_array_is_unset
- test_escaped_percent_does_not_count_toward_placeholder_total
- test_format_flags_recognized_as_placeholders (6-case data provider)
- test_non_string_non_callable_overrides_dropped (7-case data provider)
- test_mixed_valid_and_invalid_overrides_partial_keep
- test_client_constructor_initializes_strings
tests/test-strings-translation.php (+5):
- test_repeated_load_translations_registers_change_locale_once
- test_second_load_translations_overrides_textdomain
- test_change_locale_callback_reads_latest_textdomain_after_second_load
(proves bug fix #3 — writes a temp .mo file to drive the closure)
- test_mo_path_resolves_relative_to_strings_file (Strauss safety)
- test_load_translations_with_non_string_argument_is_noop
tests/test-support-user-locale.php (+4):
- test_get_user_locale_does_not_leak_across_current_user_switches
- test_locale_format_check_does_not_consult_get_available_languages
(uses zz_ZZ to prove the SDK accepts unlisted locales)
- test_en_US_is_accepted_despite_being_absent_from_get_available_languages
- test_strings_overrides_and_support_user_locale_coexist
(cross-feature smoke test)
# Verification
PHPUnit: 209/209, 739 assertions (was 179/684 — +30 tests, +55 assertions).
PHPStan: clean under phpVersion=70400.
make-pot: every translator comment in src/ now lands in the .pot
as a `#.` extracted comment alongside its msgid.
PHP 8.0+ throws an uncaught ValueError when sprintf is called with
"Too few arguments" — a real fatal that would crash the consent
screen on a customer's site. Static-string overrides are already
placeholder-validated at Config-load time, but two paths bypass
that gate:
- Closure overrides return arbitrary strings at render time.
- The trustedlogin/{ns}/strings/{key} filter can mutate $value
into anything.
Either path could return a string with MORE placeholders than the
SDK call site supplies args for. PHP 8 then fatals.
Defense (Strings::get tail): count placeholders in the resolved
value via Strings::count_placeholders(). If it exceeds the
registry-declared count for this key, fall back to a fresh
translation of the $default — which the SDK author hand-authored
against the matching arg count, so it's guaranteed safe.
count_placeholders() handles:
- Simple %s / %d / %f
- Flag + width + precision modifiers (%05d, %.2f, %-10s)
- Positional placeholders (%1$s, %3$d) — counts by max index so
reuse of the same slot doesn't double-count, and a gapped
%1$s..%3$s still reports 3 args needed
- Escaped %% — stripped before counting
- Non-string inputs → 0
Fewer-placeholders is intentionally NOT rejected: closures often
return a fully-formatted string (no placeholders left) for a
placeholder-having key, expecting the caller's sprintf to silently
ignore the extra args. That's standard PHP behavior and explicitly
allowed.
# Tests (+5)
- test_closure_returning_too_many_placeholders_falls_back_to_default
- test_filter_introducing_too_many_placeholders_falls_back_to_default
- test_closure_returning_fewer_placeholders_is_allowed (common case
of fully-formatted closure return for a 2-placeholder key)
- test_filter_calling_sprintf_inline_is_allowed
- test_count_placeholders_handles_format_flags_and_positionals
(12-case sanity sweep for the counter)
PHPUnit: 214/214, 755 assertions (was 209/739 — +5 / +16).
PHPStan: clean.
Two integrator-code paths run inside Strings::get() / resolve()
and could throw — taking the SDK with them:
1. Closure overrides invoked via call_user_func_array().
Common failure modes: undefined variable in the integrator's
callable, null pointer ($x->method() when $x is null), DB
timeout, type error in a strict-typed callback.
2. apply_filters() callbacks for
trustedlogin/{ns}/strings/{key}. WP itself does not catch
filter exceptions.
Either propagated as an uncaught fatal would crash the customer's
consent screen — the integrator's bug becomes the customer's
outage. SDK now wraps both call points in `try { ... } catch (
\Throwable $e )` and falls back to:
- Closure that throws → the translated SDK default.
- Filter that throws → the pre-filter resolved value (so the
override is still respected, just without the filter mutation).
- translate() that throws (malformed .mo) → the raw English
default with no further processing.
The catch covers `\Throwable` (PHP 7+ base of both \Exception and
\Error), so type errors and "call to undefined method" don't slip
through under \Exception-only catching.
When WP_DEBUG is on, failures are surfaced via error_log() with
the failing Strings constant + exception message — enough signal
for the integrator to diagnose without forcing them to wire a
logger.
# Tests
- test_throwing_closure_falls_back_to_default
(replaces the prior test_throwing_closure_propagates_exception
which documented the OPPOSITE behavior — that contract is
inverted by this change. Inverting a test rather than deleting
it makes the decision change visible in git blame.)
- test_throwing_filter_does_not_fatal_keeps_resolved_value
- test_closure_that_throws_error_not_exception_also_falls_back
(verifies the \Throwable catch covers \Error, not just \Exception)
PHPUnit: 216/216, 757 assertions (was 214/755 — +2 / +2).
PHPStan: clean.
# Wire failure logging through the SDK's own Logging class
Strings::log_closure_failure was using error_log() with a WP_DEBUG
gate — inconsistent with the rest of the SDK and harder for
integrators to route. Replaced with the SDK's own Logging class:
- Lands in the same trustedlogin-logs/ file as every other SDK
log line.
- Respects the integrator's `logging/enabled` Config gate.
- Fires `trustedlogin/{namespace}/logging/log_error` so
integrators can hook to forward to Sentry, CloudWatch, etc.
Logging::__construct is three property assignments; the expensive
setup_klogger() lazy-fires on first write. Building one per failure
is fine for an error-path that rarely fires.
# Trim meta narrative
Following the codebase's commenting policy: keep comments that
describe hidden constraints / subtle invariants / surprising
behavior. Strip the "Defense:", "Belt-and-suspenders:",
"Per-class-plumbing", and "This was added because..." narrative
that describes the developer's reasoning rather than the code's
behavior.
Where:
- src/Strings.php: get(), resolve(), load_translations(),
init(), reset(), log_closure_failure() — kept method
descriptions; trimmed narrative.
- src/Config.php: validate_strings(), placeholders_safe() —
same treatment.
- src/SupportUser.php: locale-related comments trimmed to
behavioral signal only.
- src/Client.php: drop the "Bind the Strings utility AFTER
validate()" rationale comment.
- tests/*.php: drop the wide `// ====` / `// ----` section
banners between groups of tests.
# Test
- test_throwing_closure_logs_through_sdk_logging verifies
log_closure_failure routes through Logging by hooking the
SDK's logging/log_error action and asserting the message
arrived.
PHPUnit: 217/217, 760 assertions.
PHPStan: clean.
- Strings::get() / resolve(): rename $default → $default_text (reserved keyword warning); add phpcs:ignore for intentional runtime translate() with variable textdomain. - Strings: add short docblock descriptions, parameter comments, reformat constants registry (one entry per line, aligned). - Config::placeholders_safe(): ignore set_error_handler dev-fn warning and add $template parameter comment. - Anonymous functions: drop `static` modifier (PHP 5.4+); deferred-init callbacks for change_locale already use named static callbacks. - AGENTS.md: add Comment Discipline section documenting PHP 5.3 runtime target and rules against meta-narrative / task-reference comments. - Drop test_closure_that_throws_error_not_exception_also_falls_back — catching \Error is PHP 7+ and the SDK targets 5.3. PHPCS, PHPStan, PHPUnit (216 tests / 759 assertions) all clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds two long-standing i18n capabilities to the SDK:
stringsto configuration #66) — a newStringsclass exposes every user-facing SDK string as a stable constant. Integrators can override any string verbatim, with an empty value, or with a closure (for context-dependent variants) via thestringsConfig key. Strauss-safe: each prefixed SDK image gets its own filter namespace, so overrides never collide across plugins that ship the SDK.support_user/localeconfig +trustedlogin/{ns}/support_user/localefilter. Locale is written throughwp_insert_user'slocalearg (WP 4.7+) and defensively re-asserted viaupdate_user_metato survivewp_insert_userignoring the arg in some scenarios.The 117 user-facing strings in
src/Form.php,src/Remote.php,src/SupportUser.php,src/Admin.php, etc. were migrated to route throughStrings::get(Strings::KEY, __('English', 'trustedlogin')). The literal__()is the anchorwp i18n make-potextracts;Strings::get()then re-translates against whichever textdomain the integrator routed viaStrings::load_translations().Why this shape
Strings::get()is static — no per-class plumbing. Call sites read likeStrings::get(Strings::SECURED_BY_TRUSTEDLOGIN, __( 'Secured by TrustedLogin', 'trustedlogin' )).try { … } catch ( \Exception $e )around every integrator callback + filter, routed through the SDK's ownLoggingsurface. PHP 5.3 target rules out\Throwable, so\Errorfrom integrator code is not caught (accepted trade-off).sprintf"too few args" guard —Strings::count_placeholders()checks the resolved string against the registry's declared placeholder budget. If a closure/filter inflates the placeholder count, we fall back to the SDK default rather than letting aValueErrorreach the caller'ssprintf.Config::validate_strings()uses a sentinel-basedvsprintftest (placeholders_safe()) to reject overrides whose placeholder layout doesn't match the SDK default — so an override of'Extend %1$s Access for %2$s'with'Extend %s Access'is rejected at config time, not at render time.Test coverage
tests/test-strings.php— Strings API (init/reset/registry/known_keys/get/resolve/count_placeholders), every override shape (null / empty / string / closure),placeholders_safe()edge cases.tests/test-strings-translation.php— translation routing against the integrator's textdomain, switch-locale behaviour, pre-init deferral.tests/test-support-user-locale.php— explicit locale, filter, fallback to site locale, defensive re-assert.tests/test-translator-comments-extracted.php— meta-test that runswp i18n make-potand verifies every// translators:comment insrc/reaches the resulting POT. Catches the WP-CLI extractor's proximity rule at CI time.Quality gates
.phpcs.xml.dist,testVersion: 5.3-) — cleanphpVersion: 70400) — cleanNotes for review
AGENTS.mdhas a new Comment Discipline section documenting the PHP 5.3 runtime target and the rule against meta-narrative / task-reference comments.tests/test-translator-comments-extracted.phpis a meta-test, not a unit test — it shells out towp i18n make-potand runs only in thetests-clicontainer. It should be kept in CI.Closes #66
Closes #140
Test plan
composer lint)composer phpstan)npx wp-env run tests-cli --env-cwd=wp-content/plugins/client composer test)Strings::SECURED_BY_TRUSTEDLOGIN) renders verbatimConfig::validate_strings()at constructionsupport_user/localeset tode_DEproduces a support user whose admin UI is Germanchange_localeon a running site reloads the SDK's.mocorrectly