Enqueuing 'wp-editor' script together with wp-edit-widgets or wp-customize-widgets triggers _doing_it_wrong error

[ramonjd]

Description

For classic themes with Gutenberg activated, the following _doing_it_wrong PHP notice appears when loading the Widgets the editor:

Notice: Function wp_enqueue_script() was called <strong>incorrectly</strong>. "wp-editor" script should not be enqueued together with the new widgets editor (wp-edit-widgets or wp-customize-widgets). Please see <a href="https://developer.wordpress.org/advanced-administration/debug/debug-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 5.8.0.) in /var/www/html/wp-includes/functions.php on line 6115

The error comes from the wp_check_widget_editor_deps check in Core:

https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/widgets.php#L2094

In the plugin, it's triggered in client-asset.php

gutenberg/lib/client-assets.php

Line 230 in eadf2dd

gutenberg_override_script(

I'm not sure how to filter out and load separately here.

Here are some potentially related comments/changes:

• https://github.com/ramonjd/wordpress-develop/blob/main/src/wp-admin/widgets-form-blocks.php#L36-L36
• Merge conflicting wp.editor objects into a single, non-conflicting module wordpress-develop#1481
• Legacy widget: Text widget results in javascript error #33203

Step-by-step reproduction instructions

1. In development mode, with the Gutenberg plugin enabled, activate a classic theme, e.g., 2011
2. Go to Appearance > Widgets
3. The error will appear

Screenshots, screen recording, code snippet

No response

Environment info

• WordPress 6.8 alpha
• Gutenberg trunk (19.7 at the time of writing)
• Theme: 2011, or any classic theme with a widget editor

Please confirm that you have searched existing issues in the repo.

• Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

• Yes

Please confirm which theme type you used for testing.

• Block
• Classic
• Hybrid (e.g. classic with theme.json)
• Not sure

[talldan]

Yep, the lowdown is that there's a namespace clash for wp.editor. WordPress's TinyMCE implementation also adds it's code as wp.editor which the @wordpress/editor package would overwrite, so the solution was to not use the @wordpress/editor package in the widget editors (as TinyMCE is needed for legacy widgets).

It looks like this issue from #65937, as that adds the editor dependency.

cc @SantosGuillamot @gziolo.

Possible solutions:

• Move the binding sources to a different package if possible. There's already an issue for moving the pattern overrides source to the block editor package - Move the Pattern Overrides feature to the block editor package #65486. It wouldn't be possible to move the post meta source there, but maybe core-data or a new package is an option? The registration code is already in the blocks package, so that's not an issue.
• Try to fix the namespace clash in another way. I think each legacy widget loads in an iframe anyway, so maybe TinyMCE's version of wp.editor isn't needed in the main frame, only the iframe (I think this is similar to how the classic block works now, though I'm not up to date with how it was achieved).

[ramonjd]

Thanks for the context @talldan 🙇🏻

[SantosGuillamot]

Interesting 🤔 It's true that now that those sources are used outside of the editor in places like widgets, maybe we need to move core sources outside of that package. It makes sense conceptually to me. I am not sure which package we should use, and creating a new one seems too much to me. I feel it would make sense to have them in the blocks package. In the end, block bindings are tied to blocks, and the registration happens there. However, sources like post-meta require the editor package, which would cause issues.

[talldan]

I'd be surprised if those usages of the editor package return anything in the widget editor. 🤔

Maybe it doesn't matter though, the usage seems like it wouldn't prevent the source from working.

[gziolo]

Post Meta source depends on @wordpress/core-data. Both @wordpress/blocks and @wordpress/block-editor don't depend on that package. In general, I don't think they should ever depend on network calls, as they are generic packages for building the standalone editor. Should we extract everything related to bindings to its own package instead? @youknowriad, what are your thoughts here? What's the expected design of @wordpress/fields? That's another place where potentially this code could get relocated.

[talldan]

For the editor dependency, a solution could be to extract the dependency and instead pass the getters in, e.g.:

// editor package
import { createPostMetaSource } from '@wordpress/core-data';

// ...

const postMetaSource = createPostMetaSource( { getCurrentPostType, getEditorSettings } );
registerBindingSource( postMetaSource );

It could even be generalized to a canEdit function that the source calls as part of its checks.

I don't have a strong opinion on the solution though, just offering some options.

[youknowriad]

Post Meta source depends on @wordpress/core-data. Both @wordpress/blocks and @wordpress/block-editor don't depend on that package. In general, I don't think they should ever depend on network calls, as they are generic packages for building the standalone editor. Should we extract everything related to bindings to its own package instead?

Core-data make network calls through api-fetch but that's fine. edit-widgets also does.

what are your thoughts here? What's the expected design of @wordpress/fields

No, that's not the right place for it. The "fields" package is the equivalent of the "block-library" package but for "fields and actions" (it's a library of WP specific fields and actions).

---

My main question is why we need bindings support in the widgets screen? If that support is inevitable, we might have to create a dedicated package for bindings. It's unfortunate thought that a legacy screen is forcing us to do so.

[youknowriad]

Also looking at the code of the "post-meta" binding, I think the "editor" dependency is not needed at all:

• The current post type should always be defined in the context, the fallback is not needed.
• The areCustomFieldsEnabled should not be done called in the binding itself, instead the binding shouldn't be registered at all by the editor package if this flag is true.

[SantosGuillamot]

My main question is why we need bindings support in the widgets screen? If that support is inevitable, we might have to create a dedicated package for bindings. It's unfortunate thought that a legacy screen is forcing us to do so.

I guess that could be reconsidered as well. The original idea was that bindings could be used wherever blocks are used.

The current post type should always be defined in the context, the fallback is not needed.

You might be right. I remember having some issues at some point, but I don't see now why that shouldn't work.

The areCustomFieldsEnabled should not be done called in the binding itself, instead the binding shouldn't be registered at all by the editor package if this flag is true.

I am not sure about this. If a block is connected to post meta, even if the metabox is enabled, shouldn't we show the meta field value (or even the UI to connect fields)?

---

I'll explore how to remove the editor dependency from post-meta, but I'm concerned about limiting ourselves if, in the future, we need that dependency in post-meta or in other core sources.

[youknowriad]

I am not sure about this. If a block is connected to post meta, even if the metabox is enabled, shouldn't we show the meta field value (or even the UI to connect fields)?

Good point :)

I guess that could be reconsidered as well. The original idea was that bindings could be used wherever blocks are used.

I think if this was done just by principle, we should maybe reconsider given these widgets are kind of legacy.

[gziolo]

Post Meta source depends on @wordpress/core-data. Both @wordpress/blocks and @wordpress/block-editor don't depend on that package. In general, I don't think they should ever depend on network calls, as they are generic packages for building the standalone editor. Should we extract everything related to bindings to its own package instead?

Core-data make network calls through api-fetch but that's fine. edit-widgets also does.

I was only confirming what @talldan said, that we can't move registration of Post Meta source to @wordpress/block-editor and @wordpress/blocks. It's crystal clear that @wordpress/core-data has to do network calls, but it's a layer above these two packages 😄

The current post type should always be defined in the context, the fallback is not needed.

Now, regarding the existing code. I agree that this fallback is not necessary here:

gutenberg/packages/editor/src/bindings/post-meta.js

Line 112 in 0f6ab5d

context?.postType || select( editorStore ).getCurrentPostType();

The areCustomFieldsEnabled should not be done called in the binding itself, instead the binding shouldn't be registered at all by the editor package if this flag is true.

The code is here:

gutenberg/packages/editor/src/bindings/post-meta.js

Lines 125 to 127 in 0f6ab5d

	// Check that custom fields metabox is not enabled.
	const areCustomFieldsEnabled =
	select( editorStore ).getEditorSettings().enableCustomFields;

It's only necessary for the post editor to detect whether the user enabled legacy metaboxes. So, we could find an alternative approach that calls this code using the key of the store represented as a string if necessary.

I think if this was done just by principle, we should maybe reconsider given these widgets are kind of legacy.

Yes, it was exactly that. That's surprising that both widgets screens has these implications regarding wp.editor global. A quick solution would be to completely disable the client-side handling for bindings.

[talldan]

One thing is that Synced Patterns don't actually work in the widget editors at the moment, so I guess there's no need for pattern overrides bindings. IIRC, this was because the multi entity saving flow was never implemented. It's something that could probably be revisited though—synced patterns can no longer be edited from the block instance.

Also wondering if it's common to use post meta with widget areas (and if it's been tested that it works?)

Another concern is that without the multi-entity saving, if a binding edits something like a site tagline, then it might not be saveable.

[SantosGuillamot]

I've created this pull request to remove the fallback in post meta. I can explore in another pull request legacy metaboxes.

Regarding widgets, I'm fine removing client-side bindings support there. I don't think it is a common use case. It was tested a bit, but it doesn't have a proper testing suite.

If we agree on that, I'll work on a pull request to remove support.

[SantosGuillamot]

I've started this pull request in case we want to remove client-side support in the widgets screen.

[SantosGuillamot]

I've merged the PR to remove the client-side registration in the widgets screen. We can always add it back properly in the future if needed. Should this be part of 6.7.2?