feat(settings): add dokan()->settings read-only accessor (PR 1/N)#3209
Open
mrabbani wants to merge 16 commits into
Open
feat(settings): add dokan()->settings read-only accessor (PR 1/N)#3209mrabbani wants to merge 16 commits into
mrabbani wants to merge 16 commits into
Conversation
…istry cache in tests
mrabbani
changed the base branch from
develop
to
refactor/simplify-settings-to-flat-array
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced May 20, 2026
…ngs (PR 2/N) (#3210) * refactor(settings): migrate Fees.php to dokan()->settings->get() Two of three call sites migrated: - shipping_fee_recipient: same flat id, schema default 'seller' matches. - tax_fee_recipient → product_tax_fee_recipient: renamed in schema; defaults match. Skipped get_shipping_tax_fee_recipient() — its dokan_get_option() call uses a dynamic third-arg default that falls back to the product tax recipient when shipping_tax_fee_recipient isn't explicitly set. The schema can only express a static default, so this site stays on dokan_get_option() with an inline note until that backward-compat contract is revisited. * refactor(settings): migrate Rewrites.php to dokan()->settings->get() Five call sites migrated. Four legacy field names were renamed in the schema — the migration uses the new flat ids, not a textual rename: - custom_store_url → vendor_store_url (default 'store' matches) - store_listing → store_listing_page (no default either side) - dashboard → vendor_dashboard_page (call default 0; downstream already wraps in absint(), so empty-string fallback is equivalent) - store_products_per_page → store_product_per_page (default 12 matches; note the schema id drops the 'products' plural) Left untouched: - get_option( 'dokan_rewrite_rules_needs_flashing', ... ) — operational state flag, not a schema setting (spec §7.2 bucket C). * feat(settings): add has_stored() + finish Fees.php migration (PR 3/N) (#3211) * feat(settings): SettingsRegistry::is_stored() — distinguish stored from defaulted Adds a lazy id-keyed set of "keys present in storage" alongside the existing field index. The set is built by applying the same bridge overlay populate_values() uses, so a value present only in a mapped legacy per-section option still counts as stored. Invalidated alongside the other caches in clear_cache(). * feat(settings): expose has_stored() on the accessor Delegates to SettingsRegistry::is_stored(). Distinguishes "user set this" from "schema default applies" — needed to preserve runtime fallback contracts (e.g. fall back to product_tax_fee_recipient when shipping_tax_fee_recipient is not explicitly stored). * test(settings): cover SettingsAccessor::has_stored() Four tests: unregistered key, registered-but-unset (returns false even though has() is true), value in the new flat option, and value only in the mapped legacy per-section option (bridge overlay). * refactor(settings): finish Fees.php migration using has_stored() The third site in get_shipping_tax_fee_recipient() was previously skipped because its third arg was a dynamic default that the schema can't express. has_stored() preserves the same backward-compat contract: fall back to the product tax recipient when shipping_tax_fee_recipient isn't explicitly user-set. * docs(settings): document has_stored() * refactor(settings): migrate functions.php to dokan()->settings (PR 4/N) (#3212) * refactor(settings): migrate 10 functions.php sites to dokan()->settings->get() Migrated sites (legacy → flat id): - L162, L1152 dokan_is_store_page / dokan_url_replace_uri_token: custom_store_url → vendor_store_url - L192 dokan_get_navigation_url helper (apply_filters wrap): dashboard → vendor_dashboard_page - L1870 dokan_admin_menu_capability: admin_access → admin_access (same id, default 'on' matches) - L2047 dokan_get_navigation_url: dashboard → vendor_dashboard_page (preserved (int) cast) - L2593 redirect_to filter: dashboard → vendor_dashboard_page (preserved (int) cast) - L3245 dokan_is_store_listing: store_listing → store_listing_page - L3295 dokan_replace_policy_page_link_placeholders: privacy_page → privacy_policy_page - L3317 dokan_privacy_policy_text ($privacy_page lookup): privacy_page → privacy_policy_page - L3436 dokan_get_terms_condition_url: reg_tc_page → reg_tc_page (same id) Sites intentionally NOT migrated — defaults / schema gaps that would silently change behavior. Each needs a schema audit or product decision before it can move to dokan()->settings: Not in schema at all (no legacy_key mapping): - L540 product_status - L2569 seller_enable_terms_and_conditions - L3946 withdraw_date_limit - L4074 store_banner_width - L4090 store_banner_height - L4112 recaptcha_enable_status Registered but missing legacy_key (bridge can't overlay): - L4108 recaptcha_site_key - L4109 recaptcha_secret_key Default mismatch — schema default contradicts historical call-site default: - L3324 enable_privacy schema 'on' vs historical '' - L3326 privacy_policy schema (none) vs historical long localized text - L4355 one_step_product_create schema 'off' vs historical 'on' Transformer-mapped (spec §9 highest-stakes failure mode): - L3669 hide_vendor_info (HideVendorInfoTransformer) Dynamic key — accessor takes a literal id: - L967 dokan_get_option( $page, 'dokan_pages' ) * refactor(settings): rename schema field IDs to be self-explanatory Field IDs are storage keys — when a field moves between sections, pages, or subpages, the ID must not change or stored values get orphaned. Audited SettingsSchema for IDs that depended on their current location or that were so vague the meaning required reading surrounding context, and renamed them to standalone names. Misleading IDs corrected: store_opening_time → store_sidebar_from_theme store_clossing_time_widget → store_contact_form_enabled store_template (field) → store_header_template display_notice → reverse_withdrawal_grace_period_notice Vague IDs made self-describing: enable_selling → vendor_auto_enable_selling address_fields → vendor_registration_address_fields welcome_wizard → vendor_welcome_wizard_enabled product_popup → vendor_new_product_popup order_status_change → vendor_can_change_order_status recaptcha → google_recaptcha_enabled recaptcha_info → google_recaptcha_info recaptcha_site_key → google_recaptcha_site_key recaptcha_secret_key → google_recaptcha_secret_key fontawesome_loading → dokan_fontawesome_enabled store_time_widget → store_opening_closing_time_widget store_product_per_page → store_products_per_page vendor_info_visibility → store_page_vendor_info_visibility vendor_store_url → vendor_store_url_slug Cascade updates: - SettingsSchema dependency keys referencing renamed fields - dokan()->settings->get() callers in Rewrites.php and functions.php - register-fields.tsx field-id check for the vendor info preview - PHPUnit tests (SettingsAccessor, SettingsRegistry, SettingsSchema, LegacySettingsRepository, SettingsRoundTrip) - Playwright DOM selectors that embed the field id in element ids (adminSettingsPage page object + 6 spec files + testData) legacy_key bridges untouched — old wp_option storage still maps to the new IDs through the existing migration path.
Member
Author
|
@claude review the PRs and goals |
…ng (#3213) AdminSettingsServiceProvider registered SettingsAccessor::class via the auto-resolve path, but SettingsAccessor::__construct( SettingsRegistry $registry ) requires its dependency — League Container's reflection-based resolver cannot fulfill a non-nullable typed parameter without an explicit binding. Result: every code path that resolves dokan()->settings during plugin activation fataled with: ArgumentCountError: Too few arguments to function WeDevs\Dokan\Admin\Settings\SettingsAccessor::__construct(), 0 passed and exactly 1 expected This was previously masked because the only consumers of the binding (SettingsAccessor's own PHPUnit tests) instantiate the class manually. PR 2 (#3210) migrated Rewrites::__construct() to call dokan()->settings, which exercises the binding during plugin bootstrap and surfaces the fatal at install/activation time. Special-case SettingsAccessor in the register() loop and supply SettingsRegistry via ->addArgument(), matching the existing pattern in AdminDashboardServiceProvider for services with required constructor deps. The rest of the provider's services use ?Type = null parameters, which is why they auto-resolved without an explicit binding. Verified: `wp plugin activate woocommerce dokan-lite` now succeeds. PHPUnit bootstrap completes (previously crashed during WC_Install). Out-of-scope follow-ups surfaced by the now-bootable test suite: - SettingsAccessorTest::test_has_stored_returns_false_when_registered_but_unset fails because hydrate_new_from_legacy() always fills schema defaults for mapped keys, which is_stored() then treats as "stored". Semantic bug in is_stored, not this DI fix. - SettingsSchemaTest has three failures around unknown variants, modal copy, and reverse-withdrawal dependencies — all schema-content drift predating this branch.
) SettingsRegistry caches its processed schema (with overlay-applied values) on first read. Without invalidation listeners, the cache went stale the moment anything wrote to dokan_admin_settings or any of the legacy dokan_* sections within the same request — dokan()->settings->get() would silently return pre-write values. This was previously masked because dokan_get_option() reads through LegacySettingsRepository, which DOES wire its own per-section flush listeners. Once call sites migrate to dokan()->settings->get(), the overlay-aware registry becomes the read path and the missing invalidation surfaces as stale values. Wire option-change listeners in SettingsRegistry's constructor: - update_option_{dokan_admin_settings} / add_option_{dokan_admin_settings} for the canonical write surface. - update_option_{section} / add_option_{section} for every legacy section name the bridge knows about, so Pro and raw third-party update_option() writes also invalidate the overlay. Bridge enumeration is deferred to first construction and wrapped in a container-availability check so the registry stays instantiable pre-bootstrap (e.g. in unit tests with no container). Regression coverage in SettingsRegistryCacheInvalidationTest: - cache invalidates on new-flat-option update - cache invalidates on legacy section update (via bridge overlay) - cache invalidates on new-flat-option add (not just update)
…okan()->settings (PR 3/N) (#3215) * test(reverse-withdrawal): characterize SettingsHelper behavior before migration Pin the public-API behavior of ReverseWithdrawal\SettingsHelper so the upcoming dokan()->settings migration is provably equivalent at each public method. Tests are written against the current dokan_get_option() implementation and must remain green after the swap. One test (test_is_enabled_returns_true_when_legacy_on) is marked markTestSkipped() because the current call site reads field 'reverse_withdrawal_enabled' from the dokan_reverse_withdrawal option, but the legacy admin form saves to field 'enabled' — a regression introduced by 66303f3 when the field id was renamed without a translation shim. The Task 3 migration to dokan()->settings->get() fixes this by routing through SettingsRegistry + LegacySettingsBridge, which correctly overlays the legacy 'enabled' field onto the new flat id. The skip will be removed in the same Task 3 commit, turning this test into a regression test for that fix. * refactor(reverse-withdrawal): migrate 1:1 settings reads to dokan()->settings Six call sites migrated, each verified against the schema: - reverse_withdrawal_enabled (legacy field 'enabled' -> same flat id; default 'off' matches). This call site was previously broken: a recent rename (66303f3) had it reading dokan_get_option('reverse_withdrawal_enabled', ...) while the legacy form still saves to field 'enabled'. The bridge maps legacy 'enabled' -> flat 'reverse_withdrawal_enabled' correctly, so the migration to dokan()->settings->get() fixes the latent bug. The characterization test test_is_enabled_returns_true_when_legacy_on is unskipped in this commit and now passes. - billing_type -> reverse_withdrawal_billing_type (default 'by_amount' matches) - reverse_balance_threshold -> reverse_withdrawal_balance_threshold (call default '150' string, schema 150 int; wc_format_decimal() normalizes both) - monthly_billing_day -> reverse_withdrawal_monthly_billing_day (call '1' string, schema 1 int; absint() normalizes) - due_period -> reverse_withdrawal_due_period (call '7' string, schema 7 int; absint() normalizes) - display_notice -> reverse_withdrawal_grace_period_notice (default 'on' matches) None of these fields carry a legacy_transformer; value shape is unchanged across the swap. The magic-default 3rd argument is dropped on each site because the schema is now the authoritative default source (spec §7.1). Verified: SettingsHelperTest 13/13 passing. * refactor(reverse-withdrawal): migrate failed_actions reads + fix isset() check failed_actions -> reverse_withdrawal_failed_penalty_actions. Both call sites migrated, but is_failed_action_enabled() required a semantic rewrite, not just an identifier swap. The legacy field stored WP multicheck shape ['enable_catalog_mode' => 'enable_catalog_mode']; the new flat field uses MulticheckArrayTransformer which returns a list ['enable_catalog_mode']. The old isset(\$arr[\$action]) check only worked on the associative shape and would silently return false on the new list. Routed through get_failed_actions() with in_array(\$action, ..., true) which works on both shapes. get_failed_actions() also casts the registry-returned value to array defensively before array_filter() — the registry may return null when a schema element predates registration. Default drift to be aware of: historical call default was [] (empty); schema default is ['enable_catalog_mode']. For fresh installs with no stored value, get_failed_actions() now returns ['enable_catalog_mode'] where it previously returned []. This is the documented schema-default contract (spec §7.1) and matches what admins see in the settings UI on a fresh install. Verified: SettingsHelperTest 13/13 passing. * docs(reverse-withdrawal): flag two unmigrated call sites with TODO payment_gateways and send_announcement are intentionally left on dokan_get_option() in this PR. Their candidate flat ids (transactions_reverse_withdrawal_payment_gateways, transactions_reverse_withdrawal_send_announcement) are defined only in the generated CSV-fragment schema, which is feature-flag-gated behind dokan_csv_schema_enabled (default false). Until that flag flips on, the ids are not registered in the live SettingsRegistry, so a dokan()->settings->get() call would miss the schema, log an unknown-key notice, and return the (correct) fallback — but the indirection adds nothing and obscures the deferral. The TODO inline comments document the intent and the unblock condition so the next pass at this file can swap them in mechanically. * chore(test): use SettingsRepository::replace() instead of raw delete_option The PHPCS sniff added by 2db969c forbids raw delete_option() against the dokan_admin_settings key outside SettingsRepository. Switch the test fixture reset to SettingsRepository::replace([]), matching the pattern in SettingsAccessorTest. * refactor(settings): migrate payment_gateways + send_announcement to dokan()->settings Resolves the two TODO(settings-migration) deferrals in ReverseWithdrawal/SettingsHelper.php by adding the missing field definitions to the hand-authored SettingsSchema.php and swapping the call sites to dokan()->settings->get(). PR 3 now migrates all 10 of SettingsHelper's dokan_get_option() call sites — none remain. Schema additions in SettingsSchema.php follow the project's existing hand-authored naming (reverse_withdrawal_*), not the transactions_reverse_withdrawal_* convention used in the auto-generated csv_fields.php reference. The CSV fragment is a documentation/reference artifact used by schema-validation tests behind a feature flag — it does not flip on in production, so the canonical schema is what live code consumes. Two new fields: - reverse_withdrawal_send_announcement (switch) legacy_key: dokan_reverse_withdrawal.send_announcement default: 'off' - reverse_withdrawal_payment_gateways (multicheck) Options built dynamically from the existing dokan_reverse_withdrawal_payment_gateways filter so Pro modules that extend that filter continue to register gateways with one declaration — matches the precedent set by withdraw_order_status_field(). legacy_key: per-slot dotted-path entries under dokan_reverse_withdrawal.payment_gateways.{gateway} legacy_transformer: MulticheckArrayTransformer::for_slots so the new flat-list shape ['cod'] bridges back to the legacy WP-multicheck shape ['cod' => 'cod'] default: ['cod'] Internal shape change for get_enabled_payment_gateways(): Pre-migration it returned the legacy associative shape ['cod' => 'cod']; post-migration with the transformer it returns the flat list ['cod']. Only consumer in dokan-lite is is_gateway_enabled_for_reverse_withdrawal() which uses in_array() — works on either shape. Verified: - SettingsHelperTest 13/13 green - Broader settings suite: 119 tests pass, same 4 pre-existing failures unrelated to this change - PHPCS clean on both files - wp plugin activate woocommerce dokan-lite succeeds (no DI fatal) - No dokan_get_option() calls remain in SettingsHelper.php * revert(settings): leave send_announcement to Pro The send_announcement field is owned by Dokan Pro, not Lite — even though the SettingsHelper helper method send_balance_exceeded_announcement() ships in Lite. Drop the schema entry I incorrectly added to SettingsSchema.php and revert the call site to dokan_get_option() with an inline comment explaining the Pro ownership. The corresponding schema entry + migration belongs in a Pro commit, not here. payment_gateways stays migrated — that one is a genuine Lite field. PR 3/N now migrates 9 of 10 call sites in SettingsHelper.php. The one remaining dokan_get_option() call is documented in place. * fix(settings): defer legacy section hooks to init to avoid recursion PR #3214 wired legacy section invalidation listeners in SettingsRegistry's constructor. Enumerating the sections requires $bridge->get_mapping() which builds the bridge map, which harvests SettingsSchema::get_schema(), which runs get_posts() in general_page(). get_posts() fires pre_get_posts hooks → current_user_can() → map_meta_cap filter → in Pro installs, ProductSubscription::filter_capability calls dokan_is_seller_dashboard() which reads dokan()->settings — re-entering DI resolution for SettingsAccessor while we're still constructing its SettingsRegistry dependency. Result: fatal 500 on any admin page in Pro installs (Lite-only installs were unaffected because Pro's hook isn't registered). Reproduced live on a Pro install loading admin.php?page=dokan-dashboard&subpage_id=reverse_withdrawal — full stack trace in the PR description. Fix: keep the new-flat-option hooks in the constructor (no bridge lookup needed — option key is a constant) but defer legacy section hook registration to the `init` action. By init, bootstrap is stable and the bridge mapping can be safely enumerated without re-entering DI resolution. If init has already fired (e.g. inside a PHPUnit test where WP_UnitTestCase boots WP fully), the hooks register immediately. Verified: - SettingsRegistryCacheInvalidationTest 3/3 green (all paths still invalidate on the next read) - ReverseWithdrawal/SettingsHelperTest 13/13 green - PHPCS clean * chore(settings): set priority on reverse_withdrawal_enabled and payment_gateways User-requested ordering tweak for the Reverse Withdrawal settings section: - reverse_withdrawal_enabled: priority 50 (renders first within the section) - reverse_withdrawal_payment_gateways: priority 70 (renders after the enabled toggle, before the unspecified-priority fields that fall back to source order)
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
2 participants
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
Lands a schema-aware, read-only settings accessor exposed as
dokan()->settings->get( \$flat_id ).Purely additive — zero behavior change for end users.
dokan_get_option()is untouched, no call sites migrated, no deprecation notice yet. Existing storage layout unchanged.This is PR 1 of a multi-PR refactor. Per-module call-site migrations (
dokan_get_option→dokan()->settings->get) and the_deprecated_function()switch are deferred to subsequent PRs.What this adds
SettingsAccessorInterface— public contract withget( \$key, \$fallback = null ),has( \$key ),all().SettingsAccessor— thin final adapter overSettingsRegistry. Reads are schema-aware: schema defaults are authoritative; legacydokan_*per-section values are transparently overlaid via the existingLegacySettingsBridgepath inSettingsRegistry::populate_values().SettingsRegistry::find_field( \$id )— O(1) id-keyed lookup backed by a lazy\$field_indexcache, invalidated alongside\$cacheinclear_cache(). Structural schema nodes (pages, sections, tabs) are intentionally excluded.dokan()->settingsresolves via a closure inServiceProviderthat delegates to the sharedSettingsAccessor::classbinding inAdminSettingsServiceProvider. The interface name is registered as a tag, sodokan()->settingsis the canonical access path.SettingsAccessorTest), 4 integration (SettingsAccessorIntegrationTest, including a real bridge-overlay test againstdokan_general), 4 registry (SettingsRegistryFindFieldTest).docs/settings/accessing.md.Design references
docs/superpowers/specs/2026-05-20-settings-accessor-design.md(not committed; gitignored underdocs/superpowers/)docs/superpowers/plans/2026-05-20-settings-accessor-pr1.md(same)Out of scope (follow-ups)
dokan_get_option()call-site migrations (~224 sites) and rawget_option('dokan_*')migrations (~79 sites, schema-mapped subset only).legacy_keymapping._deprecated_function()activation ondokan_get_option()in the final PR of the series.Test plan