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

ramonjd commented 3 days ago

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

https://github.com/WordPress/gutenberg/blob/eadf2dda88e69176c9d577ed7cd197a3da9197a2/lib/client-assets.php#L230-L230

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

Here are some potentially related comments/changes:

Step-by-step reproduction instructions

In development mode, with the Gutenberg plugin enabled, activate a classic theme, e.g., 2011
Go to Appearance > Widgets
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.

[x] Yes

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

[x] Yes

Please confirm which theme type you used for testing.

[ ] Block
[x] Classic
[ ] Hybrid (e.g. classic with theme.json)
[ ] Not sure

talldan commented 3 days ago

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 https://github.com/WordPress/gutenberg/pull/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 - https://github.com/WordPress/gutenberg/issues/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 commented 3 days ago

Thanks for the context @talldan 🙇🏻

SantosGuillamot commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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:

https://github.com/WordPress/gutenberg/blob/0f6ab5d720aa70e5982e61a3bc8e3a31b0974ac9/packages/editor/src/bindings/post-meta.js#L112

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:

https://github.com/WordPress/gutenberg/blob/0f6ab5d720aa70e5982e61a3bc8e3a31b0974ac9/packages/editor/src/bindings/post-meta.js#L125-L127

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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

SantosGuillamot commented 2 days ago

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?

WordPress / gutenberg