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

Font Library: Add all REST API endpoints. #6034

Conversation

youknowriad
Copy link
Contributor

Trac ticket: https://core.trac.wordpress.org/ticket/59166

This builds on top of the initial Font Library API PR and adds the necessary REST API controllers to make the Font Library work properly.

Testing instructions

  • You can add the following piece of code anywhere
function gutenberg_register_font_collections() {
	wp_register_font_collection( 'google-fonts', 'https://raw.githubusercontent.com/WordPress/google-fonts-to-wordpress-collection/01aa57731575bd13f9db8d86ab80a2d74e28a1ac/releases/gutenberg-17.6/collections/google-fonts-with-preview.json' );
}
add_action( 'init', 'gutenberg_register_font_collections' );
  • Go to the site editor
  • Open the global styles panel
  • Open typography
  • Click on the "Aa" icon to open the font library
  • On the third panel, you should be able to enable google fonts and use them properly.

@youknowriad youknowriad self-assigned this Feb 6, 2024
Copy link

github-actions bot commented Feb 6, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core SVN

Core Committers: Use this line as a base for the props when committing in SVN:

Props get_dave, youknowriad, mmaattiiaass, grantmkin, swissspidy, mcsf, jorbin, ocean90.

GitHub Merge commits

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: getdave <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: matiasbenedetto <[email protected]>
Co-authored-by: creativecoder <[email protected]>
Co-authored-by: swissspidy <[email protected]>
Co-authored-by: mcsf <[email protected]>
Co-authored-by: aaronjorbin <[email protected]>
Co-authored-by: ocean90 <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@youknowriad
Copy link
Contributor Author

Working on fixing the unit tests.

Comment on lines +595 to +596
// Disable autosave endpoints for font families.
'autosave_rest_controller_class' => 'stdClass',
Copy link
Member

Choose a reason for hiding this comment

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

Aside:

We should really find a better way to avoid hacks like this. I don't think this is a clean way at all, and devs will just copy this sooner or later.

If workarounds like this are added/needed in GB it's a sign that we should improve core, not just bring the workaround into core.

Maybe it could be handled via post type supports, i.e. add_post_type_support( 'autosave' ) / remove_post_type_support( 'autosave' ). I suppose https://core.trac.wordpress.org/ticket/41172 could be used to track this, maybe I can take a stab at it.

Copy link

github-actions bot commented Feb 6, 2024

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@swissspidy
Copy link
Member

@youknowriad In #6027 it was mentioned that \WP_Font_Utils::get_allowed_font_mime_types() (which so far is only used in tests) is needed for the REST API. But I don't see it being used here at all. What am I missing?

@youknowriad
Copy link
Contributor Author

@swissspidy Yes, that function is used here

protected function handle_font_file_upload( $file ) {
add_filter( 'upload_mimes', array( 'WP_Font_Utils', 'get_allowed_font_mime_types' ) );
add_filter( 'upload_dir', 'wp_get_font_dir' );
$overrides = array(
'upload_error_handler' => array( $this, 'handle_font_file_upload_error' ),
// Arbitrary string to avoid the is_uploaded_file() check applied
// when using 'wp_handle_upload'.
'action' => 'wp_handle_font_upload',
// Not testing a form submission.
'test_form' => false,
// Seems mime type for files that are not images cannot be tested.
// See wp_check_filetype_and_ext().
'test_type' => true,
// Only allow uploading font files for this request.
'mimes' => WP_Font_Utils::get_allowed_font_mime_types(),
);
$uploaded_file = wp_handle_upload( $file, $overrides );
remove_filter( 'upload_dir', 'wp_get_font_dir' );
remove_filter( 'upload_mimes', array( 'WP_Font_Utils', 'get_allowed_font_mime_types' ) );
return $uploaded_file;
}

@youknowriad
Copy link
Contributor Author

Anyone knows how to regenerate the wp-api-generated.js fixture?

@ocean90
Copy link
Member

ocean90 commented Feb 6, 2024

Anyone knows how to regenerate the wp-api-generated.js fixture?

Try running:

npm run test:php -- --filter test_build_wp_api_client_fixtures

or:

npm run test:php -- --filter WP_Test_REST_Schema_Initialization

@youknowriad youknowriad force-pushed the add/font-library-rest-controllers branch from 72b119f to 9635c4e Compare February 6, 2024 11:52
@youknowriad
Copy link
Contributor Author

@ocean90 Both of these options return a different version of that file 🤷 Not sure what I'm doing wrong.

Copy link

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Looking pretty good.

@youknowriad
Copy link
Contributor Author

Looks like all the feedback have been addressed here. I'll be committing this shortly if there's no other blocker.

@youknowriad
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants