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.

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

Site Editor: list view needs enqueue_block_editor_assets #37043

Closed
wants to merge 1 commit into from

Conversation

mattwiebe
Copy link
Contributor

Description

The list view introduced in #36379 does not fire the enqueue_block_editor_assets action. It is possible that this oversight was intentional so as to keep that pageload faster, but it breaks any editor customization that depends on it. All of our other specialized editor instances (widgets, nav menus) have used it, and this should too.

For instance, on WordPress.com, we use apiFetch.setFetchHandler to rewrite REST API requests to our centralized public-api.wordpress.com URL. All of the loading code for this is hooked to enqueue_block_editor_assets, the correct place to add assets to the editor.

How has this been tested?

Load the list view page, expected code hooked to enqueue_block_editor_assets is loaded.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • I've deleted the rest of the checklist as it was inapplicable.

@vindl vindl added this to 👀 Needs review in WordPress 5.9 Must-Haves via automation Dec 1, 2021
@noisysocks noisysocks added the [Type] Bug An existing feature does not function as intended label Dec 1, 2021
Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

Thanks, makes sense. Could you please add the doc comment for the action before the call to do_action()?

@kevin940726
Copy link
Member

This doesn't make a lot of sense to me though. The list view isn't a block editor, it's just a screen/page. Enqueuing block-editor assets seems like an overkill. We could add another filter/action if needed though.

For instance, on WordPress.com, we use apiFetch.setFetchHandler to rewrite REST API requests to our centralized public-api.wordpress.com URL. All of the loading code for this is hooked to enqueue_block_editor_assets, the correct place to add assets to the editor.

Just because the loading code is hooked in enqueue_block_editor_assets doesn't mean that it has to be loaded via that action. But then again I'm not very familiar with how PHP hooks works though 😅 . I'll defer this to @noisysocks's decision 🙈 .

@noisysocks
Copy link
Member

The problem is that enqueue_block_editor_assets is generally used by plugins to register custom sidebars, toolbar buttons, @wordpress/data extensions, anything really. It's turned into a catch-all entry point for plugin customisation. I don't think that was ever the intention but we're a little stuck now.

In the medium term, we probably ought to consider deprecating this hook or evolving it. For example we could add an argument to enqueue_block_editor_assets which contains an instance of WP_Block_Editor_Context that can be used to identify which block editor screen is being loaded.

add_action( 'enqueue_block_editor_assets', function( $context ) {
	if ( 'site-list' === $context→id ) {
		...
	}
} );

In the short term, I guess we have to decide whether the list list view should fire this action or fire a different action. @kevin940726 makes a good point that there's no "editor" in the list view. I am not sure. What do you think, @youknowriad?

@youknowriad
Copy link
Contributor

I think I agree with @kevin940726 The list view doesn't seem like an editor to me. Can't third party plugins just use the regular WP Admin hooks to enqueue their scripts and styles there?

@mattwiebe
Copy link
Contributor Author

I think that passing a WP_Block_Editor_Context object to enqueue_block_editor_assets is a good idea regardless of the way that this PR goes.

This PR would be transparently resolved by simply avoiding making the template list views separate screens within the Site Editor, as has been discussed elsewhere. Previous iterations of the Site Editor transparently flowed between template/part/page modes in a SPA-like manner and I don't understand why this iteration is departing from that.

Otherwise, since enqueue_block_editor_assets is itself ultimately fired within the admin_enqueue_scripts action, we can resolve this on dotcom, because we have people like me dedicated to making sure that broken things get fixed one way or another. But who knows what other plugins might break on account of this, since as @noisysocks says, people hook into various Editor instances in ways other than just blocks and sidebar plugins.

@mattwiebe
Copy link
Contributor Author

Given the unenthusiasm for this approach and the solution in #36488 that makes this irrelevant, I will close this.

@mattwiebe mattwiebe closed this Dec 3, 2021
WordPress 5.9 Must-Haves automation moved this from 👀 Needs review to ✅ Done Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants