Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 19 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 3 years ago.
Patch for /wp-includes/script-loader.php

Download all attachments as: .zip

Change History (21)

#1 @sabernhardt
3 years ago

  • Description modified (diff)

@domainsupport
3 years ago

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

#2 @domainsupport
3 years 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
2 years 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.


2 years ago

#6 @audrasjb
2 years ago

  • Milestone changed from Awaiting Review to 6.2

Moving for 6.2 consideration.

#7 @audrasjb
2 years ago

  • Keywords needs-testing added

#8 @audrasjb
2 years 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
2 years 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 2 years ago by domainsupport (previous) (diff)

#10 in reply to: ↑ 9 ; follow-up: @azaozz
2 years 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 2 years ago by azaozz (previous) (diff)

#11 in reply to: ↑ 10 ; follow-up: @domainsupport
2 years 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
2 years 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 2 years ago by azaozz (previous) (diff)

#13 follow-up: @domainsupport
2 years 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.


2 years ago

#15 @mukesh27
2 years 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
2 years 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
2 years 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

#18 follow-up: @domainsupport
19 months ago

Hi @hellofromTonya, bearing in mind the status of the Fonts API and the stopgap code is still currently being used ... can this bug please be re-appraised and possibly fixed?

I would be happy to create a patch that uses a function to replace array_unique() for this multi-dimensional array?

Oliver

#19 in reply to: ↑ 18 @hellofromTonya
19 months ago

Replying to domainsupport:

Hi @hellofromTonya, bearing in mind the status of the Fonts API and the stopgap code is still currently being used ... can this bug please be re-appraised and possibly fixed?

I would be happy to create a patch that uses a function to replace array_unique() for this multi-dimensional array?

Hello @domainsupport,

I originally closed the ticket because within Core at this time, injecting fonts is doing it wrong:

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.

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

The direction of fonts has changed since last we spoke. Give me a moment to share.

Planned in 6.4 is a new font management system which will include:

  • Font Library: the UI/UX for managing fonts including previewing, uploading, activating, and deactivating. Google Fonts (served locally in wp-content/uploads/fonts/) is bundled into it.
  • Font Face: the server-side generator and printer of the @font-face styles for the fonts the user activates in the Font Library.

This new system is being developed and tested in the Gutenberg repo. I invite you to follow along and contribute:

#20 @domainsupport
19 months ago

Thank you. Will look into all of that ASAP.

Note: See TracTickets for help on using tickets.