Skip to content
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

3.18: the local hosted google font file shall match the required structure #7143

Open
Mai-Saad opened this issue Nov 26, 2024 · 4 comments · Fixed by #7166
Open

3.18: the local hosted google font file shall match the required structure #7143

Mai-Saad opened this issue Nov 26, 2024 · 4 comments · Fixed by #7166
Assignees
Labels
type: bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@Mai-Saad
Copy link
Contributor

Describe the bug
Local hosted google font shall meet the expectations mentioned here #7062 (comment)
currently:
1- no /fonts after /google-fonts
2- hash is added to the font file (while hash was mentioned only for CSS file)

To Reproduce
Steps to reproduce the behavior:

  1. Enable host google fonts locally
  2. Visit the page https://new.rocketlabsqa.ovh/combine_v1fonts_template/
  3. check source and open the local combined file => google fonts mentioned there are hashed and the path not having /fonts

Expected behavior
A clear and concise description of what you expected to happen.
URL like https://new.rocketlabsqa.ovh/wp-content_new/cache/fonts/1/google-font/fonts/s/anonymouspro/v21/rP2Bp2a15UIB7Un-bOeISG3pHl829RH9.woff2 instead of https://new.rocketlabsqa.ovh/wp-content_new/cache/fonts/1/google-font/b/0/e/c20c0038dca463f0adee9b315f221/s/anonymouspro/v21/rP2Bp2a15UIB7Un-bOeISG3pHl829RH9.woff2
Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

  • OS: [e.g. iOS]
  • Browser [e.g. chrome, safari]
  • Version [e.g. 22]

Smartphone (please complete the following information):

  • Device: [e.g. iPhone6]
  • OS: [e.g. iOS8.1]
  • Browser [e.g. stock browser, safari]
  • Version [e.g. 22]

Additional context
Add any other context about the problem here.
slack thread https://wp-media.slack.com/archives/CUKB44GNN/p1732610252901139

@Mai-Saad Mai-Saad added the type: bug Indicates an unexpected problem or unintended behavior label Nov 26, 2024
@piotrbak piotrbak added this to the 3.18 milestone Nov 26, 2024
@piotrbak
Copy link
Contributor

@wp-media/engineering-plugin-team Was there any reason to have that kind of structure with hash?

@remyperona
Copy link
Contributor

The font is stored at the same level as the CSS file, but that can be updated

@Miraeld Miraeld self-assigned this Dec 2, 2024
@Miraeld
Copy link
Contributor

Miraeld commented Dec 2, 2024

Scope a solution

In https://github.com/wp-media/wp-rocket/blob/3201ce5d91a585a8a3298a7255cb65cf8b0089ae/inc/Engine/Media/Fonts/Filesystem.php,

we need to modify

$local_path = $file . $font_path;

in order to take into account the fonts folder to follow the structure /wp-content/cache/fonts/1/google-font/fonts/

Finally, in here:

$local_url = content_url( $relative_path . $font_path );

We need to adapt the new path so we are modifying correctly the content of the css file to include the correct font file.

Effort

S

@remyperona
Copy link
Contributor

I'd like to suggest a couple of changes concerning the file structure:

  • update from google-font to google-fonts
  • store the fonts in a sub-directory fonts
  • store the CSS in a sub-directory css

That way we will be able to clean the files individually in an easier way

What Gael proposed in term of solution is a good basis to do that, with a few more changes needed in the frontend controller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants