Fix and improve logic in WP_Fonts_Resolver::get_settings()

[azaozz]

What?

Follow-up to #56067.

Changes:

• Check if $variation['settings']['typography']['fontFamilies']['theme'] are set at the top of the loop. It doesn't make sense to only check for $variation['settings']['typography']['fontFamilies'].
• Move the code that sets $settings['typography']['fontFamilies']['theme'] to a empty array before that array is used. It doesn't make sense for that check to be after the array is needed/used.
• Run the above check only once. It doesn't make sense to run it on every iteration of the loop.
• Move the array_unique() after the loop. It doesn't make sense to be inside the loop/to run on every iteration.
• Check the $settings['typography']['fontFamilies']['theme'] array for uniqueness as we were just modifying it in the loop. It doesn't make sense to check $settings['typography']['fontFamilies'] for uniqueness as that array was never modified in this function. This erroneous check was also the cause of the fatal errors as far as I see.

Why?

Attempts to completely fix the errors in WP_Fonts_Resolver::get_settings() that merges the default settings with the variations (if any).

Testing Instructions

See #56067 for instructions.

[github-actions]

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

[arthur791004]

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

[anton-vlasenko]

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

[azaozz]

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.

[arthur791004]

Sounds good!

[arthur791004]

The typography.fontFamilies.theme might not be set if there is no variation or all of them don't have the font definition.

[anton-vlasenko]

I agree. If $settings['typography']['fontFamilies']['theme'] is not initialliy set and there are no $variations, this could become an issue.

[azaozz]

Right, good catch, thanks. Fixing.

[anton-vlasenko]

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

[anton-vlasenko]

Variations overwrite the globals.

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

[azaozz]

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

[anton-vlasenko]

I agree. If $settings['typography']['fontFamilies']['theme'] is not initialliy set and there are no $variations, this could become an issue.

[github-actions]

Flaky tests detected in 654ae65.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6984746001
📝 Reported issues:

• [Flaky Test] should partially select with shift + click (@webkit) #53524 in /test/e2e/specs/editor/various/multi-block-selection.spec.js