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 and Faces: disable autosaves using empty class #58018

Merged

Conversation

creativecoder
Copy link
Contributor

What?

  • Revises wp_font_family and wp_font_face post type registration to use an empty class to disable autosave routes
  • Adds tests that autosave routes are not registered

Why?

Autosaves are not appropriate for fonts because they are settings rather than content. Having your font settings automatically saved while editing them could be unpleasantly surprising!

How?

Autosaves are disabled by providing a valid, empty class that doesn't extend WP_REST_Controller when registering the post type.

Testing Instructions

  • Make a GET request to wp/v2
  • See that the routes do not include autosave endpoints for font families or faces (e.g. /wp/v2/font-families/(?P<id>[\d]+)/autosaves is not present)

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

github-actions bot commented Jan 19, 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/font-library.php
❔ lib/load.php
❔ phpunit/tests/fonts/font-library/wpRestFontFacesController.php
❔ phpunit/tests/fonts/font-library/wpRestFontFamiliesController.php

Copy link
Contributor

@matiasbenedetto matiasbenedetto left a comment

Choose a reason for hiding this comment

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

This works as advertised, though I didn't see this technique used on the WordPress core codebase. Still, it is a shorter solution than the current one, which consists of creating an empty class, so I approve the PR.

@creativecoder
Copy link
Contributor Author

I didn't see this technique used on the WordPress core codebase

The closest examples I see in Core are

  • wp_global_styles, which declares show_in_rest as false, so doesn't have automatic autosave routes added
  • attachment, which is manually excluded in WP_Post_Type::get_autosave_rest_controller

So perhaps our font post types could be excluded like attachments, as well. But until then, I think the solution here makes sense.

@creativecoder creativecoder force-pushed the update/font-autosaves-controller branch from be2b096 to d183e05 Compare January 19, 2024 20:36
@creativecoder creativecoder force-pushed the update/font-autosaves-controller branch from 08bbe65 to a1f37e4 Compare January 19, 2024 20:49
@creativecoder
Copy link
Contributor Author

It turns out that autosave_rest_controller_class was introduced only in WP 6.4, so the tests added here are failing with WP 6.3, since there's no way to disable the autosave routes with that version.

I'm skipping those tests, for now so that CI will pass, but added a core-merge comment to enable them, and made it clear they can be enabled in Gutenberg once WP 6.5 is released.

@creativecoder creativecoder merged commit 2a65de7 into try/font-library-refactor Jan 19, 2024
55 checks passed
@creativecoder creativecoder deleted the update/font-autosaves-controller branch January 19, 2024 22:16
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>
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.

2 participants