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

arthur791004 · 2023-11-13T04:22:17Z

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 $settings['typography']['fontFamilies']['theme'] won't become null after the array_unique()function inside WP_Fonts_Resolver::get_settings()

Testing Instructions

Activate the ueno theme
Go to the post editor
Ensure you won't see the error of the array_merge

Testing Instructions for Keyboard

Screenshots or screencast

vcanales

This is a good iteration over #56050. Thanks!

noahtallen

I was able to replicate the issue, and can confirm that the Gutenberg build on this PR fixes the issue.

azaozz · 2023-11-13T16:13:04Z

This PR ensures that the $settings['typography']['fontFamilies']['theme'] won't become null after the array_unique() function

PHP's array_unique() cannot return null unless there is an error (i.e. a non-array is passed to it, etc.). See: https://www.php.net/manual/en/function.array-unique.php.

Ummm, sorry but this doesn't look like a proper fix? Please add a test where $settings['typography']['fontFamilies']['theme'] is not set. Seems it will trigger a fatal error despite the changes here :)

Doing the if ( ! isset( $settings['typography']['fontFamilies']['theme'] ) || ! is_array( $settings['typography']['fontFamilies']['theme'] ) ) should be before array_merge(), not after it? Also ideally it should be outside the loop (or set to run only once) as it is totally pointless to run it on every iteration.

azaozz · 2023-11-13T16:18:26Z

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

@@ -219,7 +214,12 @@ private static function get_settings() {
 			);

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


This doesn't make sense to be in the loop.

This is the existing behavior for a long time, so I think it's out of the scope of this PR to address the fatal error. We can plan to move it outside the loop in the follow-up PR. Does it make sense?

Additionally, I'm not sure why we want to remove the duplicates on $settings['typography']['fontFamilies']. Instead, I think doing it on $settings['typography']['fontFamilies']['theme'] makes more sense as we merge the variation settings into $settings['typography']['fontFamilies']['theme'] above and it might have the duplicates.

I'm not sure why we want to remove the duplicates on $settings['typography']['fontFamilies']. Instead, I think doing it on $settings['typography']['fontFamilies']['theme']

Yep, same. Thinking this is one of the problems here that caused the fatal error (see comment below). I'd be a +1 to fix that array_unique() as it seems to be a bug.

azaozz · 2023-11-13T16:19:59Z

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

+			$settings['typography']['fontFamilies'] = array_unique( $settings['typography']['fontFamilies'], SORT_REGULAR );
+
+			// The font families from settings might become null after running the `array_unique`.
+			if ( ! isset( $settings['typography']['fontFamilies']['theme'] ) || ! is_array( $settings['typography']['fontFamilies']['theme'] ) ) {


This check should be before the array_merge() not after it? Also ideally outside of the loop, it's pointless to repeat it all the time.

The $settings['typography']['fontFamilies']['theme'] is removed after the array_unique call, so I think we can also do it here to ensure the $settings['typography']['fontFamilies']['theme'] is set.

I agreed the check should be before the array_merge() but we still need to set the value outside the loop again after the last iteration to ensure the value is set. I think the better approach is to move the array_unique call outside the loop if possible.

@arthur791004
Could you provide a code snippet that would show how array_unique() removes $settings['typography']['fontFamilies']['theme'] ?
I've asked this in the previous PR, and I got no reply.

You could modify this snippet and post a link to the new snippet here.
IMO, array_unique() only removes duplicate values. array_unique() doesn't set values to null.

Here is it: https://3v4l.org/v8DYU#v8.0.0, and I also mentioned the example here, #56067 (comment).

IMO, array_unique() only removes duplicate values. array_unique() doesn't set values to null.

Yes, it removes duplicates. When you access the value after that, the value doesn't exist anymore and returns NULL (maybe this is better than null 😅)

@arthur791004, thank you for expanding on this so I can better understand the proposed idea.
I see your point with this PR.

When you access the value after that, the value doesn't exist anymore and returns NULL.

Yes. And if an array element doesn't exist, it shouldn't be accessed directly, as accessing an undefined array element generally results in a PHP warning.

Maybe this is better than null 😅.

I respectfully disagree with that.
In the general PHP context, it's correct to use both NULL and null as it makes no difference (code-wise).

In the WordPress context, null is being used in all the *.php files and in the PHP coding standards. 😅

arthur791004 · 2023-11-13T16:25:24Z

Ummm, sorry but this doesn't look like a proper fix? Please add a test where $settings['typography']['fontFamilies']['theme'] is not set. Seems it will trigger a fatal error despite the changes here :)

The $settings['typography']['fontFamilies']['theme'] is initialized via the set_tyopgraphy_settings_array_structure if it's not set at the beginning.

Doing the if ( ! isset( $settings['typography']['fontFamilies']['theme'] ) || ! is_array( $settings['typography']['fontFamilies']['theme'] ) ) should be before array_merge(), not after it? Also ideally it should be outside the loop as it is totally pointless to run it on every iteration.

The issue is the theme key might be removed after the array_unique so we have to do it on every iteration instead of outside the loop.

Before passing the flag, doing array_unique( $settings['typography']['fontFamilies'] ) as follows will cause the fatal error.

$settings = array(
  'typography' => array(
    'fontFamilies' => array(
      'default' => array(),
      'theme' => array(),
    ),
  ),
);

array_unique( $settings['typography']['fontFamilies'] );

// Result:
// $settings = array(
//  'typography' => array(
//    'fontFamilies' => array(
//      'default' => array(),
//    ),
//  ),
// );

azaozz · 2023-11-13T16:32:21Z

Before passing the flag, doing array_unique( $settings['typography']['fontFamilies'] ) as follows will cause the fatal error.

Yea, because it is not checking the actual array that needs to be checked for uniqueness, but its "parent array" :)

