Framework/post audit hardening#4
Merged
Merged
Conversation
…dd ObjectField module; group WC SettingsPage
Pre-Phase-4 hardening. settings shipped WordPressObjectFieldStore — a
WooCommerce-order implementation (OrderUtil, shop_order, wc_get_order,
woocommerce_process_shop_order_meta) — inside a package documented and
deptrac-scoped as WooCommerce-free. deptrac could not catch it: WC symbols
are external, so no framework-layer collector sees them, and the package
would fatal on load when scoped without WooCommerce.
Move the implementation to woocommerce as OrderData/OrderFieldStore (a
rename that also corrects the misleading "WordPress" name on an order
store), and keep only the generic, WP/WC-agnostic contract in settings,
now under an ObjectField/ concept module:
settings/src/ObjectField/{ObjectFieldStoreInterface, ValueObjects/
ObjectMetaBox, Exceptions/InvalidObjectMetaBoxException}. settings drops
its WooCommerce composer "suggest" and php-scoper stub, so it is
WooCommerce-free in code and tooling. The contract stays a single-
implementation interface by design (the impl must live in woocommerce); a
docblock note records why it must not be removed on an impl count.
Also group the WC settings-page trio (WooCommerceSettingsBackend,
WCSettingsBuilder, DescriptorBackedWCSettingsPage) into a SettingsPage/
concept folder, so woocommerce/src reads as peer concept folders
(SettingsPage/, ProductData/, OrderData/).
deptrac unchanged and 0 violations; the existing ruleset already encodes
the correct architecture. PHPStan + Unit (290) green for the changed
packages; integration suite to be run under wp-env.
Assisted-by: Claude Code:claude-opus-4-8
…atorTest PHPStan flagged assertTrue( defined( '…\Requirements\FRAMEWORK_MIN_PHP' ) ) as staticMethod.alreadyNarrowedType: the namespaced const is resolved statically, so the assertion is always true. The Requirements concern file's wiring is already covered by the same-file check_requirements function_exists assertion, so the const check was redundant. Removing it restores a PHPStan-green tree and keeps exactly one wiring assertion per concern file (Environment / Plugin / Requirements / Notice). Assisted-by: Claude Code:claude-opus-4-8
…ce placement + VO/descriptor) Pre-Phase-4 hardening — codify two grammar rules that replace improvised conventions, restoring a predictable structure without reintroducing inheritance. Interface placement: an interface lives at its concept-folder root; a single-concept package keeps its contract flat at the package src root (the locked PluginInterface/PluginKernel pair is the one flat exception); no generic Contracts/ subfolder. Applied by removing the lone outlier — Hooks/Contracts/HookHandlerInterface moves to Hooks/HookHandlerInterface (the only Contracts/ folder in the tree). Value object vs descriptor: ValueObjects/ folders hold two carrier kinds, distinguished by base class + docblock, not a marker interface. A value object extends AbstractValueObject only when structural equals()/JSON is consumed (PluginHeader, Version; docblock opens "Value object ..."); a descriptor is a bare final readonly carrier, required when it holds closures/providers/WP objects (SettingsField, SettingsPage, SettingsSection, ObjectMetaBox, AdminNotice, DependencyRequirement; docblock opens "Descriptor for ..."). A marker DescriptorInterface is deferred until a consumer needs it. Both rules recorded as decision blocks in AGENTS.md. settings is multi-concept but under-foldered; its concept-folder reorganization is a separate follow-up. Assisted-by: Claude Code:claude-opus-4-8
…sonSerialize() equals() recursed only direct ValueObjectInterface properties and fell back to strict !== for everything else, so an array of value objects was compared by identity -- while jsonSerialize() (via convert_to_primitives) recurses arrays element-by-element. Two value objects holding a structurally-equal list<VO> of distinct instances therefore reported equals() === false yet serialized identically. Compare the canonical primitive projections instead -- the same convert_to_primitives() traversal jsonSerialize() uses -- so equality and serialization share one source of truth and can no longer drift. This also fixes DateTimeInterface equality (equal-but-distinct instances now compare equal) and drops two property.dynamicName PHPStan ignores plus the now-unused get_public_property_names import. Adds an Integration regression: a value object holding list<VO> asserts equals() agrees with jsonSerialize() across distinct-but-equal instances. Assisted-by: Claude Code:claude-opus-4-8
…ckend/ concept folders; align woocommerce Backend/
Pre-Phase-4 hardening — resolve settings' under-fragmentation (the
flattest yet most multi-concept package) by folding its flat files into
three peer concept folders, per the codified component grammar:
Schema/ shared declarative field model + render/process engine
(FieldType, FieldRenderer, FieldProcessor, OptionsResolver,
SettingsFieldAggregator, provider interfaces, the
SettingsField/Page/Section descriptors, field exceptions)
Backend/ WP options-page surface (SettingsBackendInterface +
WordPressSettingsBackend)
ObjectField/ unchanged
Schema/ is the cross-cutting core consumed by every surface (the WP
backend, ObjectField, and the woocommerce package), and is the clean
lift-out point should the field core ever be extracted to its own package.
Also rename woocommerce/SettingsPage/ -> woocommerce/Backend/ so the
settings-page backend surface carries one name in both packages
(settings/Backend + woocommerce/Backend), beside woocommerce's
ProductData/ + OrderData/.
Pure moves + namespace/reference rewrites; no behavior change. deptrac
unchanged (0 violations). PHPStan clean, Unit 290, Integration 274.
Assisted-by: Claude Code:claude-opus-4-8
…nto Schema free functions Pre-Phase-4 hardening — collapse copy-pasted field logic across the settings/woocommerce tier into namespaced free functions (the framework's convention, not static utility classes): Settings\Schema\filter_field_attributes() (#6, #16) Settings\Schema\is_valid_identifier() (#20) Settings\Schema\is_checkbox_checked() (#6, canonical) Settings\Schema\is_field_editable_by_current_user(SettingsField) (#6) WooCommerce\to_yes_no() (#6, Schema wrapper) #6 — the field-capability gate (copy-pasted across ~6 sites) and the attribute security filter + checkbox yes/no coercion (copy-pasted 3x with divergent rules) now have one owner each. The capability gate is a free function, not a method, to keep current_user_can() off the readonly SettingsField descriptor. Fixes a live checkbox bug: WCSettingsBuilder's (bool) cast (and FieldRenderer's checked state) mapped 'no' and arbitrary non-empty strings to checked/'yes'; the canonical rule (true/1/'1'/'yes' => checked) maps them to 'no'. The two ProductData sites already matched the canonical rule. #20 — id-charset validation now also covers SettingsSection id and SettingsPage slug (both feed wp_options keys), throwing new typed InvalidSettingsSectionException / InvalidSettingsPageException. #16 — the attribute filter keeps its strict on*-prefix block (a security control; narrowing to a handler denylist would risk under-blocking). Populating settings + woocommerce functions.php also clears the empty- aggregator finding (#9) for those two packages. deptrac unchanged. PHPStan clean, Unit 377, Integration 277. Assisted-by: Claude Code:claude-opus-4-8
…render/process layer Finding #14. The descriptor model was extensible on the WC product-data tab (a type-to-closure render registry) but the core Schema layer threw UnknownFieldTypeException on any type outside the closed FieldType enum — so a wave-2 consumer needing a bespoke control (QR's single_select_page) on a WordPress settings page would have to fork FieldRenderer/FieldProcessor. Inject a CustomFieldType registry — a final readonly descriptor carrying a type token and a string-returning render closure — into FieldRenderer and FieldProcessor, parallel to the existing OptionsResolver injection. Dispatch is enum-first -> registry-second -> throw-third: the enum stays the canonical taxonomy, the registry is the documented extension point, and the processor treats it as an allowlist so an unregistered type still throws (a typo stays loud). The seam is render-only; a custom type saves through the field's own sanitize/validate, matching the ProductData prior art (which has no sanitizer registry), falling back to the field default on absent or rejected input. Both WordPressSettingsBackend and OrderFieldStore already accept injected primitives, so the seam reaches every settings surface — options pages and order/object meta boxes — with no per-surface change. Drops the audit's custom_sanitizers half: both independent proposers verified the prior art saves through the field's own closures, so a parallel sanitizer registry would be a redundant second channel. Gates: PHPStan clean, deptrac 0, Unit 397, Integration 284. Assisted-by: Claude Code:claude-opus-4-8
Finding #13. Two demand-certain wave-2 consumers (QR, PPW) duplicate the same
Action Scheduler idiom — schedule a recurring action only if not already
scheduled. Rather than a thin AS wrapper bundled in the woocommerce package
(Action Scheduler is not WooCommerce-specific, only bundled with it), add a
backend-abstracted scheduler to utilities, shaped like the settings backend
pattern.
Scheduling/SchedulerBackendInterface is a generic hook+args contract with a
hybrid return surface: mutations (schedule_recurring/schedule_single/unschedule)
return a Result carrying a SchedulingError on failure; queries (is_scheduled,
get_next_scheduled) return plain scalars. Two final backends implement it —
ActionSchedulerBackend (as_* functions, function_exists-gated) and WPCronBackend
(the always-available fallback) — and select_scheduler_backend() picks Action
Scheduler when loaded, else WP-Cron. The Scheduler facade delegates to the
selected backend.
Backend specifics: AS idempotency uses the args-aware as_has_scheduled_action()
guard with unique=false, because AS's native unique flag keys on hook+group only
and ignores args. WP-Cron addresses a recurring event by a named schedule, not a
raw interval, so each interval N gets a synthetic dws_every_{N}s schedule
injected once through a stable cron_schedules filter callback; WP-Cron has no
grouping, so a non-empty group is rejected on every mutation. SchedulingError is
the framework's first concrete Shared\Error\ErrorInterface, pairing a
SchedulingErrorReason enum with a message and context.
Stub wiring: phpstan.dist.neon bootstraps woocommerce-packages-stubs.php (it
defines the as_* functions; woocommerce-stubs.php does not). utilities'
scoping-stubs declares the explicit-file form
"php-stubs/woocommerce-stubs:woocommerce-packages-stubs.php" so a consumer's
php-scoper leaves the as_* symbols external. That form depends on the
wordpress-configs CollectScopingStubs explicit-file support (separate repo); the
current collector silently skips the entry until then, harmless pre-Phase-4 as
no consumer scopes utilities yet. No deptrac change — as_*/wp_* are external
globals and the Utilities to Shared edge already exists.
Gates: PHPStan clean, deptrac 0, Unit 416, Integration 300.
Assisted-by: Claude Code:claude-opus-4-8
…m + scheduler A cold Codex review of this session's #14 (custom field-type seam) and #13 (scheduler) surfaced three real issues. (Two louder findings were verified non-issues: WP-Cron does NOT lose its synthetic schedule — wp_reschedule_event falls back to the interval stored in the event — and the scoping-stubs colon form is the already-flagged cross-repo dependency.) - FieldProcessor::process_custom coerces a non-scalar custom submission to the field default before the sanitizer runs, mirroring the built-in scalar guard. A tampered field[]=x to a scalar custom field with e.g. trim no longer fatals with a TypeError on save. - WPCronBackend::unschedule maps wp_clear_scheduled_hook's return instead of always returning Success: a WP_Error becomes Failure(ScheduleFailed) with the message preserved in context; an int count (including zero) stays Success — parity with the Action Scheduler backend's fail-loud mapping. - WPCronBackend::is_scheduled / get_next_scheduled return false / null for a non-empty group. A grouped schedule can never exist on WP-Cron (mutations already reject one), so the queries are consistent rather than group-blind. Gates: PHPStan clean, deptrac 0, Unit 417, Integration 303. Assisted-by: Claude Code:claude-opus-4-8
…#9/#10/#15) A batch of low-risk audit findings, each verified against current source. Doc-rot (#22): strike the phantom settings "dispatcher" (the component is the SettingsFieldAggregator; no dispatcher exists); reconcile core's boot to "two-pass" in composer.json (README/CHANGELOG already said two-pass — boot() makes two component passes, initialize then register_hooks); trim SettingsBackendInterface::get()'s docblock (the field default is render-time, not a read-time fallback) and KeyValueStoreInterface's "compose stores by ID" over-promise. Consistency nicks (#23): Version constructor protected to private (final, factory-only, matching Success/Failure); AdminNotice::$dismissible to $is_dismissible (aligns with is_persistent; the WP wp_admin_notice() 'dismissible' arg key and the is-dismissible class are preserved); move NoticeType to the AdminNotices concept root (matching FieldType at Schema/ root); standardize the per-package functions.php aggregator header; add is_wp_compatible()'s trailing .0-strip from a 3-part minimum so it agrees with WPVersionConditional (with a unit test). Empty aggregators (#9): drop the placeholder functions.php (and its composer files autoload entry + phpstan path) for core and storage; re-add when a namespace function lands. Contract clarifications: document get_plugin_header() as a consumer-facing accessor the kernel does not consult (#10, kept by decision); document that KeyValueStore callers supply valid keys and stores pass them through unsanitized (#15). Excluded by standing decisions: deptrac.yaml untouched (#25 — the unused WooCommerce-to-Core allow is harmless); the FieldRenderer on*-attribute filter stays strict (#16); OptionsStore memoization skipped (#11 — final readonly, and WordPress already caches options in-request). Gates: PHPStan clean, deptrac 0, Unit 418, Integration 303. Assisted-by: Claude Code:claude-opus-4-8
The fixture's "*" stub constraints let wordpress-stubs float to 7.0, which forced woocommerce-stubs down to v3.8.0 — a version without woocommerce-packages-stubs.php. So the utilities scoping-stubs explicit-file entry (php-stubs/woocommerce-stubs:woocommerce-packages-stubs.php) was silently skipped and the as_* exclusions came incidentally from the old bundled stub, not the path under test. Pin woocommerce-stubs to ^10.8 to match the framework, so it resolves to 10.8.0 (which ships the packages-stubs file) and the explicit-file path is exercised faithfully. Verified after a clean reinstall (which also refreshes the path-repo mirrors that composer update leaves stale): smoke passes, the collector no longer skips woocommerce-packages-stubs.php, and 11 as_* functions land in scoping-exclusions.json via the explicit-file path. Assisted-by: Claude Code:claude-opus-4-8
…ctor Pull the merged wordpress-configs trunk (9764855 -> 4431b7b) so the framework vendors the explicit-file scoping-stubs collector (the package:file form + realpath containment) and the doc cleanup. Lock-only: the shared QA configs the framework's phpstan.dist.neon includes are unchanged, so the gates are unaffected. The framework does not run the collector itself — this keeps the vendored copy current for the consumer-smoke fixture and anyone working in the repo. Assisted-by: Claude Code:claude-opus-4-8
…ring/ Finding #7. Lifecycle/ conflated two kinds of marker: kernel-dispatched ones the PluginKernel calls (Hookable, Initializable) and self-dispatched ones invoked from the component's own WP callback, not by the kernel (Outputtable, Renderable). A reader seeing Lifecycle/Renderable wrongly assumes the kernel drives it. Move Outputtable + Renderable (with their dormant Exceptions/) out to a new Rendering/ peer concept, keeping the per-marker sub-folder structure that mirrors Lifecycle/ (consistency over minimizing folders). Lifecycle/ now holds only the kernel-dispatched Hookable + Initializable, where the name is accurate. Interface and exception names are unchanged; the two interfaces' cross-references retarget to the new namespace. Pure namespace move (git mv, history preserved): no kernel, deptrac, composer, or phpstan edits — PSR-4 maps the whole Core namespace, deptrac is a namespace-prefix regex, phpstan analyses by directory. The never-thrown exceptions move as-is; dropping them is a separate follow-up that would also cover InitializationException. Gates: PHPStan clean, deptrac 0, Unit 418, Integration 303. Assisted-by: Claude Code:claude-opus-4-8
set()/remember() defaulted expiration to 0 ("never expires"). WordPress
stores a no-expiry transient autoloaded and never garbage-collects it
(delete_expired_transients reclaims only rows whose _transient_timeout_
row has lapsed). Because flush() only bumps the generation suffix, a
0-expiry row stranded by a flush persisted forever — and autoloaded on
every request.
Make expiration a required positive argument: a non-positive value is
rejected via _doing_it_wrong() and the write skipped, mirroring the
over-long-key guard. Every stored row now carries a timeout and is
reclaimed by WordPress' daily cleanup, including a generation left
unreachable by a flush. flush() stays the O(1) suffix bump; the PAYLOAD
envelope and length guard are unchanged.
Assisted-by: Claude Code:claude-opus-4-8
…base (audit #3) InvalidValueObjectException was an abstract base with zero production subclasses — even Version's invalidity exception extended the generic AbstractInvalidArgumentException directly, leaving the base unused. Adopt it as the value-object-exception family base instead of deleting it: InvalidVersionException now extends it, supplying value_object_type => 'Version', so VO-invalidity exceptions share one message format and a single catch (InvalidValueObjectException) point. Reword Version's two throw reasons to read cleanly after the base's message prefix. Document the convention and sanction the abstract in AGENTS.md — a family of distinct catchable subtypes sharing message-building intrinsically needs the subtype hierarchy; descriptor-invalidity exceptions still extend the generic bases directly. Assisted-by: Claude Code:claude-opus-4-8
#2 — DeprecatedHooksDispatcher is kept as a recorded speculative exception (zero consumers, no v1 precedent, closed ecosystem), so the build-ahead boundary stays principled rather than arbitrary. #24 — Error/ErrorInterface is kept distinct, not folded into Result/: it is load-bearing via SchedulingError, the TError bound across AbstractResult / Failure, and the counterpart to ExceptionInterface in the error-vs-exception split. Folding would couple a backend-agnostic error marker to the Result carrier and collapse that boundary. Assisted-by: Claude Code:claude-opus-4-8
The standard prefers protected over private — on final classes the two are behaviorally identical, and protected forward-designs against a later un-final (no need to remember to widen visibility, and there is no perf cost). The src had drifted to private throughout; flip all 160 private member declarations across packages/*/src to protected. Visibility-only: every changed line is exactly private -> protected, with no logic/name/signature/docblock change (verified line-by-line). The two private(set) asymmetric-visibility write-guards (HooksService::$handlers, AdminNoticesService::$stores) are deliberate and left as-is. Assisted-by: Claude Code:claude-opus-4-8
…cted(set) (audit #21) Completes the protected-over-private sweep: HooksService::$handlers and AdminNoticesService::$stores used PHP 8.4 asymmetric visibility with a private setter. Widen the set-visibility to protected so a future un-final subclass can mutate them too, matching the protected-default standard. Read visibility (public) is unchanged, and behavior is identical on these final classes. Assisted-by: Claude Code:claude-opus-4-8
…regators Revisits three cleanups from this branch the owner wanted partially undone: - Restore the file-level docblock (description + @since/@Version) on every package-root functions.php aggregator, replacing the terse one-line comment. The @since/@Version pair traces when a file changed; applied uniformly across all seven packages so the set stays consistent. - Recreate the empty core/ + storage/ aggregator placeholders and their autoload.files entries (composer.json + composer.lock snapshots) for parity with the packages that carry namespace functions. - Drop the bootstrap AggregatorTest: all four concern functions have dedicated Integration coverage (CheckRequirements/GetPluginMetadata/OutputRequirementsError/ IsCompatible), so the unit-level function_exists smoke was redundant. Assisted-by: Claude Code:claude-opus-4-8
The strict PHPCS gate was red across the branch (and trunk). Resolved every finding so the phpcs job — which exits non-zero on warnings too — passes: Code fixes: - Wrap the _doing_it_wrong() messages in TransientCache + AdminNoticesService in esc_html() (WordPress.Security.EscapeOutput), keeping the sniff active. - Flip a non-Yoda comparison in AdminNoticesService. - Rename the reserved-keyword params: SettingsField::$default -> $default_value (incl. every reader + named-arg call site), the WCSettingsBuilder local, and the $object params -> $wc_object across the order/product stores. False positives suppressed per-line, with justification: - Inline /** @var */ PHPStan hints flagged for a missing short description. - A PHP 8.4 property hook misread as removed curly-brace array access. - A core WooCommerce capability (edit_shop_orders) WPCS does not know. - A dynamically-registered cron interval WPCS cannot read statically. Also re-adds the core/storage functions.php paths to phpstan.dist.neon and trims its rot-prone stub comment. Companion: the generic-type docblock allowance (class-string<X>/list<X>) lands in the shared wordpress-configs ruleset, not here. Assisted-by: Claude Code:claude-opus-4-8
ahegyes
added a commit
that referenced
this pull request
Jul 4, 2026
…ter across five packages Kernel: the whole component phase runs inside a hook-table transaction — a copy-on-write snapshot taken before Feature resolution, and on any failure the table is diff-unwound through add_filter()/remove_filter() only, restoring window-start registrations and logging any residue that direct hook-table manipulation leaves. Stored-version downgrades fail closed without running update(). boot() exposes a diagnostic report (gated features, pruned/inert/initialized/hooked components, status, failure summary). Kernel diagnostics are best-effort: a throwing logger never alters the boot outcome, and a caught boot Throwable's class and message reach the log record. AdminNotices: the AJAX dismissal endpoint records only queued persistent dismissible notices the current user may see. CompositeLogger fans one record out to every sink, delivering to all before propagating the first failure, so failure visibility no longer depends on the logger choice. Settings: custom-location pages render Settings API notices once at page top, filtered to WordPress' shared save-confirmation bucket plus the page's own section buckets; checkbox submissions normalize to canonical yes/no; TermFieldStore covers the add-new-term surface under the edit_terms gate; WooCommerce settings sub-tabs are stable under sanitize_title() and reject colliding normalized section ids; WordPress-global name prefixes are validated at construction. Shared, bootstrap, storage, woocommerce: nested JsonSerializable values reduce recursively with a cycle guard; the requirements notice renders in the network admin (all_admin_notices); product-data default injection is $single-aware and scans for missing owned keys before the consumer gate; UserMetaStore documents the grouped read-modify-write limitation. Remediates 2026-07-03 audit findings #4 #8 #10 #11 #18 #26 #27(doc) #28 #31 #32 #34 #38 #40(doc) #84 #101 #108 and pulled-in FW-14, FW-32. Assisted-by: Claude Code:claude-fable-5
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
Affected packages
wp-framework-bootstrapwp-framework-sharedwp-framework-storagewp-framework-corewp-framework-utilitieswp-framework-settingswp-framework-woocommerceChecklist
composer changelog:add:<package>(one per affected package)composer quality-checkpasses locally (PHPCS + PHPStan + Deptrac + unit tests)README.mdmajor) are explicitly justified belowBreaking changes / migration notes