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

Critical error when removing fonts added using old naming convention in theme.json #599

Open
nicholasingham opened this issue Apr 26, 2024 · 5 comments · May be fixed by #645
Open

Critical error when removing fonts added using old naming convention in theme.json #599

nicholasingham opened this issue Apr 26, 2024 · 5 comments · May be fixed by #645

Comments

@nicholasingham
Copy link

When using the Save Changes option with Save Fonts ticked, we are getting the following error

PHP Fatal error:  Uncaught TypeError: basename(): Argument #1 ($path) must be of type string, array given in ... wp-content\plugins\create-block-theme\admin\create-theme\theme-fonts.php:117

We think this error occurs when create block theme is trying to remove fonts that were created in theme.json with the old naming convention and where the src is stored in theme.json as an array e.g.

	"fontFace": [
		{
			"fontFamily": "Ysabeau Office",
			"fontStyle": "normal",
			"fontWeight": "400",
			"src": [
				"file:./assets/fonts/ysabeau-office_normal_400.ttf"
			]
		},

We do not get the problem, when trying to remove fonts with the new naming convention where the src is stored as a string e.g.

"src": "file:./assets/fonts/S6uyw4BMUTPHvxw6XweuBCY.woff2"

We have managed to work around this by temporarily changing the line of code

$font_filename = basename( $font_face['src']);

to

$font_filename = basename( $font_face['src'][0]);

and removing fonts created the old way and reinstalling them the new way.

@nicholasingham nicholasingham changed the title Critical error when removing fonts added using old naming convention on theme.json Critical error when removing fonts added using old naming convention in theme.json Apr 26, 2024
@nicholasingham
Copy link
Author

I've done some more testing.

I think there may be two issues

(1) for fonts added via the new Gutenberg font manager system, the plugin is incorrectly adding the font src to theme.json as a string rather than as an array

(2) the plugin throws a fatal error when trying to remove a font where the src is not stored as a string in theme.json

We can reproduce the problems (using a fresh install of Wordpress 6.5.2 and Create Block Theme 2.1.2 with the 2024 theme activated) by doing the following

To reproduce the first issue

  • in the Site Editor, select Styles > Typography > Manage Fonts

  • click Install Fonts, allow access to Google Fonts and install a Google Font

  • at this point the new font shows in Installed Fonts but not in Theme Fonts

  • then navigate to Create Block Theme > Save Changes to Theme and click Save Changes

  • go back to Styles > Typography > Manage Fonts

  • the new font now shows in both Installed Fonts and Theme Fonts

In theme.json the new font has been added as follows

{
	"fontFace": [
		{
			"fontFamily": "ABeeZee",
			"fontStyle": "normal",
			"fontWeight": "400",
			"src": "file:./assets/fonts/esDR31xSG-6AGleN6teukbcHCpE.woff2"
		}
	],
	"fontFamily": "ABeeZee, sans-serif",
	"name": "ABeeZee",
	"slug": "abeezee"
}

To reproduce the second issue

  • in the Site Editor, select Styles > Typography > Manage Fonts

  • click on an pre-existing font e.g. Inter in Theme Fonts

  • in the next screen uncheck the box next to the Theme, in this case "Inter 300 900" and click Update to remove the font from the theme

  • then navigate to Create Block Theme > Save Changes to Theme and click Save Changes

  • at this point we get the "There has been a critical error ..." error message described above.

The bad news (arising from the first issue) is that users may already have created theme.json files which contain some incorrect src values so it might make sense to rewrite the code at line 177 of theme-fonts.php to handle either strings or arrays.

Hope this helps.

@nicholasingham
Copy link
Author

In case it's useful to anyone, we've found the following workaround seems to work.

At line 62 of theme-fonts.php, we've changed

$font_face['src'] = 'file:./assets/fonts/' . $font_filename;

to

$font_face['src'] = array('file:./assets/fonts/' . $font_filename);

At line 117 of theme-fonts.php, we've changed

$font_filename = basename( $font_face['src'] );

to

if(is_array($font_face['src'])){
	$font_filename = basename( $font_face['src'][0] );
} else{
	$font_filename = basename( $font_face['src'] );
}

@creativecoder
Copy link
Contributor

The font face src property can be an array or string, both are specified in the theme.json schema, so create-block-theme needs to be updated to support either.

@nicholasingham
Copy link
Author

That's very interesting. I was relying on https://developer.wordpress.org/themes/global-settings-and-styles/settings/typography/ which says that src is "an array of font file URLs where the font files are located".

As almost all WP standard theme.json files store src as an array, it seems to me that it might be better if create-block-theme reverts to writing src as an array as the default setting (as I think it used to do up until v1.3.8) but can read src as either an array or string.

@megane9988
Copy link

megane9988 commented May 20, 2024

+1

I encountered the same issue when I created the child theme using CTP and changed the font setting.

video2286005529.mp4

And I also encountered the color palette issue. The parent color palette was lost when I overwritten the child after I added a custom color.

PHP error

[20-May-2024 16:18:01 UTC]
PHP Fatal error:  Uncaught TypeError: array_filter(): Argument #1 ($array) must be of type array, null given in /wp-content/plugins/create-block-theme/admin/create-theme/theme-fonts.php:111
Stack trace:
#0 /wp-content/plugins/create-block-theme/admin/create-theme/theme-fonts.php(111): array_filter(NULL, Object(Closure))
#1 /wp-content/plugins/create-block-theme/admin/create-theme/theme-fonts.php(11): CBT_Theme_Fonts::remove_deactivated_fonts_from_theme()
#2 /wp-content/plugins/create-block-theme/includes/class-create-block-theme-api.php(451): CBT_Theme_Fonts::persist_font_settings()
#3 /wp-includes/rest-api/class-wp-rest-server.php(1230): CBT_Theme_API->rest_save_theme(Object(WP_REST_Request))
#4 /wp-includes/rest-api/class-wp-rest-server.php(1063): WP_REST_Server->respond_to_request(Object(WP_REST_Request), '/create-block-t...', Array, NULL)
#5 /wp-includes/rest-api/class-wp-rest-server.php(439): WP_REST_Server->dispatch(Object(WP_REST_Request))
#6 /wp-includes/rest-api.php(428): WP_REST_Server->serve_request('/create-block-t...')
#7 /wp-includes/class-wp-hook.php(324): rest_api_loaded(Object(WP))
#8 /wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters('', Array)
#9 /wp-includes/plugin.php(565): WP_Hook->do_action(Array)
#10 /wp-includes/class-wp.php(418): do_action_ref_array('parse_request', Array)
#11 /wp-includes/class-wp.php(813): WP->parse_request('')
#12 /wp-includes/functions.php(1336): WP->main('')
#13 /wp-blog-header.php(16): wp()
#14 /index.php(17): require('/Users/9988mega...')
#15 {main}
  thrown in /wp-content/plugins/create-block-theme/admin/create-theme/theme-fonts.php on line 111

PHP Fatal error:  Uncaught TypeError: basename(): Argument #1 ($path) must be of type string, array given in /wp-content/plugins/create-block-theme/admin/create-theme/theme-fonts.php:117
Stack trace:
#0 /wp-content/plugins/create-block-theme/admin/create-theme/theme-fonts.php(117): basename(Array)
#1 /wp-content/plugins/create-block-theme/admin/create-theme/theme-fonts.php(11): CBT_Theme_Fonts::remove_deactivated_fonts_from_theme()
#2 /wp-content/plugins/create-block-theme/includes/class-create-block-theme-api.php(451): CBT_Theme_Fonts::persist_font_settings()
#3 /wp-includes/rest-api/class-wp-rest-server.php(1230): CBT_Theme_API->rest_save_theme(Object(WP_REST_Request))
#4 /wp-includes/rest-api/class-wp-rest-server.php(1063): WP_REST_Server->respond_to_request(Object(WP_REST_Request), '/create-block-t...', Array, NULL)
#5 /wp-includes/rest-api/class-wp-rest-server.php(439): WP_REST_Server->dispatch(Object(WP_REST_Request))
#6 /wp-includes/rest-api.php(428): WP_REST_Server->serve_request('/create-block-t...')
#7 /wp-includes/class-wp-hook.php(324): rest_api_loaded(Object(WP))
#8 /wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters('', Array)
#9 /wp-includes/plugin.php(565): WP_Hook->do_action(Array)
#10 /wp-includes/class-wp.php(418): do_action_ref_array('parse_request', Array)
#11 /wp-includes/class-wp.php(813): WP->parse_request('')
#12 /wp-includes/functions.php(1336): WP->main('')
#13 /wp-blog-header.php(16): wp()
#14 /index.php(17): require('/Users/9988mega...')
#15 {main}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

3 participants