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.

Sign up for GitHub

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

Jump to bottom

[Fonts API] removing files and files loading no longer needed #57972

Merged
merged 5 commits into from
Jan 23, 2024
Merged

[Fonts API] removing files and files loading no longer needed #57972

merged 5 commits into from
Jan 23, 2024

Conversation

matiasbenedetto
Copy link
Contributor

@matiasbenedetto matiasbenedetto commented Jan 18, 2024 •
edited
Loading

What?

Removing source files around the deprecated Fonts API they are not longer needed.
See: #51820

Why?

Fixes: #51820

How?

Removing the files and the conditional loading of them.

Testing Instructions

  • Run unit tests

@github-actions GitHub Actions
Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/load.php

@matiasbenedetto matiasbenedetto added the [Type] Code Quality Issues or PRs that relate to code quality label Jan 18, 2024
@hellofromtonya
Copy link
Contributor

This PR not only removes the deprecated Fonts API but also turns on the Font Library by default. Will this be a problem (thinking of Jetpack)?

@hellofromtonya
Copy link
Contributor

This PR not only removes the deprecated Fonts API but also turns on the Font Library by default. Will this be a problem (thinking of Jetpack)?

Had a conversation with @matiasbenedetto. Jetpack is now using the Font Library by default, i.e. not the Fonts API. Should be okay then.

hellofromtonya
hellofromtonya approved these changes Jan 18, 2024
View reviewed changes
Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

LGTM 👍 (And woohoo for this old, deprecated code to finally be gone.)

@matiasbenedetto
Copy link
Contributor Author

matiasbenedetto commented Jan 18, 2024 •
edited
Loading

@hellofromtonya I needed to add an additional re-declaration guard on load.php : 4e24308
I found that was needed because the CI unit tests were failing on PHP 7.0 with this message:

Fatal error: Cannot declare class WP_Font_Face, because the name is already in use in /var/www/html/wp-content/plugins/gutenberg/lib/compat/wordpress-6.4/fonts/font-face/class-wp-font-face.php on line 21

Could you check this again, please?

This was referenced Jan 19, 2024
Merged
Closed
Merged
@hellofromtonya
Copy link
Contributor

Whoops, also need to remove the Fonts API tests. This whole folder can be removed https://github.com/WordPress/gutenberg/tree/trunk/phpunit/tests/fonts-api.

hellofromtonya
hellofromtonya reviewed Jan 22, 2024
View reviewed changes
lib/load.php
require __DIR__ . '/experimental/fonts/font-library/font-library.php';

// Load the Font Face and Font Face Resolver, if not already loaded by WordPress Core.
if ( ! class_exists( 'WP_Font_Face' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@matiasbenedetto this check needs to stay as Core loads these files. If the website has < WP 6.4.0, then the files are loaded.

@hellofromtonya
Copy link
Contributor

All tests are failing (not just fonts) for:

Indirect modification of overloaded property WP_Block_Type::$variations has no effect

which was fixed a little bit ago by b2c2a08. This PR will need a rebase to pull the changes into it.

hellofromtonya
hellofromtonya approved these changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

Re-reviewing. All of the Fonts API files including the tests are now removed.

This PR is ready for merge into trunk. Thanks @matiasbenedetto ⭐

@hellofromtonya hellofromtonya removed the request for review from spacedmonkey January 22, 2024 20:45
@matiasbenedetto
Copy link
Contributor Author

Thanks for the review and the fixes 🌟

@matiasbenedetto matiasbenedetto merged commit d1d1921 into trunk Jan 23, 2024
55 checks passed
@matiasbenedetto matiasbenedetto deleted the fix/51820 branch January 23, 2024 04:50
@github-actions github-actions bot added this to the Gutenberg 17.6 milestone Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hellofromtonya hellofromtonya hellofromtonya approved these changes

Assignees
No one assigned
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
Gutenberg 17.6
Development

Successfully merging this pull request may close these issues.

[Fonts API] Remove all files once Font Library is merged
2 participants
@matiasbenedetto @hellofromtonya