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

Jump to bottom

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

Merged
merged 11 commits into from
Sep 17, 2024
Merged

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

merged 11 commits into from
Sep 17, 2024

Conversation

Mamaduka
Copy link
Member

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

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

Testing Instructions for Keyboard

Same.

@Mamaduka Mamaduka added [Type] Code Quality Issues or PRs that relate to code quality Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Sep 12, 2024
@Mamaduka Mamaduka self-assigned this Sep 12, 2024
@Mamaduka Mamaduka requested a review from ramonjd September 12, 2024 06:13
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 12, 2024 •
edited
Loading

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 <[email protected]>
Co-authored-by: ramonjd <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>
Co-authored-by: spacedmonkey <[email protected]>

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

@Mamaduka Mamaduka added the No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core label Sep 12, 2024
ramonjd
ramonjd reviewed Sep 12, 2024
View reviewed changes
lib/rest-api.php Outdated
$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;
Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's what you meant 🤦🏻

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

Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, @ramonjd!

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

@Mamaduka
Copy link
Member Author

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 ramonjd force-pushed the update/global-styles-rest-controller-override branch from 78a3e82 to a965ed0 Compare September 12, 2024 23:34
@ramonjd
Copy link
Member

ramonjd commented Sep 12, 2024

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
Copy link
Member

ramonjd commented Sep 13, 2024

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

@Mamaduka Mamaduka force-pushed the update/global-styles-rest-controller-override branch from a965ed0 to a839bab Compare September 14, 2024 17:58
aaronrobertshaw
aaronrobertshaw reviewed Sep 16, 2024
View reviewed changes
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

ramonjd commented Sep 16, 2024

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
Copy link
Member

ramonjd commented Sep 16, 2024 •
edited
Loading

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.

@ramonjd ramonjd force-pushed the update/global-styles-rest-controller-override branch from a839bab to 0bf8e80 Compare September 16, 2024 04:48
@ramonjd
Try: update class with Core's WP_REST_Global_Styles_Controller. Inher…
7d7cd38 
…it WP_REST_Posts_Controller, make methods public et. al

Formatting
@ramonjd ramonjd force-pushed the update/global-styles-rest-controller-override branch from 0bf8e80 to 7d7cd38 Compare September 16, 2024 04:50
@ramonjd
Copy link
Member

ramonjd commented Sep 16, 2024 •
edited
Loading

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
Copy link
Member

ramonjd commented Sep 16, 2024

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

Will test that

@ramonjd ramonjd changed the title Global Styles: Update REST controller override method Global Styles: Update REST controller override method and backport changes from Core Sep 16, 2024
@ramonjd
Copy link
Member

ramonjd commented Sep 16, 2024

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.

@ramonjd
Copy link
Member

ramonjd commented Sep 16, 2024

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
Copy link
Member

ramonjd commented Sep 16, 2024

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
Copy link
Member

ramonjd commented Sep 16, 2024

It's because we're missing

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

@ramonjd
Add 6.6+ post args
43d70f9 
Overwrite Core endpoints
ramonjd
ramonjd reviewed Sep 16, 2024
View reviewed changes
Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

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

lib/class-wp-rest-global-styles-controller-gutenberg.php
Comment on lines +350 to +355
add_filter( 'private_title_format', array( $this, 'protected_title_format' ) );

$data['title']['rendered'] = get_the_title( $post->ID );

remove_filter( 'protected_title_format', array( $this, 'protected_title_format' ) );
remove_filter( 'private_title_format', array( $this, 'protected_title_format' ) );
Copy link
Member

Choose a reason for hiding this comment

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

The private_title_format filter add/remove wasn't backported from WordPress/wordpress-develop@edfc2b0

cc @youknowriad to see whether this was intentional

