Backport: theme.json webfonts handler - WP 6.0 stopgap #2622

hellofromtonya · 2022-04-22T15:59:45Z

This is a backport of the internal-only theme.json webfonts handler WordPress/gutenberg#40493.

Please note: This is a stopgap solution for WP 6.0. It will be replaced once the Webfonts API is ready for Core.

There's a lot of context that goes into why this implementation is being backported for 6.0 during beta. Please see full see Gutenberg's PR 40493 and its tracking issue.

The design is funky, but intentional. It's hiding all of the inner-workings to prevent backwards-compatibility breaks once the public Webfonts API is merged into Core (since that API is still in development).

Trac ticket: https://core.trac.wordpress.org/ticket/55567
Trac ticket: https://core.trac.wordpress.org/ticket/46370

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.

peterwilsoncc

I've added a few notes inline.

As it's still Sunday in the US, I may end up pushing the changes to your branch so you can review them Monday and revert anything you disagree with.

src/wp-includes/script-loader.php

tests/phpunit/data/themedir1/webfonts-theme/templates/index.html

tests/phpunit/tests/webfonts/wpThemeJsonWebfontsHandler.php

tests/phpunit/tests/rest-api/rest-attachments-controller.php

peterwilsoncc · 2022-04-25T02:57:27Z

src/wp-includes/script-loader.php

