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

[Fonts API] Remove global from wp_print_fonts() #50225

Merged
merged 1 commit into from
May 2, 2023

Conversation

hellofromtonya
Copy link
Contributor

@hellofromtonya hellofromtonya commented May 1, 2023

Partially fixes #50075.
Sets up decision making for which fonts to print for PR #50151.

What?

Removes the global check. Instead checks if any fonts are registered; if no, bails out.

Why?

The original design (being replaced by this PR) comes from wp_print_styles().

The global check exists to avoid instantiating WP_Fonts when invoking wp_print_fonts(). In other words, if wp_print_fonts() is called before the API is used, it would bail out. This technique saves the overhead within wp_fonts(), i.e. instantiating WP_Fonts and registering the local provider. However, this overhead is tiny and, in itself, not worthy of having a global variable for this purpose.

The function lacks a proper bail out check: if there are no fonts registered, then there's nothing to print. The global check assumed fonts are registered; however, this is a false assumption.

How?

Removes the global check.

Replaces it with wp_fonts()->get_registered_font_families(). If it returns empty, then there are no fonts registered to print. Even if handles are passed into the wp_print_fonts(), those handles must be registered.

Checking if no fonts are registered:

Testing Instructions

Visually Test:

Set up on your test site:

  1. Activate the Gutenberg plugin.
  2. Activate TT3 theme.

Before applying this PR:

  1. Go to the Site Editor > Styles > Typography.
  2. Using your browser's dev tools, search the HMTL for wp-fonts-local <style> element.
  3. Copying the internals of that element. You'll use this to compare after applying this PR.

After applying this PR:

  1. Go to the Site Editor > Styles > Typography.
  2. Using your browser's dev tools, search the HMTL for wp-fonts-local <style> element.
  3. Compare the styles to the before styles from above. They should match.

To test the early bail out if no fonts are registered:

  1. Add the following code to wp-content/mu-plugins/test.php:
<?php

var_dump( wp_print_fonts() );
exit;

An empty array should be displayed in your browser.

Running the automated tests

All tests should pass when running the PHPUnit tests:

npm run test:unit:php -- --filter Tests_Fonts_WpPrintFonts

@hellofromtonya hellofromtonya self-assigned this May 1, 2023
@hellofromtonya hellofromtonya added [Feature] Fonts API [Type] Enhancement A suggestion for improvement. [Type] Code Quality Issues or PRs that relate to code quality labels May 1, 2023
@hellofromtonya hellofromtonya marked this pull request as ready for review May 1, 2023 21:16
@hellofromtonya hellofromtonya requested review from aristath and removed request for spacedmonkey May 2, 2023 17:52
Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me.
I have tested this PR, and the contents of the style#wp-fonts-local elements is the same before and after the patch.
Therefore, I approve this PR.

@hellofromtonya hellofromtonya merged commit b91ce46 into trunk May 2, 2023
@hellofromtonya hellofromtonya deleted the try/remove-global-wp_print_fonts branch May 2, 2023 20:17
@github-actions github-actions bot added this to the Gutenberg 15.8 milestone May 2, 2023
@hellofromtonya hellofromtonya added the Backport to Gutenberg Minor Release Pull request that needs to be backported to a Gutenberg minor release label May 8, 2023
@bph
Copy link
Contributor

bph commented May 9, 2023

I just cherry-picked this PR to the release/15.7 branch to get it included in the next release: c922336

@bph bph removed the Backport to Gutenberg Minor Release Pull request that needs to be backported to a Gutenberg minor release label May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fonts API] Remove the global $wp_fonts
3 participants