lib/class-wp-rest-global-styles-controller-gutenberg.php
Comment on lines +16 to +39
class WP_REST_Global_Styles_Controller_Gutenberg extends WP_REST_Posts_Controller {

/**
* Post type.
* Whether the controller supports batching.
*
* @since 5.9.0
* @var string
* @since 6.6.0
* @var array
*/
protected $post_type;
protected $allow_batch = array( 'v1' => false );

/**
* Constructor.
*
* @since 5.9.0
*/
public function __construct() {
$this->namespace = 'wp/v2';
$this->rest_base = 'global-styles';
$this->post_type = 'wp_global_styles';
/**
* Constructor.
*
* @since 6.6.0
*
* @param string $post_type Post type.
*/
public function __construct( $post_type = 'wp_global_styles' ) {
parent::__construct( $post_type );
Copy link
Member

Choose a reason for hiding this comment

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

Done in Core in WordPress/wordpress-develop@6fc019a as part of inheriting WP_REST_Posts_Controller

lib/class-wp-rest-global-styles-controller-gutenberg.php
Comment on lines +411 to +413
'about' => array(
'href' => rest_url( 'wp/v2/types/' . $this->post_type ),
),
Copy link
Member

Choose a reason for hiding this comment

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

Done in Core in WordPress/wordpress-develop@6fc019a as part of inheriting WP_REST_Posts_Controller

lib/class-wp-rest-global-styles-controller-gutenberg.php
* @return array List of link relations.
*/
protected function get_available_actions() {
protected function get_available_actions( $post, $request ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable
Copy link
Member

Choose a reason for hiding this comment

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

Done in Core in WordPress/wordpress-develop@6fc019a as part of inheriting WP_REST_Posts_Controller

lib/class-wp-rest-global-styles-controller-gutenberg.php
* $override is set to true to avoid conflicts with the core endpoint.
* Do not sync to WordPress core.
*/
true
Copy link
Member

Choose a reason for hiding this comment

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

This is required to overwrite Core routes when this endpoint is registered.

lib/class-wp-rest-global-styles-controller-gutenberg.php
@@ -196,28 +219,10 @@ public function get_item_permissions_check( $request ) {
* @param WP_Post $post Post object.
* @return bool Whether the post can be read.
*/
protected function check_read_permission( $post ) {
public function check_read_permission( $post ) {
Copy link
Member

Choose a reason for hiding this comment

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

Done in Core in WordPress/wordpress-develop@6fc019a as part of inheriting WP_REST_Posts_Controller

lib/rest-api.php
$args['revisions_rest_controller_class'] = 'Gutenberg_REST_Global_Styles_Revisions_Controller_6_6';
$args['late_route_registration'] = true;
$args['show_in_rest'] = true;
$args['rest_base'] = 'global-styles';
Copy link
Member

Choose a reason for hiding this comment

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

The CI tests are running 6.5.5. I think it's computed for CI here.

They should probably be running 6.6, but even so, adding the args for backwards compat just in case.

Compare 6.5 https://github.com/WordPress/wordpress-develop/blob/6.5/src/wp-includes/post.php#L473

With 6.6 https://github.com/WordPress/wordpress-develop/blob/6.6/src/wp-includes/post.php#L476

Copy link
Contributor

Choose a reason for hiding this comment

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

They should probably be running 6.6

I'm assuming that these tests run on the previous major WP version due to our BC commitment. That said, I did expect we also ran the unit tests on the latest too.

phpunit/class-wp-rest-global-styles-controller-gutenberg-test.php
@@ -79,14 +79,18 @@ public static function wpTearDownAfterClass() {
* @covers WP_REST_Global_Styles_Controller_Gutenberg::register_routes
*/
public function test_register_routes() {
// Register routes so that they overwrite identical Core routes.
$global_styles_controller = new WP_REST_Global_Styles_Controller_Gutenberg();
$global_styles_controller->register_routes();
Copy link
Member

Choose a reason for hiding this comment

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

This needs to occur to ensure we're only testing the Gutenberg-registered endpoints...

aaronrobertshaw
aaronrobertshaw approved these changes Sep 16, 2024
View reviewed changes
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Thanks for the persistence here @ramon 💪

I pushed a small commit adding some missed updates to the controller so it matches core. Tests still pass locally with this change in place on 6.5.5 and latest. A quick sanity check here would be handy.

✅ I could replicate the failing tests locally on 6.5.5 before the last round of fixes
✅ All tests are passing locally and on GitHub across WP versions
✅ Global Styles controller in Gutenberg mostly matches core (route override args excepted)
✅ Manual smoke testing of the site editor looked fine

I'm happy to approve this one now but it's probably wise to get a second opinion from @Mamaduka and co.

lib/rest-api.php
$args['revisions_rest_controller_class'] = 'Gutenberg_REST_Global_Styles_Revisions_Controller_6_6';
$args['late_route_registration'] = true;
$args['show_in_rest'] = true;
$args['rest_base'] = 'global-styles';
Copy link
Contributor

Choose a reason for hiding this comment

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

They should probably be running 6.6

I'm assuming that these tests run on the previous major WP version due to our BC commitment. That said, I did expect we also ran the unit tests on the latest too.

@spacedmonkey spacedmonkey mentioned this pull request Sep 16, 2024
Merged
@aaronrobertshaw
Copy link
Contributor

I've given this PR another look with fresh eyes. I think it's in a good place to be able to merge and unblock #65071. We can address anything else in a follow-up.

@ramonjd ramonjd merged commit 46d898e into trunk Sep 17, 2024
62 of 63 checks passed
@ramonjd ramonjd deleted the update/global-styles-rest-controller-override branch September 17, 2024 00:45
@github-actions github-actions bot added this to the Gutenberg 19.3 milestone Sep 17, 2024
@Mamaduka
Copy link
Member Author

Thanks for landing this, @aaronrobertshaw and @ramonjd 🙇

@ockham ockham mentioned this pull request Nov 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ramonjd ramonjd ramonjd left review comments

@aaronrobertshaw aaronrobertshaw aaronrobertshaw approved these changes

@spacedmonkey spacedmonkey spacedmonkey approved these changes

Assignees

@Mamaduka Mamaduka

Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
Gutenberg 19.3
Development

Successfully merging this pull request may close these issues.
4 participants
@Mamaduka @ramonjd @aaronrobertshaw @spacedmonkey