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

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

Merged
merged 4 commits into from
Nov 10, 2023

Conversation

anton-vlasenko
Copy link
Contributor

@anton-vlasenko anton-vlasenko commented Nov 8, 2023

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.

  1. Activate a Pendant block theme.
  2. 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
		}
  1. Make sure you see no
Warning: array_merge(): Expected parameter 2 to be an array,

error in the admin panel.

Copy link

github-actions bot commented Nov 8, 2023

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

@anton-vlasenko anton-vlasenko marked this pull request as ready for review November 9, 2023 09:21
@anton-vlasenko anton-vlasenko added the [Type] Bug An existing feature does not function as intended label Nov 9, 2023
@simison simison requested a review from mikachan November 9, 2023 09:32
@simison
Copy link
Member

simison commented Nov 9, 2023

Alternative #55974

Comment on lines 205 to 208
// 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();
Copy link
Contributor

@azaozz azaozz Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Fixed in 8e94c1c and 061bafb.

@azaozz
Copy link
Contributor

azaozz commented Nov 9, 2023

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.

Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

azaozz commented Nov 10, 2023

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
Copy link
Contributor Author

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.

Copy link
Member

@vcanales vcanales left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 noahtallen merged commit d998c7a into trunk Nov 10, 2023
49 checks passed
@noahtallen noahtallen deleted the fix/fatal-error-in-wp-fonts-resolver-get-settings branch November 10, 2023 17:46
@github-actions github-actions bot added this to the Gutenberg 17.1 milestone Nov 10, 2023
@noahtallen
Copy link
Member

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!

noahtallen pushed a commit that referenced this pull request Nov 10, 2023
* 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.
noahtallen pushed a commit that referenced this pull request Nov 10, 2023
* 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.
@priethor 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
cbravobernal pushed a commit that referenced this pull request Nov 14, 2023
* 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.
@oandregal oandregal added the [Feature] Typography Font and typography-related issues and PRs label Nov 16, 2023
@anton-vlasenko
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor

getdave commented Jan 22, 2024

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

@ellatrix ellatrix removed the Backport to Gutenberg Minor Release Pull request that needs to be backported to a Gutenberg minor release label Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Typography Font and typography-related issues and PRs [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants