Font Library REST API endpoints: address initial feedback from feature branch #57946

creativecoder · 2024-01-18T04:38:10Z

What?

Addresses the initial feedback in #57688, including

Registering wp_font_face with show_in_rest as true and removing the manual route registration
Removing the constructors of both WP_REST_Font_Families_Controller and WP_REST_Font_Faces_Controller, and depending on the parent constructor
Removing the register_routes method of WP_REST_Font_Families_Controller and depending on the parent method
Replacing custom permission_callback methods and overriding the default ones, where needed
Adding a context param to both WP_REST_Font_Families_Controller and WP_REST_Font_Faces_Controller that defaults to edit
Adding tests for context param
Conditionally adding all fields in prepare_item_for_response to support the _fields arg
Replace get_posts calls with WP_Query and turning off the meta and term caches
Adding translation functions to strings where they were missing
Unsetting unwanted collection params from parent::get_collection_params()

Also, some bug fixes and improvements

Turning of batching for both WP_REST_Font_Families_Controller and WP_REST_Font_Faces_Controller, since it doesn't work with file uploads
Prevent font faces from being created or deleted when the parent font family id in the route is invalid
Fix race condition that sometimes causes unit tests to fail
Renaming hooked functions for deleting font family and font face posts to make it clearer what they're for
Adding a test to make sure embedding works correctly

Why?

See #55278

Testing Instructions

Minimal functionality changes

Review code changes against feedback comments
Smoke test font library
Ensure unit tests are still passing
Try creating or deleting a font face using an invalid font family id in the url: you should now get an error

github-actions · 2024-01-18T04:39:24Z

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-autosave-fonts-controller.php
❔ lib/experimental/fonts/font-library/class-wp-rest-font-faces-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/wpRestFontFacesController.php
❔ phpunit/tests/fonts/font-library/wpRestFontFamiliesController.php

jffng

Giving this a smoke test, and I'm getting a 404 in the embedded font_faces in the response from the /font-families endpoint:

 "_embedded": {
            "font_faces": [
                {
                    "code": "rest_font_face_parent_id_mismatch",
                    "message": "The font face does not belong to the specified font family with id of \"147\"",
                    "data": {
                        "status": 404
                    }
                },
                ...

I can confirm the parent font family post has an ID of 147, and it works as expected on the feature branch. Looking into why this breaks on the PR.

EDIT

In this comparison, (int) $response->data['parent'] is evaluating to 0 in my testing, so the error condition is met and thrown.

…ding font faces

creativecoder · 2024-01-18T18:20:23Z

lib/experimental/fonts/font-library/class-wp-rest-font-families-controller.php

-		$deleted = parent::delete_item( $request );
-
-		if ( is_wp_error( $deleted ) ) {
-			return $deleted;
-		}
-
-		foreach ( $this->get_font_face_ids( $font_family_id ) as $font_face_id ) {
-			wp_delete_post( $font_face_id, true );
-		}


Deleting child font families is now handled by callbacks hooked into deleted_post, so we don't need to do it here in the controller

creativecoder · 2024-01-18T18:29:50Z

Giving this a smoke test, and I'm getting a 404 in the embedded font_faces in the response from the /font-families endpoint:

Thank you @jffng for catching this. I've fixed the issue, and added a unit test to catch any regressions in the future.

creativecoder

I've left inline comments to explain the changes in hopes of making this easier to review.

creativecoder · 2024-01-18T20:04:45Z

lib/experimental/fonts/font-library/class-wp-rest-autosave-fonts-controller.php

+<?php
+/**
+ * WP_REST_Autosave_Fonts_Controller class.
+ *
+ * This file contains the class for the REST API Autosave Font Families and Font Faces Controller.
+ *
+ * @package    WordPress
+ * @subpackage REST_API
+ * @since      6.5.0
+ */
+
+if ( class_exists( 'WP_REST_Autosave_Fonts_Controller' ) ) {
+	return;
+}
+
+/**
+ * Autosave Font Families Controller class.
+ *
+ * @since 6.5.0
+ */
+class WP_REST_Autosave_Fonts_Controller {
+	public function register_routes() {
+		// disable autosave endpoints for font families and faces.
+	}
+}


Here I'm making the empty autosave controller generic so we can use it for both wp_font_family and wp_font_face post types.

creativecoder · 2024-01-18T20:06:41Z

lib/experimental/fonts/font-library/class-wp-rest-font-faces-controller.php

@@ -55,7 +33,7 @@ public function __construct() {
 	public function register_routes() {
 		register_rest_route(
 			$this->namespace,
-			'/' . $this->parent_base . '/(?P<font_family_id>[\d]+)/' . $this->rest_base,
+			'/' . $this->rest_base,


By defining the rest_base as font-families/(?P<font_family_id>[\d]+)/font-faces, we can use this rest_base directly for registering the routes.

creativecoder · 2024-01-18T20:07:06Z

lib/experimental/fonts/font-library/class-wp-rest-font-faces-controller.php

-		$this->parent_base      = $parent_post_type_obj->rest_base;
-		$this->namespace        = $parent_post_type_obj->rest_namespace;
-	}
+	protected $allow_batch = false;


Batch support is on by default. Here I'm turning it off for font families, because the batch endpoint doesn't really support file uploads.

creativecoder · 2024-01-18T20:07:45Z

lib/experimental/fonts/font-library/class-wp-rest-font-faces-controller.php

-	private $parent_base;
-
-	/**
-	 * Parent font family post type
-	 *
-	 * @since 6.5.0
-	 * @var string
-	 */
-	private $parent_post_type;
-
-	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;
-	}


Removing the constructor, as requested here: #57688 (comment)

creativecoder · 2024-01-18T20:09:25Z

lib/experimental/fonts/font-library/class-wp-rest-font-faces-controller.php

@@ -55,7 +33,7 @@ public function __construct() {
 	public function register_routes() {


Still overriding register_routes for font faces, as we don't want the WP_REST_Server::EDITABLE route that's created by default for updating existing font faces (which could get quite complex with file uploads)

creativecoder · 2024-01-18T20:25:49Z

lib/experimental/fonts/font-library/class-wp-rest-font-families-controller.php

 		}
-
-		return true;
+		return parent::get_context_param( $args );
 	}


This sets the context param default to edit, since we don't need a view context for fonts as admin only settings.

creativecoder · 2024-01-18T20:26:20Z

lib/experimental/fonts/font-library/class-wp-rest-font-families-controller.php

+			);
+		}
+
+		return parent::get_endpoint_args_for_item_schema( $method );


This overrides the default method for getting the allowed params when creating or editing a font face.

creativecoder · 2024-01-18T20:27:21Z

lib/experimental/fonts/font-library/font-library.php

 		'rest_base'                      => 'font-families',
 		'rest_controller_class'          => 'WP_REST_Font_Families_Controller',
-		'autosave_rest_controller_class' => 'WP_REST_Autosave_Font_Families_Controller',
-		'query_var'                      => false,
+		'autosave_rest_controller_class' => 'WP_REST_Autosave_Fonts_Controller',
 	);


Ordering the post type settings for wp_font_family so they match with wp_font_face.

creativecoder · 2024-01-18T20:28:25Z

lib/experimental/fonts/font-library/font-library.php

+			'show_in_rest'                   => true,
+			'rest_base'                      => 'font-families/(?P<font_family_id>[\d]+)/font-faces',
+			'rest_controller_class'          => 'WP_REST_Font_Faces_Controller',
+			'autosave_rest_controller_class' => 'WP_REST_Autosave_Fonts_Controller',


These are the new post registration settings here, using the built in plumbing for registering the routes for wp_font_face.

creativecoder · 2024-01-18T20:28:41Z

lib/experimental/fonts/font-library/font-library.php

@@ -242,5 +242,5 @@ function _wp_delete_font_face( $post_id, $post ) {
 			wp_delete_file( wp_get_font_dir()['path'] . '/' . $font_file );
 		}
 	}
-	add_action( 'before_delete_post', '_wp_delete_font_face', 10, 2 );
+	add_action( 'before_delete_post', '_wp_before_delete_font_face', 10, 2 );


Renaming all of these hooked functions for clarity.

jffng

Verified the following:

✅ Code changes address relevant feedback
✅ Smoke test font library — can still install and uninstall local and collection fonts
✅ Unit tests pass
✅ Creating and deleting a font face with invalid font family ID fails and error is thrown

