Fix fatal error in WP_Fonts_Resolver::get_settings() #55981

anton-vlasenko · 2023-11-08T21:31:59Z

What?

This PR aims to fix a PHP fatal error that occurs within WP_Fonts_Resolver::get_settings() under certain conditions.

PHP Fatal error:  Uncaught TypeError: array_merge(): Argument #1 must be of type array, null given in /lib/experimental/fonts-api/class-wp-fonts-resolver.php:208

Why?

Generally, the code should not trigger PHP fatal errors because certain variables do not match the expected type.

How?

This PR ensures that the array_merge() function inside WP_Fonts_Resolver::get_settings() always receives arrays as input parameters.

Testing instructions.

Activate a Pendant block theme.
Add

define('FONT_LIBRARY_DISABLED', true);

to your config file.
3. Edit the src/wp-content/themes/pendant/styles/dark-navy.json file by replacing the following JSON block:

"typography": {
			"fontFamilies": [
				{
					"fontFamily": "Inter, sans-serif",
					"slug": "body-font",
					"name": "Body",
					"fontFace": [
						{
							"fontDisplay": "block",
							"fontFamily": "Inter",
							"fontWeight": "100 500",
							"fontStyle": "normal",
							"fontStretch": "normal",
							"src": [
								"file:./assets/fonts/Inter-Light.ttf"
							]
						},
						{
							"fontDisplay": "block",
							"fontFamily": "Inter",
							"fontWeight": "600 900",
							"fontStyle": "normal",
							"fontStretch": "normal",
							"src": [
								"file:./assets/fonts/Inter-Bold.ttf"
							]
						}
					]
				},
				{
					"fontFamily": "'InterLight', serif",
					"slug": "heading-font",
					"name": "Headings",
					"fontFace": [
						{
							"fontDisplay": "block",
							"fontFamily": "InterLight",
							"fontStyle": "normal",
							"fontStretch": "normal",
							"src": [
								"file:./assets/fonts/Inter-Light.ttf"
							]
						}
					]
				}
			]
		}
	},

with

"typography": {
			"fontFamilies": 1
		}

Make sure you see no

Warning: array_merge(): Expected parameter 2 to be an array,

error in the admin panel.

array_merge() expects input parameters to be arrays.

github-actions · 2023-11-08T21:33:30Z

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-api/class-wp-fonts-resolver.php

simison · 2023-11-09T09:33:10Z

Alternative #55974

azaozz · 2023-11-09T10:41:56Z

lib/experimental/fonts-api/class-wp-fonts-resolver.php

+			// Initialize the font families from settings if set and is an array, otherwise default to an empty array.
+			$settings_font_families = ( isset( $settings['typography']['fontFamilies']['theme'] ) && is_array( $settings['typography']['fontFamilies']['theme'] ) )
+				? $settings['typography']['fontFamilies']['theme']
+				: array();


Thinking this can be outside (above) the loop as $settings is not changed in the loop, just a bit of optimization.

At the same time not sure what is the expected default value (and value type) of $settings['typography']['fontFamilies']['theme'] when it is empty or not set. Same for $variation['settings']['typography']['fontFamilies']['theme']. Setting these to empty arrays when missing means that the final $settings['typography']['fontFamilies']['theme'] may be an empty array. Is that expected?

Good catch! Fixed in 8e94c1c and 061bafb.

azaozz · 2023-11-09T10:51:48Z

Alternative #55974

Yep, was going to comment there that it may make sense to check both values separately. This PR does that.

The other thing I'm not sure about is what value(s) and type(s) are expected at $settings['typography']['fontFamilies']['theme'], and/or can it be missing/unset.

…]['fontFamilies']['theme'] on every iteration of the loop.

mikachan

The changes here look good to me and I believe they'll prevent the fatal error in question. I'm happy to approve if needed, although this isn't my area of expertise.

azaozz · 2023-11-10T13:10:42Z

Looking bit more at get_settings(), it's starting to look pretty buggy/inconsistent. Wondering if now would be a good time to fix it or just patch the obvious bug and leave the rest for later.

This is at the top of the loop:

if ( empty( $variation['settings']['typography']['fontFamilies'] ) ) {
	continue;
}

Not sure why it only checks $variation['settings']['typography']['fontFamilies'] when we actually expect to use/merge$variation['settings']['typography']['fontFamilies']['theme']? Doesn't make sense imho. Should probably be checking if theme is empty. That will also simplify the code after it.

This is at the bottom of the loop:

// Make sure there are no duplicates.
$settings['typography']['fontFamilies'] = array_unique( $settings['typography']['fontFamilies'] );

What is it supposed to be checking for duplicates? We've likely added some array elements to $settings['typography']['fontFamilies']['theme'] so the above code does nothing at all. Then why would it do that inside the loop, and not after it when the array has been set/merged with all the variations?

anton-vlasenko · 2023-11-10T16:47:44Z

What is it supposed to be checking for duplicates? We've likely added some array elements to $settings['typography']['fontFamilies']['theme'] so the above code does nothing at all. Then why would it do that inside the loop, and not after it when the array has been set/merged with all the variations?

@azaozz I noticed it too and agree with your concerns.
Since I'm not the author of the code, it's hard to determine the original intent.
As this PR primarily focuses on resolving the fatal errors in WP_Fonts_Resolver::get_settings(), perhaps it would be better to address these concerns in another PR.

vcanales

As this PR primarily focuses on resolving the fatal errors in WP_Fonts_Resolver::get_settings(), perhaps it would be better to address these concerns in another PR.

+1 to this, I think a refactor should be addressed after getting the fatal errors being thrown out of the way.

I've created an issue so that we don't lose track of the discussion.

