Make WordPress Core

Opened 11 months ago

Closed 3 months ago

Last modified 3 months ago

#56078 closed defect (bug) (wontfix)

PHP Warning when adding fonts to fontFamilies in wp_global_styles post

Reported by: domainsupport's profile domainsupport Owned by:
Milestone: Priority: normal
Severity: normal Version: 6.0
Component: Editor Keywords:
Focuses: administration, template Cc:

Description (last modified by sabernhardt)

I am injecting additional Google Fonts to the Full Site Editor global styles by adding elements to ['settings']['typography']['fontFamilies'] of the wp_global_styles post type in the database (rather than users having to modify the theme.json or having to create a child theme).

Which works! :)

However, it is presenting the following error ...

[27-Jun-2022 14:08:45 UTC] PHP Warning: Array to string conversion in /wp-includes/script-loader.php on line 3100

I suspect this is because on line 3100 of script-loader.php ...

<?php
                                $settings['typography']['fontFamilies']          = array_unique( $settings['typography']['fontFamilies'] );

... is being used to make sure the font families are not duplicated ... however, you can only use array_unique with an array of string values, but "fontFamilies" is an array of arrays. I believe this line should be changed to ...

<?php
                                $settings['typography']['fontFamilies']          = array_unique( $settings['typography']['fontFamilies'], SORT_REGULAR);

Please confirm and I'll create a patch file if you like.

Thanks,

Oliver

Attachments (1)

56078.diff (739 bytes) - added by domainsupport 10 months ago.
Patch for /wp-includes/script-loader.php

Download all attachments as: .zip

Change History (18)

#1 @sabernhardt
11 months ago

  • Description modified (diff)

@domainsupport
10 months ago

Patch for /wp-includes/script-loader.php

#2 @domainsupport
10 months ago

  • Keywords has-patch added

I've been using the proposed fix for over a month and it works for me here. I've added as a patch to this ticket. Hope that's OK?

Oliver

#4 @domainsupport
7 months ago

Please note that v6.1 still has this bug and the fix now needs to be applied to line 3229.

Oliver

This ticket was mentioned in Slack in #core by domainsupport. View the logs.


7 months ago

#6 @audrasjb
7 months ago

  • Milestone changed from Awaiting Review to 6.2

Moving for 6.2 consideration.

#7 @audrasjb
7 months ago

  • Keywords needs-testing added

#8 @audrasjb
7 months ago

@domainsupport could you please share a snippet of your implementation of additional Google Fonts, to help testing the patch?

Thank you!

#9 follow-up: @domainsupport
7 months ago

Sure ... so here's the post_content of a wp_global_styles post for twentytwentytwo theme which successfully adds "La Belle Aurore" and "Open Sans" to the Global Styles font family drop down ...

{"version":2,"isGlobalStylesUserThemeJSON":true,"settings":{"typography":{"fontFamilies":[{"fontFamily":"\"La Belle Aurore\"","name":"La Belle Aurore","slug":"la-belle-aurore"},{"fontFamily":"\"Open Sans\"","name":"Open Sans","slug":"open-sans"},{"fontFamily":"\"Source Serif Pro\", serif","name":"Source Serif Pro","slug":"source-serif-pro","fontFace":[{"fontFamily":"Source Serif Pro","fontWeight":"200 900","fontStyle":"normal","fontStretch":"normal","src":["file:.\/assets\/fonts\/source-serif-pro\/SourceSerif4Variable-Roman.ttf.woff2"]},{"fontFamily":"Source Serif Pro","fontWeight":"200 900","fontStyle":"italic","fontStretch":"normal","src":["file:.\/assets\/fonts\/source-serif-pro\/SourceSerif4Variable-Italic.ttf.woff2"]}]}]}}}

As soon as you have this in the database, the PHP notices will start.

Notice that default theme fonts are also in there.

Oliver

Last edited 7 months ago by domainsupport (previous) (diff)

#10 in reply to: ↑ 9 ; follow-up: @azaozz
7 months ago

Replying to domainsupport:

Sure ... so here's the post_content of a...

Thanks, this is helpful.

I think what @audrasjb meant was to show an example of the code that adds these extra fonts.

injecting additional Google Fonts to the Full Site Editor global styles by adding elements to ['settings']['typography']['fontFamilies'] of the wp_global_styles post type in the database

Hmmm, not sure if this is supported/expected/documented way of adding more fonts? :)

you can only use array_unique with an array of string values

Right, array_unique() cannot be used for multi-dimensional arrays. There's an example of a function in the PHP manual that may be helpful here: https://www.php.net/manual/en/function.array-unique.php#116302.

Last edited 7 months ago by azaozz (previous) (diff)

#11 in reply to: ↑ 10 ; follow-up: @domainsupport
7 months ago

Replying to azaozz:

I think what @audrasjb meant was to show an example of the code that adds these extra fonts.

Oh right, sure. OK, well the easiest way to demonstrate that is to install Template Editor plugin https://wordpress.org/plugins/template-editor/ then go to "Dashboard - Appearance - Theme Options", choose a font (and style) and "Save Options".

