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.

Sign up for GitHub

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() #56067

Merged
merged 1 commit into from
Nov 13, 2023
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions lib/experimental/fonts-api/class-wp-fonts-resolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,11 +200,6 @@ private static function get_settings() {
if ( $set_theme_structure ) {
$set_theme_structure = false;
$settings = static::set_tyopgraphy_settings_array_structure( $settings );

// Initialize the font families from settings if set and is an array, otherwise default to an empty array.
if ( ! isset( $settings['typography']['fontFamilies']['theme'] ) || ! is_array( $settings['typography']['fontFamilies']['theme'] ) ) {
$settings['typography']['fontFamilies']['theme'] = array();
}
}

// Initialize the font families from variation if set and is an array, otherwise default to an empty array.
Expand All @@ -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 );
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@arthur791004 arthur791004 Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


// 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'] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@arthur791004 arthur791004 Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@anton-vlasenko anton-vlasenko Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

$settings['typography']['fontFamilies']['theme'] = array();
}
}

return $settings;
Expand Down
Loading