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 Families REST API endpoint: ensure unique font family slugs #57861

Conversation

creativecoder
Copy link
Contributor

@creativecoder creativecoder commented Jan 15, 2024

What?

Better slug handling for wp/v2/font-families and wp/v2/font-families/<id> endpoints

  • Ensure a unique slug when creating a new font family
  • Disallow changing the slug for existing font families
  • Allow querying for a font family by slug using wp/v2/font-families?slug=
  • Removed duplicate data storage so that settings stored outside of post_content are removed before JSON encoding (e.g. font family slug is stored as post_name, so the slug is not duplicated in the json encoded post_content)

Why?

Slugs are a secondary unique identifier for font families, so should be unique. Part of #55278.

Testing Instructions

  • Check that you cannot creating a font family (POST wp/v2/font-families) providing font_family_settings[slug] that is already in use.
  • Check that you cannot update (POST wp/v2/font-families/<id>) the slug for an existing font family
  • Check that you can query for font families by slug (wp/v2/font-families?slug=)

@creativecoder 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 15, 2024
@creativecoder creativecoder self-assigned this Jan 15, 2024
Copy link

github-actions bot commented Jan 15, 2024

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

@@ -331,7 +356,7 @@ public function test_create_item_with_default_preview( $settings ) {
public function data_create_item_with_default_preview() {
$default_settings = array(
'name' => 'Open Sans',
'slug' => 'open-sans',
'slug' => 'create_item_with_default_preview',
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking if the slug should be changed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The slugs for test fixtures just need to be something unique, since that constraint is now added for font families. I'll make this less confusing by using 'open-sans-2', instead.

Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

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

✅ Cannot create a font family (POST wp/v2/font-families) providing font_family_settings[slug] that is already in use.
✅ Can query for font families by slug (wp/v2/font-families?slug=)
🔲 (did not test this yet) Cannot update (POST wp/v2/font-families/<id>) the slug for an existing font family.

For context, I was testing this against the client side PR and added a commit to handle the case of an existing / already installed font family: 04c4225

Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

Test report:

  • Cannot create a font family (POST wp/v2/font-families) providing font_family_settings[slug] that is already in use ✅
  • Cannot update (POST wp/v2/font-families/<id>) the slug for an existing font family ✅
  • Can query for font families by slug (wp/v2/font-families?slug=) ✅

+1 to @jffng's inline comment re the slug in the test, but otherwise this LGTM.

@creativecoder creativecoder merged commit 7c82cb9 into try/font-library-refactor Jan 16, 2024
55 checks passed
@creativecoder creativecoder deleted the add/font-family-slug-check-before-creation branch January 16, 2024 16:45
mikachan added a commit that referenced this pull request Jan 23, 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>

* 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 <jcrist@pbking.com>

* 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 <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>

* 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 <jonger4@gmail.com>

* user ternary operator

* correct translation formatting and comments

* renaming function

* renaming tests

* improve url matching

Co-authored-by: Grant Kinney <creativecoder@users.noreply.github.com>

* return error without rest_ensure_response

* fix contradictory if condition

---------

Co-authored-by: Jeff Ong <jonger4@gmail.com>
Co-authored-by: Grant Kinney <creativecoder@users.noreply.github.com>

* Remove old WP_REST_Autosave_Fonts_Controller class

---------

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>
creativecoder added a commit that referenced this pull request Jan 23, 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)

* 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 <jonger4@gmail.com>
Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com>
Co-authored-by: Jason Crist <jcrist@pbking.com>
Co-authored-by: Sarah Norris <sarah@sekai.co.uk>
Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Typography Font and typography-related issues and PRs REST API Interaction Related to REST API [Type] Experimental Experimental feature or API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants