Font Library by matiasbenedetto · Pull Request #5285 · WordPress/wordpress-develop

matiasbenedetto · 2023-09-22T18:34:48Z

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

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

src/wp-includes/fonts/font-library/class-wp-rest-font-library-controller.php

This reverts commit d4bce59.

spacedmonkey · 2023-10-05T13:36:06Z

src/wp-includes/rest-api/endpoints/class-wp-rest-font-collections-controller.php

+					'callback'            => array( $this, 'get_font_collections' ),
+					'permission_callback' => array( $this, 'update_font_library_permissions_check' ),
+				),
+				'schema' => array( $this, 'get_items_schema' ),


There idea of schema, is the desciptions an item in the response in the request. The shape of the repsonse should match between items and item endpoints. Items should just be a array of item. That way you can reuse the same prepare_items_for_response. See this example.

wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-block-types-controller.php

Lines 133 to 157 in 23c811e

public function get_items( $request ) {

$data = array();

$block_types = $this->block_registry->get_all_registered();

// Retrieve the list of registered collection query parameters.

$registered = $this->get_collection_params();

$namespace = '';

if ( isset( $registered['namespace'] ) && ! empty( $request['namespace'] ) ) {

$namespace = $request['namespace'];

}

foreach ( $block_types as $slug => $obj ) {

if ( $namespace ) {

list ( $block_namespace ) = explode( '/', $obj->name );

if ( $namespace !== $block_namespace ) {

continue;

}

}

$block_type = $this->prepare_item_for_response( $obj, $request );

$data[] = $this->prepare_response_for_collection( $block_type );

}

return rest_ensure_response( $data );

}

spacedmonkey · 2023-10-05T13:41:16Z

src/wp-includes/rest-api/endpoints/class-wp-rest-font-collections-controller.php

+			$collections[] = $this->prepare_item_for_response( $collection->get_config(), null );
+		}
+
+		return new WP_REST_Response( $collections, 200 );


Suggested change

$collections[] = $this->prepare_item_for_response( $collection->get_config(), null );

}

return new WP_REST_Response( $collections, 200 );

$font_collection = $this->prepare_item_for_response( $collection->get_config(), null );

$collections[] = $this->prepare_response_for_collection( $font_collection );

}

return rest_ensure_response( $collections );

I see this is now included at:

https://github.com/WordPress/gutenberg/blob/066f8777b113ca8a71c79deb052d1deb23fe9860/lib/compat/wordpress-6.5/fonts/class-wp-rest-font-collections-controller.php#L102-L103

spacedmonkey · 2023-10-05T13:44:16Z

src/wp-includes/rest-api/endpoints/class-wp-rest-font-collections-controller.php

