Global Styles: Update REST controller override method and backport changes from Core #65259

Mamaduka · 2024-09-12T06:13:45Z

What?

Updates Global Styles REST controller method override to use post-type registration filter instead of initializing.

Why?

This avoids extra work and prevents both controls from initializing, which are later merged, and the core class is discarded.

See register_rest_route for more details (the $override argument is the key 😄).

Testing Instructions

Set logger somewhere in WP_REST_Global_Styles_Controller_Gutenberg.
Open a post or page.
Confirm that test messages are logged.

Testing Instructions for Keyboard

Same.

github-actions · 2024-09-12T06:14:04Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: spacedmonkey <spacedmonkey@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

ramonjd · 2024-09-12T06:24:53Z

lib/rest-api.php

-	$global_styles_controller->register_routes();
+function gutenberg_override_global_styles_endpoint( array $args, string $post_type ): array {
+	if ( 'wp_global_styles' === $post_type ) {
+		$args['rest_controller_class'] = WP_REST_Global_Styles_Controller_Gutenberg::class;


Oh, that's what you meant 🤦🏻

I just assumed we were doing this somewhere - but clearly not! 🤭

Thanks!

No override was working; it was just doing double work. Using a post-registration filter should be preferred when possible.

Here, I meant why we were doing full override instead of extending the existing class and your answer was satisfying :)

Oh, thanks for confirming. Good to know my powers of comprehension are not completely borked 🤣

I smoked tested this PR only, but things are working as expected with no PHP errors. I'll give it a better test in the morning (AEST) if no-one gets to it first.

Thank you, @ramonjd!

I see one PHP Unit test failure specific to PHP 8.2 env. Investigating that now.

Mamaduka · 2024-09-12T08:46:09Z

I cannot replicate PHP unit test failures locally, making it harder to debug.

@ramonjd, can you check if they're also passing for you locally?

ramonjd · 2024-09-12T23:40:46Z

can you check if they're also passing for you locally?

Yeah, they're passing for me locally too. 🤔 I YOLOed a rebase push to see if the universe is just playing tricks on us.

However, the tests that appear to be failing are related to two, fairly recent features: background images and style variations. So that's something to start with. I'll see if I can reproduce locally....

ramonjd · 2024-09-13T00:17:17Z

I'm stumped. It's almost as if the tests are ignoring the two features, e.g.,

gutenberg/lib/class-wp-rest-global-styles-controller-gutenberg.php

Line 335 in a965ed0

    
           $variations = WP_Theme_JSON_Resolver_Gutenberg::get_style_variations( 'block' );

gutenberg/lib/class-wp-rest-global-styles-controller-gutenberg.php

Lines 711 to 720 in a965ed0

    
           // Register theme-defined variations e.g. from block style variation partials under `/styles`. 
        
           $partials = WP_Theme_JSON_Resolver_Gutenberg::get_style_variations( 'block' ); 
        
           gutenberg_register_block_style_variations_from_theme_json_partials( $partials ); 
        
           $variations = WP_Theme_JSON_Resolver_Gutenberg::get_style_variations(); 
        
           // Add resolved theme asset links. 
        
           foreach ( $variations as $variation ) { 
        
           	$variation_theme_json = new WP_Theme_JSON_Gutenberg( $variation ); 
        
           	$resolved_theme_uris  = WP_Theme_JSON_Resolver_Gutenberg::get_resolved_theme_uris( $variation_theme_json );

Which makes me think it's running the WP_REST_Global_Styles_Controller class from whichever Core version is running.
But even if it were, it looks like the Core classes have the same methods, so scratch that.

It smells of some race condition, e.g., Core methods executing without knowledge of the test fixtures 🤷🏻

Fooling around with the hook priority didn't lead me anywhere.

cc @aaronrobertshaw just in case he has some tricks up his sleeve

aaronrobertshaw

Thanks for the ping 👍

cc @aaronrobertshaw just in case he has some tricks up his sleeve

No tricks per se. Just that I don't have a super deep understanding of everything under the hood with our REST API so maybe I can ask some dumb questions that help spark ideas on explanations/fixes.

Yeah, they're passing for me locally too.

Same for me. All tests passes regardless of if I isolate the single test, the controller tests, multisite, or all PHP tests.

The error for test_update_item_with_custom_block_style_variations is reminiscent of some failures that were occurring when the section styles feature was refactored right before 6.6 beta/RC.

I'm struggling to bring repressed traumatic memories back to the surface but IIRC the error was related to the post not being found to update or something. Gutenberg had some duotone actions that triggered block style variations to be registered and also likely ensured creation of the global styles CPT entry.

This very likely could be a red herring, so feel free to ignore it. I tried removing the Gutenberg duotone actions locally and destroying wp-env but the test still passed locally 🤷

Which makes me think it's running the WP_REST_Global_Styles_Controller class from whichever Core version is running.
But even if it were, it looks like the Core classes have the same methods, so scratch that.

One difference is that the WP_REST_Global_Styles_Controller in Gutenberg is extending WP_REST_Controller whereas WP_REST_Global_Styles_Controller in core extends WP_REST_Posts_Controller. If this PR is now "replacing" the controller class rather than adding additional routes, should it be updated to extend the posts controller as per core's?

ramonjd · 2024-09-16T04:21:51Z

If this PR is now "replacing" the controller class rather than adding additional routes, should it be updated to extend the posts controller as per core's?

🤦🏻 Great spotting. That totally slipped past the net.

ramonjd · 2024-09-16T04:39:00Z

I'm prepping a commit for this branch that brings over all the changes from the Core class. They should have been synced I believe.

It's partly my fault after the Core patch: WordPress/wordpress-develop@6fc019a#diff-58982cfca0cd603cc3ace01a90ef7f243c7842eb8bf99291667a54f9d94ceecc

I think this PR is a good home for it since we're defining the post controller.

It'll be a single commit, so it can be rolled back.

…it WP_REST_Posts_Controller, make methods public et. al Formatting

ramonjd · 2024-09-16T04:59:21Z

I'm prepping a commit for this branch that brings over all the changes from the Core class. They should have been synced I believe.

Okay, that did absolutely nothing to fix the tests 🤣

ramonjd · 2024-09-16T05:08:24Z

Actually, and I missed this too, I think it's because an earlier commit removed the register_routes()

Will test that

ramonjd · 2024-09-16T05:18:17Z

Actually, and I missed this too, I think it's because an earlier commit removed the register_routes()

Will test that

Okay, that did absolutely nothing to fix the tests 🤣

Ditto.

… for backwards compat

ramonjd · 2024-09-16T05:33:19Z

Okay, sorry for all the noise @Mamaduka and @aaronrobertshaw

I'm still stumped. I wonder if it's a sign of a deeper bug.

Wondering if we need to define the revisions controller too. See: https://github.com/WordPress/wordpress-develop/blob/6b81c17cb77ee1bd5fd672a689a1fec85b497161/src/wp-includes/post.php#L489

I did that, but I doubt that will affect the failing tests, which are the same failing tests as with previous commits 🤔

ramonjd · 2024-09-16T06:27:25Z

Okay!

I can confirm it's because the unit tests are running on WP 6.5.5

https://github.com/WordPress/gutenberg/actions/runs/10878265051/job/30180961587?pr=65259#step:13:75

I have reproduced this locally... now to debug

ramonjd · 2024-09-16T07:03:10Z

It's because we're missing

	$args['show_in_rest']                    = true;
	$args['rest_base']                       = 'global-styles';

Overwrite Core endpoints

ramonjd

@Mamaduka looks like things are passing now.

Basically, it was due to the CI using 6.5.5 so we have to register all missing post args since then.

Thanks @aaronrobertshaw for the inheritance tip and helping to test.

ramonjd · 2024-09-16T04:52:29Z