Fix - Kirki downloading Google Fonts on every front-end request - #116
Merged
Merged
Conversation
Kirki's bundled webfont module tries to save Google Fonts locally so visitors never contact Google directly, but it does that download inline while a real visitor's page is rendering, and doesn't remember a failed attempt: if wp-content/fonts isn't writable, every single request re-attempts the same failed download (6+ seconds and a batch of PHP warnings, per visitor, forever). - kirki_enqueue_google_fonts is filtered to skip local generation entirely when the fonts directory (or its parent, if not created yet) isn't writable, so those installs get a fast page with a system-font fallback instead of a slow one with repeated failed downloads. - On a writable install, the first-ever download after a Customizer save no longer happens on a real visitor's request: a background WP-Cron tick (scheduled a few seconds after save) warms the local files instead, via Kirki's own normal front-end code path. - Removed the block editor's direct request to fonts.googleapis.com; it's the one request in this whole flow the front end already avoids by design, and dropping it is a one-line fix. Verified: an isolated test of the writability filter and the prewarm-scheduling logic covers "not yet created", "exists but unwritable", "writable" and "already scheduled" cases. See #88. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
QA suite — refused, no regression specThis PR changes product source but adds no spec, so the suite was Source files changed with no matching spec |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Five moderate issues remain unresolved.
Review effort: Lite
Findings: 3
Open (3)
What changed in this PR
This PR defers Kirki Google Font downloads, handles unwritable font storage, and removes direct block-editor Google Fonts requests.
Changes:
- Adds filesystem checks and background WP-Cron prewarming.
- Removes direct editor font loading.
- Loads the new font-handling helper.
Review findings:
- Moderate (4 votes): Prewarming runs after every Customizer save instead of only font changes.
- Moderate (4 votes): Loopback requests disable TLS verification.
- Moderate (3 votes): Existing files at the fonts path are treated as valid directories.
- Moderate (1 vote): Failed loopbacks are consumed without retry.
- Moderate (1 vote): Programmatic font migrations do not trigger prewarming.
| File | Description |
|---|---|
inc/kirki-local-fonts.php |
Adds font download safeguards and background prewarming. |
functions.php |
Removes direct editor font loading and includes the helper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| function flash_kirki_local_fonts_writable() { | ||
| $dir = WP_CONTENT_DIR . '/fonts'; | ||
|
|
||
| return file_exists( $dir ) ? wp_is_writable( $dir ) : wp_is_writable( WP_CONTENT_DIR ); |
| wp_schedule_single_event( time() + 5, 'flash_kirki_prewarm_fonts' ); | ||
| } | ||
| } | ||
| add_action( 'customize_save_after', 'flash_kirki_schedule_font_prewarm' ); |
| add_action( 'customize_save_after', 'flash_kirki_schedule_font_prewarm' ); | ||
|
|
||
| function flash_kirki_prewarm_fonts() { | ||
| wp_remote_get( home_url( '/' ), array( 'timeout' => 15, 'sslverify' => false ) ); |
Same corrections as themegrill/flash-pro's matching commit: 1. The writability check discarded fonts that were already successfully downloaded before the directory became read-only (e.g. "build with write access, deploy read-only" hosting). Read access and write access are different things; a site with existing local files should keep serving them regardless of current writability. The gate now only skips generation when nothing has ever been cached AND the directory is unwritable -- the actual severe case from #88 -- not whenever it's merely unwritable right now. 2. Reverted removing the block editor's Google Fonts request. Checked whether Kirki's own async webfont loader covers the block editor's preview area (which would have made the removal safe) and it doesn't: Async.php only enqueues its loader when $wp_customize or is_customize_preview() is true, neither of which is true on the block editor screen. Removing that line would have silently broken the heading-font preview in the editor with nothing replacing it -- a real regression for something the issue itself rates as a non-functional consistency nicety, not worth that trade-off. Verified with an expanded isolated test: the "something was already cached before it became unwritable" case now correctly keeps serving fonts instead of discarding them, on top of the existing four cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
wp_schedule_single_event() only queues an event; WordPress's default pseudo-cron only checks for due events on a subsequent front-end request, so on a low-traffic site the prewarm could sit unfired for an unbounded amount of time. Schedule the event for "now" instead of 5 seconds out, and call WordPress's own spawn_cron() right after so the loopback dispatch is triggered immediately rather than waited for. spawn_cron() is non-blocking (fire-and-forget, ~0.01s to open the socket), so this adds no measurable delay to the Customizer save request itself. If spawn_cron() is rate-limited by WP_CRON_LOCK_TIMEOUT, the already-persisted event still fires on the next normal cron check, same as before - so this is a strict improvement with no new failure mode. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…iles Live Playwright/WP-CLI reproduction of issue #88's unwritable-directory case (chmod-equivalent via icacls on Windows) exposed a real regression: flash_kirki_local_fonts_ready() treated a non-empty kirki_downloaded_font_files option as proof that cached fonts exist, but that option lives in the database and can outlive the actual files - e.g. a container redeploy resets the filesystem while the DB persists. With a stale option and an empty unwritable directory, the gate said "ready", so Kirki still hit the same synchronous download-and-fail loop on every request that issue #88 reports (confirmed live: 500-file every request, identical PHP warnings in the log, until this fix). Replaced the option check with a direct glob() for physical files under the fonts directory - the only source of truth that can't go stale independently of the filesystem it's describing. Re-verified live: same unwritable+stale-option scenario now resolves in ~1-2s with zero warnings, and the original regression guard (unwritable dir that still has genuinely-cached files on disk) still returns ready, confirmed with an actual cached file on a write-denied directory. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
kirki_enqueue_google_fonts is also used by Async.php, which loads fonts directly from Google's CDN using webfontloader.js in the WordPress Editor and Customizer. This path doesn't touch the server/filesystem, so it isn't affected by the original issue. This fix also blocks those fonts when the directory is unwritable, though. Verified on wp-admin/post-new.php: baseline loads webfontloader.js and Montserrat, but with this filter returning [], the script never enqueues and the editor preview breaks. Could this be scoped to Embed.php's download path instead of the shared filter? Same cached-font gap as the flash-pro version too: flash_kirki_local_fonts_ready() treats the directory as ready if it finds any cached font file, so a site with one cached font but an unwritable dir will still retry-and-fail forever for any newly selected font. |
kirki_enqueue_google_fonts is used by both Embed.php (the real, front- end-only download path this fix targets) and Async.php (webfontloader.js, loaded on admin/customize-preview pages - never touches the filesystem). Emptying the font list whenever the fonts dir was unwritable blocked the Async path too, breaking font previews in the block editor and Customizer on exactly the hosts this fix was meant to help. Now a no-op whenever is_admin() or is_customize_preview() is true, mirroring Kirki's own Webfonts::get_method() gate between the two paths. Also scoped the readiness check to the specific font(s) being requested instead of "any font is cached anywhere" - a host with one font already cached but an unwritable directory would otherwise still attempt (and retry-loop on) every newly selected font. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Author
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
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.
Closes themegrill/flash-pro#88
Summary
Kirki's bundled font module downloads Google Fonts locally so visitors never contact Google directly, but it does that download inline while a real visitor's page is rendering, and never remembers a failed attempt. Traced the exact mechanism in the bundled Kirki source (
Webfonts/Downloader.php,Webfonts/Embed.php):get_option( 'kirki_downloaded_font_files' )then marks them done.wp-content/fontsisn't writable, that option key is never set (the write fails), so every single subsequent request re-attempts the identical failed download — 6+ seconds and a batch of PHP warnings, per visitor, forever, on affected hosts.Fix
New
inc/kirki-local-fonts.php, three small hooks, nothing in the vendored Kirki code touched:kirki_enqueue_google_fontsfilter empties the font list (skip download attempt entirely, fall back to system fonts) whenever the fonts directory — or its parent, if not created yet — isn't writable. This is a local filesystem check, not a network call, so it's cheap on every request and directly kills the "retry forever" failure mode.customize_save_afterschedules a one-off WP-Cron tick a few seconds later that pings the homepage (wp_remote_get), so the first-ever download after a real font change happens in the background instead of on a real visitor's request. Reuses Kirki's own existing front-end code path rather than reimplementing font generation.fonts.googleapis.com(flash_block_editor_styles()) — the one place in this flow that was hitting Google directly regardless, and removing it is one line.Testing
Isolated test of
flash_kirki_skip_fonts_when_unwritable()and the prewarm-scheduling logic (WP functions stubbed, no real WP needed): fonts-dir-not-yet-created+writable, not-yet-created+unwritable, exists+unwritable, and writable-with-a-duplicate-save (schedules exactly once) all behave as intended.🤖 Generated with Claude Code