-
Notifications
You must be signed in to change notification settings - Fork 232
Fix #7417: Preload fonts external domains #7519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #7417: Preload fonts external domains #7519
Conversation
- Introduced a filter for external font domains to exclude from processing in the Controller. - Added a method in Subscriber to retrieve external font exclusions for beacon configuration. - Implemented a method in DynamicLists to get external font exclusions from dynamic lists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR transitions WP Rocket's font preloading from a hardcoded allowlist approach to a flexible exclusion-based model. The change allows processing of fonts from all external domains by default while providing mechanisms to exclude problematic domains through filters and dynamic lists.
- Removes hardcoded external font domain allowlist and tag name exclusions from JavaScript
- Adds configurable external font exclusions via
rocket_external_font_exclusions
filter - Integrates dynamic lists system for centralized exclusion management
Reviewed Changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
assets/js/wpr-beacon.js | Removes hardcoded exclusions and implements configurable external font filtering |
inc/Engine/Media/PreloadFonts/Frontend/Controller.php | Adds external_font_exclusions to beacon data via filter |
inc/Engine/Media/PreloadFonts/Frontend/Subscriber.php | Implements filter handler for external font exclusions |
inc/Engine/Optimization/DynamicLists/DynamicLists.php | Adds method to retrieve exclusions from dynamic lists |
tests/* | Comprehensive test coverage for new functionality |
Comments suppressed due to low confidence (1)
tests/Integration/inc/Engine/Media/PreloadFonts/Frontend/Subscriber/getExternalFontExclusions.php:3
- The namespace path
inc\Media\PreloadFonts
doesn't match the actual class pathinc\Engine\Media\PreloadFonts
. This should beWP_Rocket\Tests\Integration\inc\Engine\Media\PreloadFonts\Frontend\Subscriber
.
namespace WP_Rocket\Tests\Integration\inc\Media\PreloadFonts\Frontend\Subscriber;
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to test this PR with templates having numerous external styles that are not font styles. I worry about useless fetch request for external styles. Maybe this is an edge case.
Thank you, @Miraeld, for this PR. Testing Notes so far:
As a result, only the external fonts from bunny.net and fonts.cdnfonts.com are actually excluded — these being the only domains from the filter list that WP Rocket was able to preconnect to. ![]() • Fontshare Case (shared in Slack thread): • External fonts injected via <script> and used below the fold, will still be preloaded, but only if the fonts are hosted on domains other than Google Fonts (e.g. Bunny Fonts). < to be handled in separate GH #7541 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA done. All tests are passing.
testrail-report-749.pdf
Description
Fixes #7417
This enhancement transitions WP Rocket's font preloading from a hardcoded allowlist approach to an exclusion-based model for external font processing. This change allows WP Rocket to process fonts from all external domains by default, while providing mechanisms to exclude problematic domains.
Type of change
Detailed scenario
What was tested
Automated Testing:
Manual Testing:
rocket_external_font_exclusions
filter properly merges developer exclusions with dynamic list exclusionsexternal_font_exclusions
array is correctly passed to the JavaScript beacon viarocket_beacon_data
How to test
Testing Steps
console.log(rocket_beacon_data.external_font_exclusions)
external_font_exclusions
to the dynamic lists dataTechnical description
Documentation
This pull request introduces significant changes to improve font processing and external font exclusion handling in the
wpr-beacon.js
file and related backend logic. It removes unnecessary methods and constants, adds a new configuration for excluding external fonts, and includes extensive test coverage for the new functionality.Frontend Changes:
EXCLUDED_TAG_NAMES
constant and associated methods (canElementBeStyledWithFontFamily
andcanElementBeProcessed
) fromassets/js/wpr-beacon.js
to simplify the font processing logic. [1] [2] [3]Backend Changes:
rocket_external_font_exclusions
, to allow dynamic configuration of external font exclusions. This was implemented inFrontend/Controller.php
andFrontend/Subscriber.php
. [1] [2] [3]get_external_font_exclusions
inDynamicLists.php
to retrieve external font exclusions from dynamic lists.Test Coverage:
Frontend/Subscriber
andDynamicLists
. [1] [2] [3] [4]New dependencies
None
Risks
None
Mandatory Checklist
Code validation
Code style