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

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

Closed
wants to merge 3 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
484946d
Make sure theme families are array before merging
Nov 10, 2023
0e33f23
Fix fatal error in WP_Fonts_Resolver::get_settings()
arthur791004 Nov 13, 2023
fab55e8
Remove unnecessary check
Nov 13, 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
2 changes: 1 addition & 1 deletion 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 @@ -214,7 +214,7 @@ private static function get_settings() {

// Merge the variation settings with the global settings.
$settings['typography']['fontFamilies']['theme'] = array_merge(
$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.

Excuse me, but I don't understand why this change is necessary.
If $settings['typography']['fontFamilies']['theme'] is null, it will be set to an array here (around line 205):

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

Copy link
Member Author

@vcanales vcanales Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My best guess is that it's needed because the line you mention depends on us being within the editor:

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

Line 190 in 787f8ea

$set_theme_structure = true;

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

Line 200 in 787f8ea

if ( $set_theme_structure ) {

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

Line 201 in 787f8ea

$set_theme_structure = false;

If I understand that logic correctly, the second time we go through this condition, $set_theme_structure could be false, hence $settings['typography']['fontFamilies']['theme'] never getting initialized as an array. Another solution would be to move that initialization out of the conditional on $set_theme_structure.

Copy link
Contributor

Choose a reason for hiding this comment

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

$settings['typography']['fontFamilies']['theme'] never getting initialized as an array.

Yes, $set_theme_structure is false on the second iteration of the foreach loop.

However, I don't understand how this affects $settings['typography']['fontFamilies']['theme'].
$settings['typography']['fontFamilies']['theme'] is set to an array during the first iteration of the loop and remains an array in subsequent iterations. array_merge() always returns an array value.

Copy link
Member Author

@vcanales vcanales Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Line 222 in 787f8ea

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

After array_unique on line 222, $settings['typography']['fontFamilies']['theme'] is going null, and it doesn't get reset on the next iteration. It's probably going null because array_unique is getting rid of a duplicate key that contains the the theme array, because the function isn't really comparing the value? I don't think array_unique works well with multidimensional arrays.

Copy link
Contributor

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

Yes, the main issue is the $settings['typography']['fontFamilies']['theme'] becomes null after array_unique as it removes the key with the duplicate value.

Hence, I proposed #56067 to

  1. Set the flag of array_unique to SORT_REGULAR. The default flag is SORT_STRING and it means that the elements are converted to a string to do the comparison. However, the value after the conversion becomes Array and it leads to $settings['typography']['fontFamilies']['theme'] being null if the $settings['typography']['fontFamilies'] contains multiple keys. 
    $settings['typography']['fontFamilies'] = array(
  'default' => array(),
  'theme' => array(),
);

print_r( (string) $settings['typography']['fontFamilies']['default'] ); // Array
print_r( (string) $settings['typography']['fontFamilies']['theme'] ); // Array
  2. Reset the $settings['typography']['fontFamilies']['theme'] after the array_unique call to avoid it becoming null in the next iteration. WDYT?

Copy link
Contributor

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

@vcanales

After array_unique on line 222, $settings['typography']['fontFamilies']['theme'] is going null,

I'm not able to reproduce this.

Can you provide the values of $settings and $variations at which this issue occurs? Please see this code snippet: https://3v4l.org/InOuS#v7.3.0

After array_unique is applied on line 222, $settings['typography']['fontFamilies']['theme'] becomes null and does not reset in the subsequent iteration.

Can you provide an example of such behavior? Specifically, what exact array should I pass to array_unique() in order to make $settings['typography']['fontFamilies']['theme'] a null value?

$variation_font_families
);

Expand Down
Loading