As far as I see the foreach ( $variations as $variation ) loop may add elements to the $settings['typography']['fontFamilies']['theme'] array, not to $settings['typography']['fontFamilies']. So why is the latter being checked for uniqueness at every iteration? That seems to be one of the bugs here that caused the fatal error.

I.e. should the array_unique() in the foreach ( $variations as $variation ) loop be removing elements from the $settings['typography']['fontFamilies'] array?

arthur791004 · 2023-11-13T16:43:04Z

I suspect we have to do the array_unique on $settings['typography']['fontFamilies']['theme'] instead of $settings['typography']['fontFamilies'] as we merge the values into it rather than its parent. Even so, I guess it's still safe to do it outside the loop.

azaozz · 2023-11-14T14:29:52Z

I suspect we have to do the array_unique on...

Yea, lets fix this and also move it after the loop. Re-checked again today and am positive this was the main problem that was causing the fatal errors. Will also have to move the if ( ! isset( $settings['typography']['fontFamilies']['theme'] ) ||... above the array_merge() where it used to be as it doesn't make sense to be below it.

What is "the etiquette" here when something that was merged needs to be changed/fixed? Continue in the same PR or open a new one? If it was core trac I would have reopened the ticket, unfortunately GH cannot do that..

noahtallen · 2023-11-17T11:06:28Z

I always just open a new PR and link back as much as possible. I guess the equivalent to a reopen-able "ticket" would be a GitHub issue.

getdave · 2024-01-19T13:57:19Z

Can I confirm whether you feel this will need a Core backport for WP 6.5?

getdave · 2024-01-22T15:43:12Z

Looking into this I believe these APIs will be removed in 6.5 and thus we do not need to backport this PR.

getdave · 2024-01-22T15:43:37Z

cc @mikachan for confidence check

mikachan · 2024-01-22T16:23:27Z

Looking into this I believe these APIs will be removed in 6.5 and thus we do not need to backport this PR.

Yes that's correct! The file changed in this PR will be removed as part of #57972, and so won't need a backport.

arthur791004 requested a review from spacedmonkey as a code owner November 13, 2023 04:22

arthur791004 mentioned this pull request Nov 13, 2023

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

Closed

Fix fatal error in WP_Fonts_Resolver::get_settings()

913d5dd

arthur791004 force-pushed the fix/fatal-error-in-wp-fonts-resolver-get-settings-2 branch from 4e999f0 to 913d5dd Compare November 13, 2023 07:10

vcanales approved these changes Nov 13, 2023

View reviewed changes

vcanales added [Type] Bug An existing feature does not function as intended [Feature] Typography Font and typography-related issues and PRs Needs PHP backport Needs PHP backport to Core labels Nov 13, 2023

noahtallen approved these changes Nov 13, 2023

View reviewed changes

noahtallen merged commit cf689ec into WordPress:trunk Nov 13, 2023
47 of 50 checks passed

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

noahtallen modified the milestones: Gutenberg 17.1, Gutenberg 17.0 Nov 13, 2023

noahtallen pushed a commit that referenced this pull request Nov 13, 2023

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

11599fc

arthur791004 mentioned this pull request Nov 13, 2023

WP 6.4 + Gutenberg are causing critical errors when editing posts on Atomic sites Automattic/wp-calypso#83986

Closed

azaozz self-requested a review November 13, 2023 16:16

azaozz reviewed Nov 13, 2023

View reviewed changes

arthur791004 deleted the fix/fatal-error-in-wp-fonts-resolver-get-settings-2 branch November 13, 2023 16:25

cbravobernal pushed a commit that referenced this pull request Nov 14, 2023

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

2bff7a9

azaozz mentioned this pull request Nov 23, 2023

Fix and improve logic in WP_Fonts_Resolver::get_settings() #56489

Open

getdave mentioned this pull request Jan 18, 2024

Plugin: Synchronize PHP changes for WordPress 6.5 release #57959

Closed

getdave removed the Needs PHP backport Needs PHP backport to Core label Jan 22, 2024

Directory

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

arthur791004 commented Nov 13, 2023

vcanales left a comment

noahtallen left a comment

azaozz commented Nov 13, 2023 •

edited

azaozz Nov 13, 2023

arthur791004 Nov 13, 2023 •

edited

azaozz Nov 13, 2023

azaozz Nov 13, 2023

arthur791004 Nov 13, 2023

anton-vlasenko Nov 13, 2023

arthur791004 Nov 14, 2023 •

edited

anton-vlasenko Nov 22, 2023 •

edited

arthur791004 commented Nov 13, 2023

azaozz commented Nov 13, 2023 •

edited

arthur791004 commented Nov 13, 2023

azaozz commented Nov 14, 2023 •

edited

noahtallen commented Nov 17, 2023

getdave commented Jan 19, 2024

getdave commented Jan 22, 2024

getdave commented Jan 22, 2024

mikachan commented Jan 22, 2024

Directory

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

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

Conversation

arthur791004 commented Nov 13, 2023

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

vcanales left a comment

Choose a reason for hiding this comment

noahtallen left a comment

Choose a reason for hiding this comment

azaozz commented Nov 13, 2023 • edited

azaozz Nov 13, 2023

Choose a reason for hiding this comment

arthur791004 Nov 13, 2023 • edited

Choose a reason for hiding this comment

azaozz Nov 13, 2023

Choose a reason for hiding this comment

azaozz Nov 13, 2023

Choose a reason for hiding this comment

arthur791004 Nov 13, 2023

Choose a reason for hiding this comment

anton-vlasenko Nov 13, 2023

Choose a reason for hiding this comment

arthur791004 Nov 14, 2023 • edited

Choose a reason for hiding this comment

anton-vlasenko Nov 22, 2023 • edited

Choose a reason for hiding this comment

arthur791004 commented Nov 13, 2023

azaozz commented Nov 13, 2023 • edited

arthur791004 commented Nov 13, 2023

azaozz commented Nov 14, 2023 • edited

noahtallen commented Nov 17, 2023

getdave commented Jan 19, 2024

getdave commented Jan 22, 2024

getdave commented Jan 22, 2024

mikachan commented Jan 22, 2024

azaozz commented Nov 13, 2023 •

edited

arthur791004 Nov 13, 2023 •

edited

arthur791004 Nov 14, 2023 •

edited

anton-vlasenko Nov 22, 2023 •

edited

azaozz commented Nov 13, 2023 •

edited

azaozz commented Nov 14, 2023 •

edited