+					'context'     => array( 'view', 'edit', 'embed' ),
+				),
+				'data'        => array(
+					'description' => __( 'Data of the font collection.' ),


This seems like it is embedded font family object no? This should use links to link to font object.
https://developer.wordpress.org/rest-api/using-the-rest-api/linking-and-embedding/

spacedmonkey · 2023-10-05T13:46:27Z

@matiasbenedetto My review was requested. Please ensure before requesting my reviewing the following things are complete, before I review.

All unit tests are pass.
PHPCS coding standards pass.
All feedback has been actioned.

spacedmonkey · 2023-10-05T13:51:07Z

src/wp-includes/rest-api/endpoints/class-wp-rest-font-families-controller.php

+ *
+ * @since 6.4.0
+ */
+class WP_REST_Font_Families_Controller extends WP_REST_Controller {


@matiasbenedetto Sorry if I am missing this, but why is this a custom controller? Doesn't this map to the post contoller for font CPT?

spacedmonkey · 2023-10-05T13:55:29Z

src/wp-includes/fonts.php

+function wp_register_font_collection( $config ) {
+	return WP_Font_Library::register_font_collection( $config );
+}
+


how do a unregister a font collection.

That's not currently possible but that work is already started: WordPress/gutenberg#54701

There is now covered by

https://github.com/WordPress/gutenberg/blob/066f8777b113ca8a71c79deb052d1deb23fe9860/lib/compat/wordpress-6.5/fonts/fonts.php#L147-L149

spacedmonkey · 2023-10-05T13:58:18Z

src/wp-includes/rest-api/endpoints/class-wp-rest-font-families-controller.php

+	public function update_font_library_permissions_check() {
+		if ( ! current_user_can( 'edit_theme_options' ) ) {
+			return new WP_Error(
+				'rest_cannot_update_font_library',
+				__( 'Sorry, you are not allowed to update the Font Library on this site.' ),
+				array(
+					'status' => rest_authorization_required_code(),
+				)
+			);
+		}
+		return true;
+	}


Can we not use a meta capabilities for the font post type instead of just using edit_theme_options? Example
https://core.trac.wordpress.org/ticket/54516

This received detailed discussion over on WordPress/gutenberg#55280.

hellofromtonya · 2023-10-05T14:26:32Z

Sharing @spacedmonkey observations from Make/Core slack in the #core channel:

I am currently reviewing the font apis PR . I must say, I am very worried about the PR in it's current state. The code simply doesn't follow the WP core code style and doesn't feel WordPress. I have a number of problems with it, including.

Limited developer API. We need function like wp_insert_font / wp_create_font etc.

Lack of filter or actions.

No way to unregister font collections.

Capabilities. Creating new fonts should have capabilies and not simply map to edit_theme_options

Confusing API structure. Collection should have embedded font objects

What happens to fonts when collections are unregistered.

If fonts are stored as post object, can I query to get all fonts from a collection.

Are fonts deleted when the user is deleted.

No way to filter where a font is stored. ( #hosting channel for detail ).

With time very limited in this release, it feels like actioning the above, feel like it is going too hard to achieve in this release.

I think this feature needs some more time to bake.

Some links to help:

No way to filter where a font is stored.

See WordPress/gutenberg#54697 to add WP_FONTS_DIR constant to unblock web hosts.

No way to unregister font collections.

WordPress/gutenberg#54697

…est_prepare_item.

matiasbenedetto · 2023-10-11T15:47:36Z

Closing this PR due to the inclusion of this feature was punted to 6.5:
https://wordpress.slack.com/archives/C02RQBWTW/p1696525101347939

matiasbenedetto · 2023-10-11T17:12:19Z

Tracking the required changes to make Font Library ready for 6.5 in this Gutenberg issue: WordPress/gutenberg#54169

matiasbenedetto added 28 commits September 22, 2023 15:14

add main backend files

9a00814

add comment about CDN link

6f9aefc

load main files

d78d99b

add routes from font library to routes test

1d74144

adding PHP unit tests for font library

42eeb48

fix register post type for font family post type

9e06a93

move the font library functions

b323b67

avoid loading file deleted

715eea9

add wp_register_default_font_collection function

e54426a

load font-library functions file

1078767

register default font collection with init hook

699f45b

fix rest-schema test

a6dca33

Merge branch 'trunk' into port/font-library

9ba9187

settings the right 'supports' values for wp_font_family post type

5084180

set hierarchical as false for wp_font_family post type

46a1e9f

fix url in test

7e666e6

fix set_upload_dir unit test

e6cce43

fix unit test by using solvable host name

673c042

avoid using gutenberg class, use core class instead

72bbec5

add the font assets for the tests

1d8de84

Remove font files created after tests run

d769cc5

removing unwanted echo line

c4a31d2

Merge branch 'trunk' into port/font-library

57f05a1

add the custom post type routes to the routes test

8933cb8

Removing class re-declaration check

9aeb0ba

format php

1725825

updating wp-api-generated.js schema

d4bce59

Merge branch 'trunk' into port/font-library

89e888f

spacedmonkey reviewed Sep 25, 2023

View reviewed changes

src/wp-includes/fonts/font-library/class-wp-rest-font-library-controller.php Outdated Show resolved Hide resolved

Revert "updating wp-api-generated.js schema"

ab70608

This reverts commit d4bce59.

matiasbenedetto requested a review from mukeshpanchal27 October 5, 2023 12:07

matiasbenedetto and others added 4 commits October 5, 2023 09:28

add missing 'since 6.4' comment

e28134f

Get rid of WP_REST_Font_Library_Controller_UnitTestCase.

b2a96fc

Fix parent class name.

640159e

fix typo in comment

d44da25

spacedmonkey reviewed Oct 5, 2023

View reviewed changes

anton-vlasenko added 8 commits October 5, 2023 18:46

Rename controller methods.

603ee9e

Fix 404 errors.

7382406

Move the tests to Tests_Fonts_WpRestFontCollectionsController.

d9ffea9

Fix the tests.

9edfb39

Improve PHPDOC blocks.

c4de43f

Fix @return type.

9deeb4a

Implement the test for Tests_Fonts_WpRestFontCollectionsController::t…

6c9e856

…est_prepare_item.

Fix the test.

89cd000

matiasbenedetto closed this Oct 11, 2023

matiasbenedetto mentioned this pull request Oct 11, 2023

Font Library: refactor REST API WordPress/gutenberg#55278

Closed

pbking mentioned this pull request Oct 18, 2023

Fix/55278 standardize font library rest api WordPress/gutenberg#55454

Closed

nefeline mentioned this pull request Dec 12, 2023

[CYS on Core] Ensure the fonts selected in the Assembler are displayed on the frontend woocommerce/woocommerce#41924

Closed

swissspidy mentioned this pull request Feb 5, 2024

Font Library: Add post types and low level APIs #6027

Closed

getdave mentioned this pull request Feb 5, 2024

Font Library Address Outstanding Feedback from WP 6.4 review WordPress/gutenberg#58675

Merged

hellofromtonya mentioned this pull request Feb 27, 2024

Reconsider placing uploaded fonts within the uploads directory WordPress/gutenberg#59417

Closed

	public function get_items( $request ) {
	$data = array();
	$block_types = $this->block_registry->get_all_registered();

	// Retrieve the list of registered collection query parameters.
	$registered = $this->get_collection_params();
	$namespace = '';
	if ( isset( $registered['namespace'] ) && ! empty( $request['namespace'] ) ) {
	$namespace = $request['namespace'];
	}

	foreach ( $block_types as $slug => $obj ) {
	if ( $namespace ) {
	list ( $block_namespace ) = explode( '/', $obj->name );

	if ( $namespace !== $block_namespace ) {
	continue;
	}
	}
	$block_type = $this->prepare_item_for_response( $obj, $request );
	$data[] = $this->prepare_response_for_collection( $block_type );
	}

	return rest_ensure_response( $data );
	}

-			$collections[] = $this->prepare_item_for_response( $collection->get_config(), null );
-		}
-		return new WP_REST_Response( $collections, 200 );
+			$font_collection = $this->prepare_item_for_response( $collection->get_config(), null );
+			$collections[]     = $this->prepare_response_for_collection( $font_collection );
+		}
+		return rest_ensure_response( $collections );

Conversation

matiasbenedetto commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacedmonkey commented Oct 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellofromtonya commented Oct 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matiasbenedetto commented Oct 11, 2023

Uh oh!

matiasbenedetto commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

matiasbenedetto commented Sep 22, 2023 •

edited

Loading

hellofromtonya commented Oct 5, 2023 •

edited

Loading