Font Library Refactor #57688
Merged
Font Library Refactor #57688
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
creativecoder
added
REST API Interaction
|
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/experimental/fonts/font-library/class-wp-rest-font-faces-controller.php ❔ phpunit/data/fonts/OpenSans-Regular.otf ❔ phpunit/data/fonts/OpenSans-Regular.ttf ❔ phpunit/data/fonts/OpenSans-Regular.woff ❔ phpunit/data/fonts/OpenSans-Regular.woff2 ❔ phpunit/tests/fonts/font-library/fontFamilyBackwardsCompatibility.php ❔ phpunit/tests/fonts/font-library/fontLibraryHooks.php ❔ phpunit/tests/fonts/font-library/wpFontCollection/isConfigValid.php ❔ phpunit/tests/fonts/font-library/wpFontFamilyUtils/getFontFaceSlug.php ❔ phpunit/tests/fonts/font-library/wpRestFontCollectionsController.php ❔ phpunit/tests/fonts/font-library/wpRestFontFacesController.php ❔ phpunit/tests/fonts/font-library/wpRestFontFamiliesController.php ❔ lib/experimental/fonts/font-library/class-wp-font-collection.php ❔ lib/experimental/fonts/font-library/class-wp-font-family-utils.php ❔ lib/experimental/fonts/font-library/class-wp-font-library.php ❔ lib/experimental/fonts/font-library/class-wp-rest-font-collections-controller.php ❔ lib/experimental/fonts/font-library/class-wp-rest-font-families-controller.php ❔ lib/experimental/fonts/font-library/font-library.php ❔ lib/load.php ❔ phpunit/tests/fonts/font-library/wpFontCollection/__construct.php ❔ phpunit/tests/fonts/font-library/wpFontCollection/getConfig.php ❔ phpunit/tests/fonts/font-library/wpFontFamily/__construct.php ❔ phpunit/tests/fonts/font-library/wpFontFamilyUtils/formatFontFamily.php ❔ phpunit/tests/fonts/font-library/wpFontLibrary/registerFontCollection.php ❔ phpunit/tests/fonts/font-library/wpFontCollection/getContent.php |
creativecoder
force-pushed
the
try/font-library-refactor
branch
from
d1d353d to
42565a3
Compare
jffng
reviewed
Jan 16, 2024
lib/experimental/fonts/font-library/class-wp-rest-font-families-controller.php
Outdated
Show resolved
Hide resolved
25 tasks
creativecoder
added
the
[Status] In Progress
This was referenced Jan 17, 2024
spacedmonkey
previously requested changes
Jan 17, 2024
Comment on lines
34
to
46
| public function __construct() { | ||
| $post_type = 'wp_font_face'; | ||
| $this->post_type = $post_type; | ||
|
|
||
| $post_type_obj = get_post_type_object( $post_type ); | ||
| $this->rest_base = $post_type_obj->rest_base; | ||
|
|
||
| $parent_post_type = 'wp_font_family'; | ||
| $this->parent_post_type = $parent_post_type; | ||
| $parent_post_type_obj = get_post_type_object( $parent_post_type ); | ||
| $this->parent_base = $parent_post_type_obj->rest_base; | ||
| $this->namespace = $parent_post_type_obj->rest_namespace; | ||
| } |
There was a problem hiding this comment.
Suggested change
| public function __construct() { | |
| $post_type = 'wp_font_face'; | |
| $this->post_type = $post_type; | |
| $post_type_obj = get_post_type_object( $post_type ); | |
| $this->rest_base = $post_type_obj->rest_base; | |
| $parent_post_type = 'wp_font_family'; | |
| $this->parent_post_type = $parent_post_type; | |
| $parent_post_type_obj = get_post_type_object( $parent_post_type ); | |
| $this->parent_base = $parent_post_type_obj->rest_base; | |
| $this->namespace = $parent_post_type_obj->rest_namespace; | |
| } |
Not needed, use the parent class constructor.
There was a problem hiding this comment.
I'm a bit confused by this suggestion, and the one below to use the parent class's register_route. There's nothing I'm seeing in the parent WP_REST_Posts_Controller that handles a parent post type, or sets up nested routes like what we're using for font faces: wp/v2/font-families/<id>/font-faces.
The constructor and route registration for font faces is directly inspired by how the revisions endpoints work, as they have similarly nested routes with parents.
Please let me know what I'm missing something, and if there's an easier way to handle this!
There was a problem hiding this comment.
Note that we are able to remove the __contruct and register_routes methods from the Font Families controller, as that one uses all the standard routes and callbacks. I'll proceed with updating that.
There was a problem hiding this comment.
Update here that I was able to remove the constructor function, at least, for WP_REST_Font_Faces_Controller by using 'rest_base' => 'font-families/(?P<font_family_id>[\d]+)/font-faces' when registering the wp_font_face` custom post type, and depending only on the parent controller method.
There was a problem hiding this comment.
WP_REST_Font_Faces_Controller::__contruct removed in 32689e1
Comment on lines
48
to
100
| /** | ||
| * Registers the routes for posts. | ||
| * | ||
| * @since 6.5.0 | ||
| * | ||
| * @see register_rest_route() | ||
| */ | ||
| public function register_routes() { | ||
| register_rest_route( | ||
| $this->namespace, | ||
| '/' . $this->parent_base . '/(?P<font_family_id>[\d]+)/' . $this->rest_base, | ||
| array( | ||
| 'args' => array( | ||
| 'font_family_id' => array( | ||
| 'description' => __( 'The ID for the parent font family of the font face.', 'gutenberg' ), | ||
| 'type' => 'integer', | ||
| 'required' => true, | ||
| ), | ||
| ), | ||
| array( | ||
| 'methods' => WP_REST_Server::READABLE, | ||
| 'callback' => array( $this, 'get_items' ), | ||
| 'permission_callback' => array( $this, 'get_font_faces_permissions_check' ), | ||
| 'args' => $this->get_collection_params(), | ||
| ), | ||
| array( | ||
| 'methods' => WP_REST_Server::CREATABLE, | ||
| 'callback' => array( $this, 'create_item' ), | ||
| 'permission_callback' => array( $this, 'create_item_permissions_check' ), | ||
| 'args' => $this->get_create_params(), | ||
| ), | ||
| 'schema' => array( $this, 'get_public_item_schema' ), | ||
| ) | ||
| ); | ||
|
|
||
| register_rest_route( | ||
| $this->namespace, | ||
| '/' . $this->parent_base . '/(?P<font_family_id>[\d]+)/' . $this->rest_base . '/(?P<id>[\d]+)', | ||
| array( | ||
| 'args' => array( | ||
| 'font_family_id' => array( | ||
| 'description' => __( 'The ID for the parent font family of the font face.', 'gutenberg' ), | ||
| 'type' => 'integer', | ||
| 'required' => true, | ||
| ), | ||
| 'id' => array( | ||
| 'description' => __( 'Unique identifier for the font face.', 'gutenberg' ), | ||
| 'type' => 'integer', | ||
| 'required' => true, | ||
| ), | ||
| ), | ||
| array( | ||
| 'methods' => WP_REST_Server::READABLE, | ||
| 'callback' => array( $this, 'get_item' ), | ||
| 'permission_callback' => array( $this, 'get_font_faces_permissions_check' ), | ||
| 'args' => array(), | ||
| ), | ||
| array( | ||
| 'methods' => WP_REST_Server::DELETABLE, | ||
| 'callback' => array( $this, 'delete_item' ), | ||
| 'permission_callback' => array( $this, 'delete_item_permissions_check' ), | ||
| 'args' => array( | ||
| 'force' => array( | ||
| 'type' => 'boolean', | ||
| 'default' => false, | ||
| 'description' => __( 'Whether to bypass Trash and force deletion.', 'default' ), | ||
| ), | ||
| ), | ||
| ), | ||
| 'schema' => array( $this, 'get_public_item_schema' ), | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Suggested change
| /** | |
| * Registers the routes for posts. | |
| * | |
| * @since 6.5.0 | |
| * | |
| * @see register_rest_route() | |
| */ | |
| public function register_routes() { | |
| register_rest_route( | |
| $this->namespace, | |
| '/' . $this->parent_base . '/(?P<font_family_id>[\d]+)/' . $this->rest_base, | |
| array( | |
| 'args' => array( | |
| 'font_family_id' => array( | |
| 'description' => __( 'The ID for the parent font family of the font face.', 'gutenberg' ), | |
| 'type' => 'integer', | |
| 'required' => true, | |
| ), | |
| ), | |
| array( | |
| 'methods' => WP_REST_Server::READABLE, | |
| 'callback' => array( $this, 'get_items' ), | |
| 'permission_callback' => array( $this, 'get_font_faces_permissions_check' ), | |
| 'args' => $this->get_collection_params(), | |
| ), | |
| array( | |
| 'methods' => WP_REST_Server::CREATABLE, | |
| 'callback' => array( $this, 'create_item' ), | |
| 'permission_callback' => array( $this, 'create_item_permissions_check' ), | |
| 'args' => $this->get_create_params(), | |
| ), | |
| 'schema' => array( $this, 'get_public_item_schema' ), | |
| ) | |
| ); | |
| register_rest_route( | |
| $this->namespace, | |
| '/' . $this->parent_base . '/(?P<font_family_id>[\d]+)/' . $this->rest_base . '/(?P<id>[\d]+)', | |
| array( | |
| 'args' => array( | |
| 'font_family_id' => array( | |
| 'description' => __( 'The ID for the parent font family of the font face.', 'gutenberg' ), | |
| 'type' => 'integer', | |
| 'required' => true, | |
| ), | |
| 'id' => array( | |
| 'description' => __( 'Unique identifier for the font face.', 'gutenberg' ), | |
| 'type' => 'integer', | |
| 'required' => true, | |
| ), | |
| ), | |
| array( | |
| 'methods' => WP_REST_Server::READABLE, | |
| 'callback' => array( $this, 'get_item' ), | |
| 'permission_callback' => array( $this, 'get_font_faces_permissions_check' ), | |
| 'args' => array(), | |
| ), | |
| array( | |
| 'methods' => WP_REST_Server::DELETABLE, | |
| 'callback' => array( $this, 'delete_item' ), | |
| 'permission_callback' => array( $this, 'delete_item_permissions_check' ), | |
| 'args' => array( | |
| 'force' => array( | |
| 'type' => 'boolean', | |
| 'default' => false, | |
| 'description' => __( 'Whether to bypass Trash and force deletion.', 'default' ), | |
| ), | |
| ), | |
| ), | |
| 'schema' => array( $this, 'get_public_item_schema' ), | |
| ) | |
| ); | |
| } |
Not needed, use parent class register_route.
Please override methods like one get_items_permissions_check which custom logic / messages.
There was a problem hiding this comment.
We are intentionally omitting the WP_REST_Server::EDITABLE endpoint for existing font faces, because adding/replacing font assets for existing font faces could add considerable complexity to the endpoint. I didn't see a way to use parent::register_routes and omit that route. Please let us know if there's a good way to do that!
| /** | ||
| * Checks if a given request has access to font faces. | ||
| * | ||
| * @since 6.5.0 | ||
| * | ||
| * @return true|WP_Error True if the request has read access, WP_Error object otherwise. | ||
| */ | ||
| public function get_font_faces_permissions_check() { | ||
| $post_type = get_post_type_object( $this->post_type ); | ||
|
|
||
| if ( ! current_user_can( $post_type->cap->edit_posts ) ) { | ||
| return new WP_Error( | ||
| 'rest_cannot_read', | ||
| __( 'Sorry, you are not allowed to access font faces.', 'gutenberg' ), | ||
| array( 'status' => rest_authorization_required_code() ) | ||
| ); | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Suggested change
| /** | |
| * Checks if a given request has access to font faces. | |
| * | |
| * @since 6.5.0 | |
| * | |
| * @return true|WP_Error True if the request has read access, WP_Error object otherwise. | |
| */ | |
| public function get_font_faces_permissions_check() { | |
| $post_type = get_post_type_object( $this->post_type ); | |
| if ( ! current_user_can( $post_type->cap->edit_posts ) ) { | |
| return new WP_Error( | |
| 'rest_cannot_read', | |
| __( 'Sorry, you are not allowed to access font faces.', 'gutenberg' ), | |
| array( 'status' => rest_authorization_required_code() ) | |
| ); | |
| } | |
| return true; | |
| } |
Not needed.
There was a problem hiding this comment.
We did try inheriting WP_REST_Posts_Controller::get_items_permissions_check directly. However, it allows read access to all users (unless the context param is edit), which I don't think we want.
Font families/faces are not content, and are more like typography settings presets that are activated and used on the site by inserting them into Global Styles typography settings. I can't think of a reason why they would need to be accessed generally until after activation (which is when they can be used as fonts in Global Styles and blocks), and at that point you need to look directly at Global Styles.
We can add a context param if you think that's needed, but there isn't really a context that's used other than edit, so it doesn't seem very meaningful. The view context for fonts is effectively stored in Global Styles.
There was a problem hiding this comment.
Moved to overriding get_items_permissions_check and get_item_permissions_check instead in 32689e1
Comment on lines
381
to
384
| $data['id'] = $item->ID; | ||
| $data['theme_json_version'] = 2; | ||
| $data['parent'] = $item->post_parent; | ||
| $data['font_face_settings'] = $this->get_settings_from_post( $item ); |
There was a problem hiding this comment.
Please check to see if field has been requested like this.
$fields = $this->get_fields_for_response( $request );
// Base fields for every post.
$data = array();
if ( rest_is_field_included( 'id', $fields ) ) {
$data['id'] = $post->ID;
}
There was a problem hiding this comment.
Done in 32689e1
Comment on lines
387
to
388
| $links = $this->prepare_links( $item ); | ||
| $response->add_links( $links ); |
There was a problem hiding this comment.
Suggested change
| $links = $this->prepare_links( $item ); | |
| $response->add_links( $links ); | |
| if ( rest_is_field_included( '_links', $fields ) || rest_is_field_included( '_embedded', $fields ) ) { | |
| $links = $this->prepare_links( $item ); | |
| $response->add_links( $links ); | |
| } |
As noted above, only prepare and add links if requested.
There was a problem hiding this comment.
Done in 32689e1
| * @param WP_Post $post Post object. | ||
| * @return void | ||
| */ | ||
| function _wp_delete_font_family( $post_id, $post ) { |
There was a problem hiding this comment.
Why does this function need id and post? Can you get id from post object like this $post->ID
There was a problem hiding this comment.
This function is hooked into deleted_post, so we're using the params that are provided by that hook.
There was a problem hiding this comment.
I've renamed the function to _wp_after_delete_font_family in 32689e1 to try and make it clearer how it's being used.
packages/edit-site/src/components/global-styles/font-library-modal/local-fonts.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/font-library-modal/context.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/font-library-modal/context.js
Outdated
Show resolved
Hide resolved
| const promises = fontFacesData.map( ( faceData ) => | ||
| fetchInstallFontFace( fontFamilyId, faceData ) | ||
| ); | ||
| const responses = await Promise.allSettled( promises ); |
There was a problem hiding this comment.
Why was allSettled used over all here?
This should be wrapped in a try / catch
There was a problem hiding this comment.
Why was
allSettledused overallhere?
We’re using allSettled because all will reject as soon as one fails. allSettled will wait for each one to be completed, fulfilled, or rejected. In our use case, it’s possible to get into a state where there are some successful and some failed font face installations, and we want to know the details so we can handle the activation and error reporting accordingly.
This should be wrapped in a try / catch
This function is already being wrapped in a try/catch block where it's being called in installFont here, where we can handle the error messages on a more detailed level.
|
Size Change: +749 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
mikachan
added a commit
that referenced
this pull request
Jan 18, 2024
* Wrap error messages in sprintf * Use await rather than then * Add variables for API URLs * Update packages/edit-site/src/components/global-styles/font-library-modal/resolvers.js Co-authored-by: Jeff Ong <jonger4@gmail.com> --------- Co-authored-by: Jeff Ong <jonger4@gmail.com>
|
@spacedmonkey Thanks for all the feedback! We've gone through your comments one by one, and 2 PRs have been merged into this branch to address them:
In some cases we had questions or asked for clarification. This feature branch should be ready for another review. |
Comment on lines
+790
to
+812
| /** | ||
| * Returns relative path to an uploaded font file. | ||
| * | ||
| * The path is relative to the current fonts dir. | ||
| * | ||
| * @since 6.5.0 | ||
| * @access private | ||
| * | ||
| * @param string $path Full path to the file. | ||
| * @return string Relative path on success, unchanged path on failure. | ||
| */ | ||
| protected function relative_fonts_path( $path ) { | ||
| $new_path = $path; | ||
|
|
||
| $fonts_dir = wp_get_font_dir(); | ||
| if ( str_starts_with( $new_path, $fonts_dir['path'] ) ) { | ||
| $new_path = str_replace( $fonts_dir, '', $new_path ); | ||
| $new_path = ltrim( $new_path, '/' ); | ||
| } | ||
|
|
||
| return $new_path; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this function should not be part of the rest controller class but part of WP_Font_Library or WP_Font_Family. It won't be used just by this controller but for the WP_REST_Font_Families_Controller too to write the preview assets.
|
Flaky tests detected in dde2059. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7631536022
|
creativecoder
commented
Jan 23, 2024
packages/edit-site/src/components/global-styles/font-library-modal/library-font-variant.js
Show resolved
Hide resolved
11 tasks
swissspidy
reviewed
Jan 24, 2024
| ( empty( $config['src'] ) && empty( $config['font_families'] ) ) || | ||
| ( ! empty( $config['src'] ) && ! empty( $config['font_families'] ) ) | ||
| ) { | ||
| _doing_it_wrong( __METHOD__, __( 'Font Collection config "src" option OR "font_families" option are required.', 'gutenberg' ), '6.5.0' ); |
There was a problem hiding this comment.
Best to use sprintf placeholders for src and font_families here to prevent accidental translation
There was a problem hiding this comment.
added here: 18f9963
|
@creativecoder just FYI spotted a tiny improvement possibility for translatable strings. |
|
🙏 Thank you @swissspidy ! |
| 'map_meta_cap' => false, | ||
| 'query_var' => false, | ||
| 'show_in_rest' => true, | ||
| 'rest_base' => 'font-families/(?P<font_family_id>[\d]+)/font-faces', |
| 'post_type' => 'wp_font_family', | ||
| // Set a maximum, but in reality there will be far less than this. | ||
| 'posts_per_page' => 999, | ||
| 'update_post_meta_cache' => false, |
There was a problem hiding this comment.
Post meta is needed, as post meta is used.
There was a problem hiding this comment.
Thanks, updating here: #58333
| 'fontFamily' => array( | ||
| 'description' => 'CSS font-family value.', | ||
| 'type' => 'string', | ||
| 'default' => '', |
There was a problem hiding this comment.
@creativecoder Doesn't really matter what it is designed for. REST API endpoints get used by developer for whatever they want. REST API is an interface to WordPress, more than just gutenberg will use this REST API.
| * @return true|WP_Error True if the request has read access, WP_Error object otherwise. | ||
| */ | ||
| public function get_item_permissions_check( $request ) { | ||
| return $this->get_items_permissions_check( $request ); |
There was a problem hiding this comment.
This should do a current_user_can( 'read_post', $post->ID ) call. Permissions are based on ids.
There was a problem hiding this comment.
Updated here: #58333
| private function get_validation_errors( $font_family_settings, $files ) { | ||
| $error_messages = array(); | ||
| public function get_item_permissions_check( $request ) { | ||
| return $this->get_items_permissions_check( $request ); |
There was a problem hiding this comment.
As noted else, this permission check is not id based.
There was a problem hiding this comment.
Updated here: #58333
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
mikachan
added a commit
that referenced
this pull request
Jan 26, 2024
* Font Library: add wp_font_face post type and scaffold font face REST API controller (#57656) * Font Library: create font faces through the REST API (#57702) * Refactor Font Family Controller (#57785) * Font Family and Font Face REST API endpoints: better data handling and errors (#57843) * Font Families REST API endpoint: ensure unique font family slugs (#57861) * Font Library: delete child font faces and font assets when deleting parent (#57867) Co-authored-by: Sarah Norris <1645628+mikachan@users.noreply.github.com> * Font Library: refactor client side install functions to work with revised API (#57844) * Add batchInstallFontFaces function and related plumbing. * Fix resolver name. * Add embedding and rebuild theme.json settings for fontFamily. * Handle responses directly, add to collection before activating. Remove unused test. * Remove getIntersectingFontFaces. * Check for existing font family before installing. * Reference src, not uploadedFile key. Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> * Check for existing font family using GET /font-families?slug=. * Filter already installed font faces (determined by matching fontWeight AND fontStyle) --------- Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: Jason Crist <jcrist@pbking.com> * Cleanup/font library view error handling (#57926) * Add batchInstallFontFaces function and related plumbing. * Fix resolver name. * Add embedding and rebuild theme.json settings for fontFamily. * Handle responses directly, add to collection before activating. Remove unused test. * Remove getIntersectingFontFaces. * Check for existing font family before installing. * Reference src, not uploadedFile key. Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> * Check for existing font family using GET /font-families?slug=. * Filter already installed font faces (determined by matching fontWeight AND fontStyle) * moved response processing into the resolver for fetchGetFontFamilyBySlug * Moved response processing for font family installation to the resolver * Refactored font face installation process to handle errors more cleanly * Cleanup error handling for font library view * Add i18n function to error messages * Add TODO comment for uninstall notice --------- Co-authored-by: Jeff Ong <jonger4@gmail.com> Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: Sarah Norris <sarah@sekai.co.uk> * Font Faces endpoint: prevent creating font faces with duplicate settings (#57903) * Font Library: Update uninstall/delete on client side (#57932) * Fix delete endpoint * Update fetchUninstallFontFamily to match new format * Update uninstallFont * Add uninstall notice back in * Tidy up comments * Re-word uninstall notices * Add spacing to error message * Refactored how font family values were processed so they would retain their id, which is now used to delete a font family without fetching data via slug * Rename uninstallFont to uninstallFontFamily * Throw uninstall errors rather than returning them --------- Co-authored-by: Jason Crist <jcrist@pbking.com> * Update packages/edit-site/src/components/global-styles/font-library-modal/local-fonts.js Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com> * Font Library: address JS feedback in #57688 (#57961) * Wrap error messages in sprintf * Use await rather than then * Add variables for API URLs * Update packages/edit-site/src/components/global-styles/font-library-modal/resolvers.js Co-authored-by: Jeff Ong <jonger4@gmail.com> --------- Co-authored-by: Jeff Ong <jonger4@gmail.com> * Add missing dep in font-demo * Move notice to top of local-fonts panel * Add container around spinner * Move notice to TabPanelLayout * Remove spacer from LocalFonts * Move notice and setNotice state to context.js * Move spacer outside of notice container * Rename LocalFonts to UploadFonts * Make notices dismissible * Reset notices onRemove * Reset notice when new tab is selected * Reset notice on each action * Fix notice re-render on fetchFontCollection * Move notices below font collection title and description --------- Co-authored-by: Grant Kinney <creativecoder@users.noreply.github.com> Co-authored-by: Grant Kinney <hi@grant.mk> Co-authored-by: Jeff Ong <jonger4@gmail.com> Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: Jason Crist <jcrist@pbking.com> Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
cbravobernal
pushed a commit
that referenced
this pull request
Jan 30, 2024
* Font Library: add wp_font_face post type and scaffold font face REST API controller (#57656) * Font Library: create font faces through the REST API (#57702) * Refactor Font Family Controller (#57785) * Font Family and Font Face REST API endpoints: better data handling and errors (#57843) * Font Families REST API endpoint: ensure unique font family slugs (#57861) * Font Library: delete child font faces and font assets when deleting parent (#57867) Co-authored-by: Sarah Norris <1645628+mikachan@users.noreply.github.com> * Font Library: refactor client side install functions to work with revised API (#57844) * Add batchInstallFontFaces function and related plumbing. * Fix resolver name. * Add embedding and rebuild theme.json settings for fontFamily. * Handle responses directly, add to collection before activating. Remove unused test. * Remove getIntersectingFontFaces. * Check for existing font family before installing. * Reference src, not uploadedFile key. Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> * Check for existing font family using GET /font-families?slug=. * Filter already installed font faces (determined by matching fontWeight AND fontStyle) --------- Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: Jason Crist <jcrist@pbking.com> * Cleanup/font library view error handling (#57926) * Add batchInstallFontFaces function and related plumbing. * Fix resolver name. * Add embedding and rebuild theme.json settings for fontFamily. * Handle responses directly, add to collection before activating. Remove unused test. * Remove getIntersectingFontFaces. * Check for existing font family before installing. * Reference src, not uploadedFile key. Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> * Check for existing font family using GET /font-families?slug=. * Filter already installed font faces (determined by matching fontWeight AND fontStyle) * moved response processing into the resolver for fetchGetFontFamilyBySlug * Moved response processing for font family installation to the resolver * Refactored font face installation process to handle errors more cleanly * Cleanup error handling for font library view * Add i18n function to error messages * Add TODO comment for uninstall notice --------- Co-authored-by: Jeff Ong <jonger4@gmail.com> Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: Sarah Norris <sarah@sekai.co.uk> * Font Faces endpoint: prevent creating font faces with duplicate settings (#57903) * Font Library: Update uninstall/delete on client side (#57932) * Fix delete endpoint * Update fetchUninstallFontFamily to match new format * Update uninstallFont * Add uninstall notice back in * Tidy up comments * Re-word uninstall notices * Add spacing to error message * Refactored how font family values were processed so they would retain their id, which is now used to delete a font family without fetching data via slug * Rename uninstallFont to uninstallFontFamily * Throw uninstall errors rather than returning them --------- Co-authored-by: Jason Crist <jcrist@pbking.com> * Update packages/edit-site/src/components/global-styles/font-library-modal/local-fonts.js Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com> * Font Library: address JS feedback in #57688 (#57961) * Wrap error messages in sprintf * Use await rather than then * Add variables for API URLs * Update packages/edit-site/src/components/global-styles/font-library-modal/resolvers.js Co-authored-by: Jeff Ong <jonger4@gmail.com> --------- Co-authored-by: Jeff Ong <jonger4@gmail.com> * Add missing dep in font-demo * Move notice to top of local-fonts panel * Add container around spinner * Move notice to TabPanelLayout * Remove spacer from LocalFonts * Move notice and setNotice state to context.js * Move spacer outside of notice container * Rename LocalFonts to UploadFonts * Make notices dismissible * Reset notices onRemove * Reset notice when new tab is selected * Reset notice on each action * Fix notice re-render on fetchFontCollection * Move notices below font collection title and description --------- Co-authored-by: Grant Kinney <creativecoder@users.noreply.github.com> Co-authored-by: Grant Kinney <hi@grant.mk> Co-authored-by: Jeff Ong <jonger4@gmail.com> Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: Jason Crist <jcrist@pbking.com> Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
youknowriad
pushed a commit
that referenced
this pull request
Jan 31, 2024
* Font Library: add wp_font_face post type and scaffold font face REST API controller (#57656) * Font Library: create font faces through the REST API (#57702) * Refactor Font Family Controller (#57785) * Font Family and Font Face REST API endpoints: better data handling and errors (#57843) * Font Families REST API endpoint: ensure unique font family slugs (#57861) * Font Library: delete child font faces and font assets when deleting parent (#57867) Co-authored-by: Sarah Norris <1645628+mikachan@users.noreply.github.com> * Font Library: refactor client side install functions to work with revised API (#57844) * Add batchInstallFontFaces function and related plumbing. * Fix resolver name. * Add embedding and rebuild theme.json settings for fontFamily. * Handle responses directly, add to collection before activating. Remove unused test. * Remove getIntersectingFontFaces. * Check for existing font family before installing. * Reference src, not uploadedFile key. Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> * Check for existing font family using GET /font-families?slug=. * Filter already installed font faces (determined by matching fontWeight AND fontStyle) --------- Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: Jason Crist <jcrist@pbking.com> * Cleanup/font library view error handling (#57926) * Add batchInstallFontFaces function and related plumbing. * Fix resolver name. * Add embedding and rebuild theme.json settings for fontFamily. * Handle responses directly, add to collection before activating. Remove unused test. * Remove getIntersectingFontFaces. * Check for existing font family before installing. * Reference src, not uploadedFile key. Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> * Check for existing font family using GET /font-families?slug=. * Filter already installed font faces (determined by matching fontWeight AND fontStyle) * moved response processing into the resolver for fetchGetFontFamilyBySlug * Moved response processing for font family installation to the resolver * Refactored font face installation process to handle errors more cleanly * Cleanup error handling for font library view * Add i18n function to error messages * Add TODO comment for uninstall notice --------- Co-authored-by: Jeff Ong <jonger4@gmail.com> Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: Sarah Norris <sarah@sekai.co.uk> * Font Faces endpoint: prevent creating font faces with duplicate settings (#57903) * Font Library: Update uninstall/delete on client side (#57932) * Fix delete endpoint * Update fetchUninstallFontFamily to match new format * Update uninstallFont * Add uninstall notice back in * Tidy up comments * Re-word uninstall notices * Add spacing to error message * Refactored how font family values were processed so they would retain their id, which is now used to delete a font family without fetching data via slug * Rename uninstallFont to uninstallFontFamily * Throw uninstall errors rather than returning them --------- Co-authored-by: Jason Crist <jcrist@pbking.com> * Update packages/edit-site/src/components/global-styles/font-library-modal/local-fonts.js Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com> * Font Library: address JS feedback in #57688 (#57961) * Wrap error messages in sprintf * Use await rather than then * Add variables for API URLs * Update packages/edit-site/src/components/global-styles/font-library-modal/resolvers.js Co-authored-by: Jeff Ong <jonger4@gmail.com> --------- Co-authored-by: Jeff Ong <jonger4@gmail.com> * Add missing dep in font-demo * Move notice to top of local-fonts panel * Add container around spinner * Move notice to TabPanelLayout * Remove spacer from LocalFonts * Move notice and setNotice state to context.js * Move spacer outside of notice container * Rename LocalFonts to UploadFonts * Make notices dismissible * Reset notices onRemove * Reset notice when new tab is selected * Reset notice on each action * Fix notice re-render on fetchFontCollection * Move notices below font collection title and description --------- Co-authored-by: Grant Kinney <creativecoder@users.noreply.github.com> Co-authored-by: Grant Kinney <hi@grant.mk> Co-authored-by: Jeff Ong <jonger4@gmail.com> Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: Jason Crist <jcrist@pbking.com> Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
youknowriad
added
Backported to WP Core
|
@creativecoder, was the I'm removing unnecessary compat layers from the plugin, and this test is now failing due to missing the |
|
@Mamaduka Because the legacy version of the font library was never in core, there isn't a reason to port the function or tests there. And It's been long enough that I think |
|
Thanks, @creativecoder! Is the By the way, here's the clean-up PR - #64096. |
@Mamaduka That's a bit different--the default fonts folder was moved from I'd err on the side of leaving that code in, at least until the WP 6.7 release, as I think there were a lot more people using the font library at that point. And the new folder location is only used for newly installed fonts, so existing fonts in the old location will still depend on the hooks in the related code. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a feature branch for refactoring the Font Library implementation.
The main purpose is to make the changes necessary to reimplement the REST API portion of the feature, including front-end changes to work with the new API implementation.
wp/v2/font-familiesendpointswp/v2/font-families/<id>/font-facesendpointswp/v2/font-collectionsendpointsSee #55278 and #57616 for more context
Testing Instructions
trunk, then loading this branch and see that they've migrated to the new data format and continue to work as expected