Fix fatal error in WP_Fonts_Resolver::get_settings()

[arthur791004]

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

1. Activate the ueno theme
2. Go to the post editor
3. 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]

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]

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

[arthur791004]

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.

[azaozz]

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]

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.

[arthur791004]

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.

[anton-vlasenko]

@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.

[arthur791004]

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 😅)

[anton-vlasenko]

@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]

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]

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]

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]

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]

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]

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

[getdave]

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

[getdave]

cc @mikachan for confidence check

[mikachan]

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.