fix(settings): stop reading Dokan settings before init - #3346
Conversation
|
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 Plus 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 |
|
Pro companion: getdokan/dokan-pro#5983 — same root cause, fixes the four Pro modules that read settings in their constructors. Merge this Lite PR first. |
Reading any Dokan setting now walks the legacy settings bridge, which builds the full admin settings schema — and that schema is translated. `FullWidthVendorLayout::register_hooks()` read `vendor_layout_style` on `plugins_loaded`, so `esc_html__()` ran before `init`, where `load_plugin_textdomain()` is registered. WordPress 6.7+ answers that with a `_load_textdomain_just_in_time` doing-it-wrong notice, logged twice on every request. Register the layout hooks unconditionally and move the gate into the callbacks via `is_latest_layout()`. All three run on `init` or later, and the repository memoizes the section snapshot, so the deferred read costs nothing. Also drop the eager schema build from `LegacySettingsRepository`'s constructor. It called `known_sections()` purely to learn which legacy `dokan_*` options to bind flush listeners to, which forced a full schema build on construction — on every front-end request that read a single option. Worse, that map was harvested before Pro registers its schema filters on `init`, so Pro-only sections never got listeners and their snapshots could go stale. Invalidation now listens on the generic `added_option` / `updated_option` / `deleted_option` hooks and decides membership against the snapshots actually held, which needs no schema and covers unmapped sections too.
814eed3 to
e098072
Compare
All Submissions:
Changes proposed in this Pull Request:
Every request on this branch logs the WordPress 6.7 doing-it-wrong notice twice:
A
doing_it_wrong_runbacktrace pins it to one chain:On this branch, reading any Dokan setting walks the legacy settings bridge, and the bridge harvests its legacy-key mapping out of
SettingsSchema::get_schema()— a ~2,800-line schema full ofesc_html__()calls.register_hooks()runs onplugins_loaded, butload_plugin_textdomain()is registered oninit(dokan-class.php:263), so the translation calls land too early.BridgeBootstrapalready defers its own harvest toinitpriority 999 for exactly this reason; these two call sites had not been given the same treatment.1.
FullWidthVendorLayout— the trigger.The three layout hooks are now registered unconditionally, and the
vendor_layout_stylecheck moved into the callbacks behind a newis_latest_layout()helper. All three fire oninitor later, so the read is safe there, and the repository memoizes the section snapshot so deferring it costs nothing. Externally observable behaviour is unchanged — a legacy-layout marketplace still gets no assets and no template override.2.
LegacySettingsRepository::__construct()— a second defect found on the way.The constructor called
known_sections()purely to learn which legacydokan_*options to bind cache-flush listeners to. That forced a full schema build during construction, on every front-end request that read a single option. It was also wrong: the map was harvested before Pro registers itsdokan_get_admin_settings_schemafilters oninit, so Pro-only sections never got listeners at all and their snapshots could serve stale values after a write.Invalidation now listens on WordPress' generic
added_option/updated_option/deleted_optionhooks and decides membership against the snapshots actually held. No schema needed, and unmapped sections are covered too.Fix 2 alone does not silence the notice — with the constructor lazy, the schema build simply moves down one frame into
all(). Both changes are required.Related Pull Request(s)
refactor/simplify-settings-to-flat-array) — base branch.fix(dashboard): stop building vendor dashboard layout config on background requests), which edits the sameregister_hooks()lines but targetsdevelop. That PR movesregister_vendor_dashboard_assetsfrominittowp_enqueue_scriptspriority 5 and keeps the pre-initdokan_get_option()read, so it does not address this notice. Whichever lands second will need a deliberate merge resolution — the two changes are compatible in intent (both want the work to happen later), they just touch the same block.Closes
debug.logon the branch.How to test the changes in this Pull Request:
WP_DEBUGandWP_DEBUG_LOGtotrue.wp-content/debug.loggains two_load_textdomain_just_in_timenotices for thedokan-litedomain per request.dokan-litenotices. (Any remaining_load_textdomain_just_in_timetraces come fromdokan-pro/includes/Module.php:169on thedokandomain — a separate issue in that repo, normally hidden because Pro suppresses its owndoing_it_wrong_trigger_erroroutput.)vendor_layout_style = latestthe React layout and its assets load; withlegacythe theme template is used and no dashboard assets are enqueued.Test coverage
Four new tests, each confirmed failing before the fix:
LegacySettingsRepositoryTest::test_constructor_does_not_build_the_settings_schemaLegacySettingsRepositoryTest::test_write_to_unmapped_section_flushes_its_snapshotFullWidthVendorLayoutTest::test_register_hooks_does_not_read_settingsFullWidthVendorLayoutTest::test_layout_setting_is_read_when_a_callback_runsFull PHPUnit run: 440 tests, 7 failures — the identical set before and after this change (Commission ×4,
OrderStatusChange×2,WithdrawChargeTransformer×1), verified by re-running the suite against the unmodified sources on the same environment. No regressions. PHPCS clean on all changed files.Changelog entry
Fix: translations no longer load before
initReading a Dokan setting builds the admin settings schema through the legacy settings bridge, and that schema is translated. Two call sites did this before the
initaction, where the plugin's text domain is loaded — the vendor layout shortcode read its setting during hook registration onplugins_loaded, and the legacy settings repository built the whole schema in its constructor just to work out which options to watch. WordPress 6.7 and later flags this with a_load_textdomain_just_in_timenotice, which was written todebug.logtwice on every request, and the redundant schema build cost time on front-end requests that only needed a single option. Both call sites now defer the read untilinitor later, and cache invalidation no longer needs the schema at all.Before Changes
debug.log, two entries per request:After Changes
No
dokan-liteentries — verified across repeated bootstraps on a Pro-active install.