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

Backport: WP_Theme_JSON_Resolver changes #3901

Closed
wants to merge 16 commits into from
Closed

Backport: WP_Theme_JSON_Resolver changes #3901

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
1ff63af
Add tests
Mamaduka Jan 24, 2023
f28b16c
Migrate WP_Query performance optimizations
Mamaduka Jan 24, 2023
91b86d8
Merged data should consider origin to return early
Mamaduka Jan 24, 2023
f3cc87c
Make child themes inherit parent's style variations
Mamaduka Jan 24, 2023
19b6516
Try assertSameSets
Mamaduka Jan 24, 2023
d85fa86
Update Global Styles controller tests
Mamaduka Jan 24, 2023
0e595e6
Add since mention to docblock
Mamaduka Jan 24, 2023
8c1d84a
Capitalize JSON here
Mamaduka Feb 3, 2023
8c1db45
Fix spacing
Mamaduka Feb 3, 2023
8b5cc6c
Reference ticket
Mamaduka Feb 3, 2023
9952fa3
Fix fomatting
Mamaduka Feb 3, 2023
3b40b25
plural
Mamaduka Feb 3, 2023
8820c62
Use plural
Mamaduka Feb 3, 2023
3a090ec
Reference the ticket
Mamaduka Feb 3, 2023
9870b44
ensure all code runs before the assertions
Mamaduka Feb 3, 2023
1cc848e
Spacing CS fix
Mamaduka Feb 3, 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
27 changes: 26 additions & 1 deletion tests/phpunit/tests/rest-api/rest-global-styles-controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,27 @@ public function test_get_theme_items() {
$response = rest_get_server()->dispatch( $request );
$data = $response->get_data();
$expected = array(
array(
'version' => 2,
'title' => 'variation-b',
'settings' => array(
'blocks' => array(
'core/post-title' => array(
'color' => array(
'palette' => array(
'theme' => array(
array(
'slug' => 'light',
'name' => 'Light',
'color' => '#f1f1f1',
),
),
),
),
),
),
),
),
array(
'version' => 2,
'title' => 'Block theme variation',
Expand Down Expand Up @@ -511,6 +532,10 @@ public function test_get_theme_items() {
),
),
);
$this->assertSameSetsWithIndex( $data, $expected );

wp_recursive_ksort( $data );
wp_recursive_ksort( $expected );

$this->assertSameSets( $data, $expected );
Comment on lines +535 to +539
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it preferred sorting arrays before assertions, or can we use something like assertEqualsCanonicalizing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here the deeply nested array of arrays are being recursively key sorted before the assertion. assertSameSets() repeats an index sort at the first dimension of the arrays. That's not necessary.

Instead of assertSameSets(), use assertSame().

I'm wondering though why this needed. Pulling down to explore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, @hellofromtonya

Let me know if any follow-ups are needed for PHPUnits tests on this PR.

}
}
2 changes: 1 addition & 1 deletion tests/phpunit/tests/theme/wpThemeJsonResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,7 +970,7 @@ public function test_get_style_variations_returns_all_variations() {
wp_recursive_ksort( $actual_settings );
wp_recursive_ksort( $expected_settings );

$this->assertSameSets(
$this->assertSame(
$expected_settings,
$actual_settings
);
Expand Down