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 and improve logic in WP_Fonts_Resolver::get_settings() #56489

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de97736
Fix and improve logic in WP_Fonts_Resolver::get_settings()
azaozz Nov 23, 2023
38c8f13
Ensure `$settings['typography']['fontFamilies']['theme']` is not empt…
azaozz Nov 24, 2023
654ae65
Umm, typo?
azaozz Nov 24, 2023
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
38 changes: 22 additions & 16 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 @@ -191,35 +191,41 @@ private static function get_settings() {

foreach ( $variations as $variation ) {

// Skip if settings.typography.fontFamilies are not defined in the variation.
if ( empty( $variation['settings']['typography']['fontFamilies'] ) ) {
// Skip if settings.typography.fontFamilies.theme is not defined or is not an array.
if (
! isset( $variation['settings']['typography']['fontFamilies']['theme'] ) ||
! is_array( $variation['settings']['typography']['fontFamilies']['theme'] )
) {
continue;
}

// One time, set any missing parts of the array structure.
if ( $set_theme_structure ) {
$set_theme_structure = false;
$settings = static::set_tyopgraphy_settings_array_structure( $settings );
}

// Initialize the font families from variation if set and is an array, otherwise default to an empty array.
$variation_font_families = ( isset( $variation['settings']['typography']['fontFamilies']['theme'] ) && is_array( $variation['settings']['typography']['fontFamilies']['theme'] ) )
? $variation['settings']['typography']['fontFamilies']['theme']
: array();
// Font families from settings must be an array.
if (
! isset( $settings['typography']['fontFamilies']['theme'] ) ||
! is_array( $settings['typography']['fontFamilies']['theme'] )
) {
$settings['typography']['fontFamilies']['theme'] = array();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need this anymore as the set_tyopgraphy_settings_array_structure sets the typography.fontFamilies.theme structure in the given array and it works well without this check now

Copy link
Contributor

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

Choose a reason for hiding this comment

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

@arthur791004 The set_tyopgraphy_settings_array_structure() method checks whether the value of $data['typography']['fontFamilies']['theme'] is not empty. However, it does not verify whether the value is actually an array. Therefore, the value could be not empty but not an array. So, in my opinion, this check is still necessary.

Alternatively, this check could be moved to the set_tyopgraphy_settings_array_structure() method instead.

Copy link
Contributor Author

@azaozz azaozz Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, thinking that set_tyopgraphy_settings_array_structure() should be ensuring this is an array, not just checking if empty. I.e. move this code:

if (
	! isset( $settings['typography']['fontFamilies']['theme'] ) ||
	! is_array( $settings['typography']['fontFamilies']['theme'] )
) {
	$settings['typography']['fontFamilies']['theme'] = array();
}

from get_settings() to set_tyopgraphy_settings_array_structure(), and not just for themes but for all values that should be arrays.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

}
}

// Merge the variation settings with the global settings.
// Merge the variation settings with the global settings. Variations overwrite the globals.
Copy link
Contributor

Choose a reason for hiding this comment

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

Variations overwrite the globals.

I like the effort to explain what array_unique() does here. 👍

Copy link
Contributor Author

@azaozz azaozz Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hehe, yea may not be obvious to all. A simple inline comment may save somebody an hour or two in some cases :)

$settings['typography']['fontFamilies']['theme'] = array_merge(
$settings['typography']['fontFamilies']['theme'],
$variation_font_families
$variation['settings']['typography']['fontFamilies']['theme']
);
}

// Make sure there are no duplicates.
$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'] ) ) {
$settings['typography']['fontFamilies']['theme'] = array();
}
// Make sure there are no duplicate values with different names (array keys).
if ( ! empty( $settings['typography']['fontFamilies']['theme'] ) ) {
$settings['typography']['fontFamilies']['theme'] = array_unique(
$settings['typography']['fontFamilies']['theme'],
SORT_REGULAR
);
}

return $settings;
Expand Down
Loading