+	 * @param array $mime_types Array of mime types.
+	 * @return array Mime types with webfonts formats.
+	 */
+	$fn_add_mime_types = static function( $mime_types ) {


‼️ I was able to upload a woff2 file with unfiltered uploads disabled in my wp-config file:

define( 'DISALLOW_UNFILTERED_HTML', true ); define( 'ALLOW_UNFILTERED_UPLOADS', false );

I downloaded a copy of Roberto from Google fonts to do so.

Also, I am seeing different mime types in the downloads from Google fonts:

ttf: application/x-font-ttf

woff2: application/octet-stream

The latter may have been due to the way I downloaded it (curl .... > woff.woff2). It was only once I changed the mime type that I was able to upload it.

Hey @peterwilsoncc, see my notes here #2622 (comment). Does removing the web fonts from mime types upload_mimes filter resolve this issue for you?

In my testing, I'm able reproduce the ability to upload font files to Media library when setting the following constants in wp-config.php:

define( 'DISALLOW_UNFILTERED_HTML', false ); define( 'ALLOW_UNFILTERED_UPLOADS', true );

I was able to upload font files from TT2's assets folder.

After applying 515afda, uploading a font file is no longer allowed and the following error notification appears:

Also, I am seeing different mime types in the downloads from Google fonts:

ttf: application/x-font-ttf

woff2: application/octet-stream

I did a comparison on Mac using file --mime-type [filename]. Here are the results:

woff2 font files:

Using TT2's assets/fonts/ all of the .woff2 files => application/octet-stream

Via cUrl curl -L -o roboto.woff2 https://fonts.gstatic.com/s/roboto/v29/KFOmCnqEu92Fr1Mu5mxKKTU1Kvnz.woff2 => application/octet-stream

ttf font files:

Directly downloading Roboto zip from Google Fonts and then running file --mime-type Roboto-Black.ttf (et al) in terminal => font/sfnt

Using new TT2's assets/fonts/dm-sans/ => font/sfnt

Thought: Does this code need to specify the font file mime types for generating font-face and allowing the browser to request the file from the server?

I did a test. After removing the add_filter in the stopgap (no font file mime types added), it still works => meaning the font-face styles get rendered in <head> and the browser GETs the files from the server.

cc @aristath @peterwilsoncc

Thought: Does this code need to specify the font file mime types for generating font-face and allowing the browser to request the file from the server?

I did a test. After removing the add_filter in the stopgap (no font file mime types added), it still works => meaning the font-face styles get rendered in <head> and the browser GETs the files from the server.

In that case, I think it is not needed.

I'll confirm and push to your PR so any modifications to mime type settings can be considered over a longer period.

Seeing the same so have removed all the mime type related code in 66a940a

Yes, it's safe to remove that part of the code. It was part of the grander webfonts API plan to allow uploading font-files in the media library, and then be able to use them in the API. It's something for the future, and not necessary right now. 👍

peterwilsoncc · 2022-04-25T03:20:54Z

I've pushed six commits with the changes requests in my original review. I've since added a couple more comments regarding the test failure and ability to upload files.

I am considering whether it would be easier to:

add the font types to wp_get_mime_types() directly
disallow fonts for users with filtered access in get_allowed_mime_types()

It seems likely the fonts will be added to the former function once the API is introduced.

Restoring the original code to initialized each level of the multi-dimensional array to an empty array before attempting to deeply access. This level-by-level initialization makes Core future proof should more scalar or `null` to `array` deprecations occur for autovivification.

hellofromtonya · 2022-04-25T12:48:49Z

I am considering whether it would be easier to

add the font types to wp_get_mime_types() directly

I thought about that too. But if the API itself is still in experimental / development, my thinking was to keep it within this stoppgap function so that it can easily be turned off with a single remove_action(), letting the API continue on its path towards maturing as a Core API. In doing so, Gutenberg can turn off all of this stopgap code with one line of code, thus replacing it with the API.

disallow fonts for users with filtered access in get_allowed_mime_types()

Hey @peterwilsoncc, do you mean removing font types before they go through the 'upload_mimes' filter? If yes, how would the API in Gutenberg modify the font types, if needed, during its continued development?

Oh wait, I see what you mean @peterwilsoncc. Remove the font formats from the allowed "upload" mime types to prevent the issue you noted #2622 (comment). Also, web fonts currently are only supported within the theme itself and not through the Media library. Yes, that makes sense.

Commit 515afda achieves this.

@peterwilsoncc what do you think?

peterwilsoncc

In my view this ~~puppy~~ pull request is good to go.

I've sort one final clarification from Tonya and pushed some code myself so will leave it for the UK or US to commit prior to beta 3.

jffng · 2022-04-26T13:40:25Z

I tested the latest version of this PR against TT2's style variations #2440 and the fonts are loading as expected 👏 :

Kapture.2022-04-26.at.09.37.52.mp4

hellofromtonya · 2022-04-26T13:46:54Z

Thank you @peterwilsoncc @aristath @jffng! I pushed some unhappy path integration tests. Once CI is all green 🟢 , I'll get this backport PR committed.

hellofromtonya · 2022-04-26T14:47:39Z

Committed via https://core.trac.wordpress.org/changeset/53282.

Backport theme.json webfonts handler - stopgap.

7233f92

hellofromtonya mentioned this pull request Apr 22, 2022

Move webfonts handling to internal only (not accessible) - WP 6.0 Stopgap WordPress/gutenberg#40472

Closed

7 tasks

hellofromtonya force-pushed the add/themejson-webfonts-handler-stopgap branch 4 times, most recently from 580cd29 to dfc0f62 Compare April 22, 2022 17:12

hellofromtonya mentioned this pull request Apr 22, 2022

Turn off WP 6.0 stopgap handler to use Webfonts API. WordPress/gutenberg#40555

Merged

hellofromtonya force-pushed the add/themejson-webfonts-handler-stopgap branch from dfc0f62 to 9a00831 Compare April 22, 2022 17:24

hellofromtonya requested a review from peterwilsoncc April 22, 2022 17:24

Adds integration test.

096c86c

hellofromtonya force-pushed the add/themejson-webfonts-handler-stopgap branch from 9a00831 to 096c86c Compare April 22, 2022 17:32

peterwilsoncc requested changes Apr 25, 2022

View reviewed changes

peterwilsoncc reviewed Apr 25, 2022

View reviewed changes

tests/phpunit/tests/rest-api/rest-attachments-controller.php Outdated Show resolved Hide resolved

peterwilsoncc added 6 commits April 25, 2022 12:05

Document _wp_theme_json_webfonts_handler as private.

819f511

Remove duplication of inherint checks.

58839d5

Remove duplication of inherint set up.

02cd05a

Remove duplication of inherint checks.

4cbfea4

EOF line break.

6851f57

Document ticket numbers in tests.

2cd734b

peterwilsoncc reviewed Apr 25, 2022

View reviewed changes

hellofromtonya mentioned this pull request Apr 25, 2022

Webfonts: Add internal-only theme.json webfonts handler - WP 6.0 stopgap WordPress/gutenberg#40493

Closed

hellofromtonya added 2 commits April 25, 2022 07:10

Fix failing multisite test.

22b868c

hellofromtonya and others added 3 commits April 25, 2022 08:09

Removes web font mimes from allowed uploads.

515afda

Remove mime type additions for later.

66a940a

Keep functions completely unchanged.

28d76f8

peterwilsoncc approved these changes Apr 26, 2022

View reviewed changes

aristath approved these changes Apr 26, 2022

View reviewed changes

jffng mentioned this pull request Apr 26, 2022

Twenty Twenty-Two: add alternate style variations #2440

Closed

3 tasks

Add unhappy path integration tests.

2163f2b

Add new test theme to Tests_Theme_ThemeDir::test_theme_list.

30d5aaf

hellofromtonya closed this Apr 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport: theme.json webfonts handler - WP 6.0 stopgap #2622

Backport: theme.json webfonts handler - WP 6.0 stopgap #2622

hellofromtonya commented Apr 22, 2022 •

edited

Loading

peterwilsoncc left a comment

peterwilsoncc Apr 25, 2022

hellofromtonya Apr 25, 2022

hellofromtonya Apr 25, 2022

hellofromtonya Apr 25, 2022

hellofromtonya Apr 25, 2022 •

edited

Loading

peterwilsoncc Apr 26, 2022

peterwilsoncc Apr 26, 2022

aristath Apr 26, 2022

peterwilsoncc commented Apr 25, 2022

hellofromtonya commented Apr 25, 2022 •

edited

Loading

peterwilsoncc left a comment

jffng commented Apr 26, 2022

hellofromtonya commented Apr 26, 2022

hellofromtonya commented Apr 26, 2022

Backport: theme.json webfonts handler - WP 6.0 stopgap #2622

Backport: theme.json webfonts handler - WP 6.0 stopgap #2622

Conversation

hellofromtonya commented Apr 22, 2022 • edited Loading

peterwilsoncc left a comment

Choose a reason for hiding this comment

peterwilsoncc Apr 25, 2022

Choose a reason for hiding this comment

hellofromtonya Apr 25, 2022

Choose a reason for hiding this comment

hellofromtonya Apr 25, 2022

Choose a reason for hiding this comment

hellofromtonya Apr 25, 2022

Choose a reason for hiding this comment

hellofromtonya Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

peterwilsoncc Apr 26, 2022

Choose a reason for hiding this comment

peterwilsoncc Apr 26, 2022

Choose a reason for hiding this comment

aristath Apr 26, 2022

Choose a reason for hiding this comment

peterwilsoncc commented Apr 25, 2022

hellofromtonya commented Apr 25, 2022 • edited Loading

peterwilsoncc left a comment

Choose a reason for hiding this comment

jffng commented Apr 26, 2022

hellofromtonya commented Apr 26, 2022

hellofromtonya commented Apr 26, 2022

hellofromtonya commented Apr 22, 2022 •

edited

Loading

hellofromtonya Apr 25, 2022 •

edited

Loading

hellofromtonya commented Apr 25, 2022 •

edited

Loading