…e branch (#57946)

* 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Jason Crist <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Matias Benedetto <[email protected]> Co-authored-by: Sarah Norris <[email protected]> * Fix unique key prop warning when opening modal * Add key props to FontsGrid children * 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 <[email protected]> * Add slug/id back to FontCollection * Change tabsFromCollections inline with Font Collections PR * Use child.key for key prop in FontsGrid * Update packages/edit-site/src/components/global-styles/font-library-modal/local-fonts.js Co-authored-by: Jonny Harris <[email protected]> * 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 <[email protected]> --------- Co-authored-by: Jeff Ong <[email protected]> * Font Library REST API endpoints: address initial feedback from feature branch (#57946) * Font Library: font collection refactor to use the new schema (#57884) * google fonts collection data provisional url * rename controller methods * fix get_items parameters * fix endpoint return * rafactor font collection class * fix tests for the refactored class * refactor font collections rest controller * update font collection tests * update the frontend to use the new endpoint data schema * format php * adding linter line ignore rul * replacing throwing an exception by calling doing_it_wrong * add translation marks Co-authored-by: Jeff Ong <[email protected]> * user ternary operator * correct translation formatting and comments * renaming function * renaming tests * improve url matching Co-authored-by: Grant Kinney <[email protected]> * return error without rest_ensure_response * fix contradictory if condition --------- Co-authored-by: Jeff Ong <[email protected]> Co-authored-by: Grant Kinney <[email protected]> * Remove old WP_REST_Autosave_Fonts_Controller class --------- Co-authored-by: Grant Kinney <[email protected]> Co-authored-by: Grant Kinney <[email protected]> Co-authored-by: Jeff Ong <[email protected]> Co-authored-by: Matias Benedetto <[email protected]> Co-authored-by: Jason Crist <[email protected]> Co-authored-by: Jonny Harris <[email protected]>

* 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) * Font Library: refactor client side install functions to work with revised API (#57844) * Cleanup/font library view error handling (#57926) * Font Faces endpoint: prevent creating font faces with duplicate settings (#57903) * Font Library: Update uninstall/delete on client side (#57932) * Font Library: address JS feedback in #57688 (#57961) * Font Library REST API endpoints: address initial feedback from feature branch (#57946) * Font Library: font collection refactor to use the new schema (#57884) * Fix font asset download when font faces are installed (#58021) * Font Families and Faces: disable autosaves using empty class (#58018) * Adds migration for legacy font family content (#58032) --------- Co-authored-by: Jeff Ong <[email protected]> Co-authored-by: Matias Benedetto <[email protected]> Co-authored-by: Jason Crist <[email protected]> Co-authored-by: Sarah Norris <[email protected]> Co-authored-by: Jonny Harris <[email protected]>

creativecoder added REST API Interaction Related to REST API [Type] Experimental Experimental feature or API. [Feature] Typography Font and typography-related issues and PRs labels Jan 18, 2024

creativecoder requested review from pbking, matiasbenedetto, mikachan and jffng January 18, 2024 04:38

creativecoder self-assigned this Jan 18, 2024

jffng reviewed Jan 18, 2024

View reviewed changes

creativecoder added 5 commits January 18, 2024 12:24

Addresses initial feedback on font controllers

80331bf

Remove font face controller constructor and a few other tweaks

34560e3

Completes implementing context param so it works correctly with embed…

4dbfe43

…ding font faces

Prevent creating or deleting font faces with invalid font families

d5743c9

Fix indenting and clarify comment

cc24936

creativecoder force-pushed the update/font-library-endpoints-initial-feedback branch from ffc8cc8 to cc24936 Compare January 18, 2024 18:25

creativecoder marked this pull request as ready for review January 18, 2024 18:28

creativecoder requested a review from spacedmonkey as a code owner January 18, 2024 18:28

creativecoder commented Jan 18, 2024

View reviewed changes

jffng approved these changes Jan 19, 2024

View reviewed changes

creativecoder merged commit 32689e1 into try/font-library-refactor Jan 19, 2024
57 checks passed

creativecoder deleted the update/font-library-endpoints-initial-feedback branch January 19, 2024 02:02

creativecoder mentioned this pull request Jan 19, 2024

Font Library Refactor #57688

Merged

creativecoder added a commit that referenced this pull request Jan 22, 2024

Font Library REST API endpoints: address initial feedback from featur…

2ed7a3b

…e branch (#57946)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Font Library REST API endpoints: address initial feedback from feature branch #57946

Font Library REST API endpoints: address initial feedback from feature branch #57946

creativecoder commented Jan 18, 2024 •

edited

Loading

github-actions bot commented Jan 18, 2024

jffng left a comment •

edited

Loading

creativecoder Jan 18, 2024

creativecoder commented Jan 18, 2024

creativecoder left a comment

creativecoder Jan 18, 2024

creativecoder Jan 18, 2024

creativecoder Jan 18, 2024

creativecoder Jan 18, 2024

creativecoder Jan 18, 2024

creativecoder Jan 18, 2024

creativecoder Jan 18, 2024

creativecoder Jan 18, 2024 •

edited

Loading

creativecoder Jan 18, 2024

creativecoder Jan 18, 2024

jffng left a comment

		@@ -55,7 +33,7 @@ public function __construct() {
		public function register_routes() {

Font Library REST API endpoints: address initial feedback from feature branch #57946

Font Library REST API endpoints: address initial feedback from feature branch #57946

Conversation

creativecoder commented Jan 18, 2024 • edited Loading

What?

Why?

Testing Instructions

github-actions bot commented Jan 18, 2024

jffng left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

creativecoder commented Jan 18, 2024

creativecoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

creativecoder Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jffng left a comment

Choose a reason for hiding this comment

creativecoder commented Jan 18, 2024 •

edited

Loading

jffng left a comment •

edited

Loading

creativecoder Jan 18, 2024 •

edited

Loading