noahtallen · 2023-11-10T17:49:06Z

I hope I didn't step on any toes here -- this issue has been causing quite a few problems, so getting it into a patch release feels urgent enough to warrant moving fast!

* Check variables before passing them to array_merge. array_merge() expects input parameters to be arrays. * Optimization: don't check for the existence of $settings['typography']['fontFamilies']['theme'] on every iteration of the loop. * Optimization: ternary is not needed here.

Follow up to #55981

* Check variables before passing them to array_merge. array_merge() expects input parameters to be arrays. * Optimization: don't check for the existence of $settings['typography']['fontFamilies']['theme'] on every iteration of the loop. * Optimization: ternary is not needed here.

anton-vlasenko · 2024-01-19T12:44:56Z

Regarding backporting to Core: this PR should be backported as part of a larger PR that includes backporting the /experimental/fonts-api code to Core.
It doesn't make sense to backport this single PR to Core, as the WP_Fonts_Resolver class does not yet exist in Core yet.

mikachan · 2024-01-19T13:03:26Z

Regarding backporting to Core: this PR should be backported as part of a larger PR that includes backporting the /experimental/fonts-api code to Core.

To confirm further, this file is actually about to be removed and so will not require backporting at all. Please see this PR for more info: #57972

getdave · 2024-01-22T15:06:35Z

✅ I updated the PHP Sync Tracking Issue to note this PR does not require a backport.

anton-vlasenko added 2 commits November 8, 2023 22:01

Check variables before passing them to array_merge.

5e88c36

array_merge() expects input parameters to be arrays.

Fix the comment.

5737291

anton-vlasenko marked this pull request as ready for review November 9, 2023 09:21

anton-vlasenko requested a review from spacedmonkey as a code owner November 9, 2023 09:21

anton-vlasenko requested review from hellofromtonya, azaozz and ironprogrammer November 9, 2023 09:29

anton-vlasenko added the [Type] Bug An existing feature does not function as intended label Nov 9, 2023

simison requested a review from mikachan November 9, 2023 09:32

simison mentioned this pull request Nov 9, 2023

Fonts API: Add is_array check before using array_merge #55974

Closed

azaozz reviewed Nov 9, 2023

View reviewed changes

anton-vlasenko added 2 commits November 9, 2023 13:49

Optimization: don't check for the existence of $settings['typography'…

8e94c1c

…]['fontFamilies']['theme'] on every iteration of the loop.

Optimization: ternary is not needed here.

061bafb

mikachan reviewed Nov 10, 2023

View reviewed changes

vcanales mentioned this pull request Nov 10, 2023

Follow up: Concerns Regarding WP_Fonts_Resolver::get_settings() #56044

Open

vcanales approved these changes Nov 10, 2023

View reviewed changes

noahtallen merged commit d998c7a into trunk Nov 10, 2023
49 checks passed

noahtallen deleted the fix/fatal-error-in-wp-fonts-resolver-get-settings branch November 10, 2023 17:46

github-actions bot added this to the Gutenberg 17.1 milestone Nov 10, 2023

priethor added the Backport to Gutenberg Minor Release Pull request that needs to be backported to a Gutenberg minor release label Nov 10, 2023

vcanales pushed a commit that referenced this pull request Nov 10, 2023

Make sure theme families are array before merging

484946d

Follow up to #55981

vcanales mentioned this pull request Nov 10, 2023

Make sure theme families are array before attempting to merge them #56050

Closed

oandregal added the [Feature] Typography Font and typography-related issues and PRs label Nov 16, 2023

getdave mentioned this pull request Jan 18, 2024

Plugin: Synchronize PHP changes for WordPress 6.5 release #57959

Closed

ellatrix removed the Backport to Gutenberg Minor Release Pull request that needs to be backported to a Gutenberg minor release label Feb 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix fatal error in WP_Fonts_Resolver::get_settings() #55981

Fix fatal error in WP_Fonts_Resolver::get_settings() #55981

anton-vlasenko commented Nov 8, 2023 •

edited

Loading

github-actions bot commented Nov 8, 2023

simison commented Nov 9, 2023

azaozz Nov 9, 2023 •

edited

Loading

anton-vlasenko Nov 9, 2023

azaozz commented Nov 9, 2023

mikachan left a comment

azaozz commented Nov 10, 2023 •

edited

Loading

anton-vlasenko commented Nov 10, 2023

vcanales left a comment

noahtallen commented Nov 10, 2023

anton-vlasenko commented Jan 19, 2024

mikachan commented Jan 19, 2024

getdave commented Jan 22, 2024

Fix fatal error in WP_Fonts_Resolver::get_settings() #55981

Fix fatal error in WP_Fonts_Resolver::get_settings() #55981

Conversation

anton-vlasenko commented Nov 8, 2023 • edited Loading

What?

Why?

How?

Testing instructions.

github-actions bot commented Nov 8, 2023

simison commented Nov 9, 2023

azaozz Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

anton-vlasenko Nov 9, 2023

Choose a reason for hiding this comment

azaozz commented Nov 9, 2023

mikachan left a comment

Choose a reason for hiding this comment

azaozz commented Nov 10, 2023 • edited Loading

anton-vlasenko commented Nov 10, 2023

vcanales left a comment

Choose a reason for hiding this comment

noahtallen commented Nov 10, 2023

anton-vlasenko commented Jan 19, 2024

mikachan commented Jan 19, 2024

getdave commented Jan 22, 2024

anton-vlasenko commented Nov 8, 2023 •

edited

Loading

azaozz Nov 9, 2023 •

edited

Loading

azaozz commented Nov 10, 2023 •

edited

Loading