The code to save the fonts can be found here https://plugins.trac.wordpress.org/browser/template-editor/trunk/template-editor.php#L1396

injecting additional Google Fonts to the Full Site Editor global styles by adding elements to ['settings']['typography']['fontFamilies'] of the wp_global_styles post type in the database

Hmmm, not sure if this is supported/expected/documented way of adding more fonts? :)

Haha :) Sure, it's not documented ... but the way that script-loader.php loads the fonts from those in the wp_global_styles post and then merges them with those from theme.json suggests that it is expected (even if array_unique() isn't used correctly, hence the error).

you can only use array_unique with an array of string values

Right, array_unique() cannot be used for multi-dimensional arrays. There's an example of a function in the PHP manual that may be helpful here: https://www.php.net/manual/en/function.array-unique.php#116302.

Yes, I've used something similar in the past ... are you saying you'd like me to amend the patch accordingly rather than just adding SORT_REGULAR which prevents the PHP notification?

Oliver

#12 in reply to: ↑ 11 @azaozz
7 months ago

Replying to domainsupport:

the easiest way to demonstrate that is to install Template Editor plugin

Thanks, this is really helpful.

the way that script-loader.php loads the fonts from those in the wp_global_styles post and then merges them with those from theme.json suggests that it is expected

I tried to reproduce this without using the Template Editor plugin, but seems it's not possible. Assuming there are very few cases, if any, where this bug is triggered. That's not surprising as _wp_theme_json_webfonts_handler() is clearly marked as a temporary solution: "A future public Webfonts API will replace this stopgap code.".

At the same time the use of array_unique() is still a bug and should probably be fixed, even in this temporary code.

are you saying you'd like me to amend the patch accordingly rather than just adding SORT_REGULAR which prevents the PHP notification?

The intention here is to make sure no duplicate font families remain in the settings after merging them with the fonts from theme.json. As far as I see fontFamily is the identification, so probably best to ensure there are no duplicates of it even if name and slug are not the same. Perhaps the function that looks in the arrays and compares sub-array elements would work best.

On the other hand this is a temporary, stopgap code that is going to be removed soon (I hope), so maybe adding SORT_REGULAR would be enough. Going to defer to @hellofromTonya here for a decision.

Last edited 7 months ago by azaozz (previous) (diff)

#13 follow-up: @domainsupport
7 months ago

No wonder it was so difficult to find a way to inject fonts into the Global Styles interface! I've just read this on https://make.wordpress.org/core/2022/04/22/status-of-webfonts-api-for-wordpress-6-0-inclusion/ ...

This led to the decision to pursue a pared down version that only supports declaring fonts within a theme.json file, that is only available to theme authors, rather than to the wider extender base (i.e., plugin authors). To ensure this approach, the core of the API was put into a private theme.json handler preventing plugins from registering or enqueuing, bypassing the current performance and robustness concerns.

So ... my bad for finding this loop-hole way to do precisely what is quite clearly not meant to be done but like you say, it is a bug nonetheless :)

If it were up to me I would vote for custom multidimensional array unique function unless the Webfonts API is imminent ... ?

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


4 months ago

#15 @mukesh27
4 months ago

This ticket was discussed during the recent bug scrub. It looks like it's unlikely that work will be done on this during the 6.2 cycle.

@azaozz @audrasjb Is this still possible to land in 6.2, or should it be moved to Future Release for now?

#16 in reply to: ↑ 13 @hellofromTonya
3 months ago

  • Keywords has-patch needs-testing removed
  • Milestone 6.2 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Replying to domainsupport:

No wonder it was so difficult to find a way to inject fonts into the Global Styles interface! I've just read this on https://make.wordpress.org/core/2022/04/22/status-of-webfonts-api-for-wordpress-6-0-inclusion/ ...

This led to the decision to pursue a pared down version that only supports declaring fonts within a theme.json file, that is only available to theme authors, rather than to the wider extender base (i.e., plugin authors). To ensure this approach, the core of the API was put into a private theme.json handler preventing plugins from registering or enqueuing, bypassing the current performance and robustness concerns.

So ... my bad for finding this loop-hole way to do precisely what is quite clearly not meant to be done but like you say, it is a bug nonetheless :)

If it were up to me I would vote for custom multidimensional array unique function unless the Webfonts API is imminent ... ?

I'm hesitant to change the current stopgap code in Core. Why?

  • It's temporary.
  • Injecting fonts is doing it wrong at this time, as the stopgap does not support nor was it designed for any fonts other than those in the active theme's theme.json file.

Registering fonts is supported in the Fonts API being tested and refined in Gutenberg. This API will be introduced into Core in 6.3 which will remove this stopgap code.

@domainsupport I invite you test and help with the Fonts API https://github.com/WordPress/gutenberg/issues/41479.

I'm closing this ticket as wontfix since the injecting fonts is not supported nor intended.

#17 @domainsupport
3 months ago

Thank you @hellofromTonya, it is regrettable that a bug in core that's relatively simple to fix will remain for at least the next 6 months but I take onboard your reasoning and will gladly test the Fonts API in due course which sounds fantastic and something that's been required for years!

Oliver

Note: See TracTickets for help on using tickets.