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

Audit of the extensibility available in the widgets admin screen #31126

Closed
draganescu opened this issue Apr 23, 2021 · 13 comments
Closed

Audit of the extensibility available in the widgets admin screen #31126

draganescu opened this issue Apr 23, 2021 · 13 comments
Assignees
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. [Type] Overview Comprehensive, high level view of an area of focus often with multiple tracking issues

Comments

@draganescu
Copy link
Contributor

draganescu commented Apr 23, 2021

Closes #28673

Audit Widgets Screen third-party extensibility

I looked at three files implementing the current Menus screen in WP Admin:

  • wp-admin/widgets.php
  • wp-includes/widgets.php
  • wp-admin/includes/widgets.php

I found, as expected a list of checks, filters and actions. Then I ran through the menus endpoint to check which of them are already fired by the REST API and so are already supported, and which depend on server side rendering. They're marked with:

  • 🟩 if the REST API calls or the editor's code in "lib" already fire these filters and actions when building the responses or results
  • 🟥 if these filters and actions depend on things we don't do anymore in the new editor
  • 🟧 if these filters and actions can be supported and are not
  • ⬜ if these filters and actions are not relevant for the Widgets editor (searched how the functions that fire them are used)

Theme supports and permissions

  • 🟧 supports widgets is required
  • 🟧 user needs edit_theme_options permission
  • 🟧 widgets_access user setting needs to be on

As far as I could find these are all the checks for being allowed to do things on that screen.

Filters

wp-includes/widgets.php

  • ⬜ apply_filter register_sidebar_defaults
  • ⬜ apply_filter dynamic_sidebar_has_widgets
  • ⬜ apply_filter dynamic_sidebar_params
  • ⬜ apply_filter is_active_sidebar
  • 🟩 apply_filter sidebars_widgets
  • 🟩 apply_filter widget_display_callback

wp-admin/includes/widgets.php

  • 🟥 add_filter admin_body_class and sets wp_widgets_access_body_class

wp-admin/widgets.php

  • 🟥 add_filter calls admin_body_class and sets wp_widgets_access_body_class

Actions

wp-includes/widgets.php

  • ⬜ register_sidebar
  • ⬜ wp_register_sidebar_widget
  • ⬜ dynamic_sidebar_before
  • ⬜ dynamic_sidebar_after
  • ⬜ dynamic_sidebar
  • ⬜ the_widget
  • 🟩 widgets_init

wp-admin/includes/widgets.php

  • none

wp-admin/widgets.php

  • 🟧 sidebar_admin_setup
  • 🟧 delete_widget
  • 🟧 widgets_admin_page
  • 🟧 sidebar_admin_page

JS Events

wp-admin/js/widgets.js

  • 🟩 widget-added
@noisysocks
Copy link
Member

Are you still working on this? It's a great start 🙂

@draganescu
Copy link
Contributor Author

Yes I am :(

@skorasaurus skorasaurus added the [Feature] Widgets Screen The block-based screen that replaced widgets.php. label May 6, 2021
@draganescu
Copy link
Contributor Author

I think the initial audit is complete. The orange boxes are what I could not find as being supported but can be added but I am 100% some are false positives.

@draganescu draganescu changed the title [Draft] Audit of the extensibility available in the widgets admin screen Audit of the extensibility available in the widgets admin screen May 8, 2021
@draganescu
Copy link
Contributor Author

As part of the audit I have tested the following stack of "popular" widgets, based on top recommended lists all over the web:

  • Awesome Weather Widget
  • Astra widgets
  • SiteOrigin Widget bundle
  • Candy Social Widget
  • Contact Form 7
  • Contact Widgets
  • Easy Table of Contents
  • Feature a Page Widget
  • Fixed Widget
  • Image Widget
  • Meks Easy Ads Widget
  • Meks Smart Social Widget
  • Opening Hours
  • Simple Social Icons
  • The Events Calendar
  • Widget Importer & Exporter
  • Widget Options
  • Content Aware Sidebars
  • WP Call Button
  • WP Tab Widget
  • WP Tab Widget
  • WPForms Lite
  • Jetpack
  • Woo

I will leave thes notes here:

  • most of the problems that appear are because plugins do not use the standard widget_added event, but instead wait for various DOM elements to appear in WP admin pages
  • some widgets need to be updated to account for the new preview function the the Legacy Block widget, as they rander themselves to eagerly and appear in other widget's previews
  • some widget forms work perfectly in the Customizer Widget block editor but not in the stand alone Widget block editor, despite both instances of the widgets block based editor being the same thing. The explanation is that plugins use different instantiation techiques for the classic screen and the customizer widgets section, with the customizer widgets section being usually the correct implementation.

@noisysocks
Copy link
Member

most of the problems that appear are because plugins do not use the standard widget_added event, but instead wait for various DOM elements to appear in WP admin pages

Which events? Can we fake any of them?

@noisysocks
Copy link
Member

noisysocks commented May 17, 2021

Thanks for looking into this @draganescu. So, to confirm, to close this issue, we should go through each of the 🟧s and add the filter/action to the new screen? If so maybe let's add this to Needs Dev and close #28673?

@draganescu
Copy link
Contributor Author

draganescu commented May 19, 2021

Go through each of the 🟧s and add the filter/action to the new screen.

Yes, if possible.

@draganescu draganescu added [Type] Overview Comprehensive, high level view of an area of focus often with multiple tracking issues [Type] Task Issues or PRs that have been broken down into an individual action to take and removed [Type] Task Issues or PRs that have been broken down into an individual action to take labels May 19, 2021
@noisysocks
Copy link
Member

I'll unassigned you so that somebody can pick this up and add filters for all the 🟧 items.

@tellthemachines tellthemachines self-assigned this May 21, 2021
@tellthemachines
Copy link
Contributor

@draganescu @noisysocks I'm not sure what action is required for these three items:

🟧 supports widgets is required
🟧 user needs edit_theme_options permission
🟧 widgets_access user setting needs to be on

🟧 sidebar_admin_setup - this is already firing in both the widgets screen and the customizer, so I think nothing further is needed?

🟧 delete_widget - this can be added in the widgets controller, I'll do a PR for that.

🟧 widgets_admin_page is supposed to fire just before the page content loads, and 🟧 sidebar_admin_page fires just before the admin footer. For both of these I can't find anywhere suitable to add them on the PHP side, and if I add them in the JS (with doAction from the hooks package) any actions added with PHP will be ignored. Not sure what our best option here is 😕

@noisysocks
Copy link
Member

Thanks for looking into this!

It looks like, now that widgets has been merged into core, all of these changes need to be made in core and not the plugin.

🟧 supports widgets is required
🟧 widgets_access user setting needs to be on

@draganescu is referring to these two checks.

https://github.com/WordPress/wordpress-develop/blob/d270b29f15dcb3609fc98066fac134b279d34c87/src/wp-admin/widgets-form.php#L75

https://github.com/WordPress/wordpress-develop/blob/d270b29f15dcb3609fc98066fac134b279d34c87/src/wp-admin/widgets-form.php#L14

They currently only exist in wp-admin/widgets-form.php which means we're only performing them in the old screen. We should move the checks to wp-admin/widgets.php so that they happen in both screens.

🟧 user needs edit_theme_options permission

This requires no action because the check is already in wp-admin/widgets.php.

https://github.com/WordPress/wordpress-develop/blob/d270b29f15dcb3609fc98066fac134b279d34c87/src/wp-admin/widgets.php#L15

🟧 sidebar_admin_setup - this is already firing in both the widgets screen and the customizer, so I think nothing further is needed?

It looks like we're only calling this in wp-admin/widgets-form.php. We'll need to duplicate it in wp-admin/widgets-form-blocks.php so that it fires in both screens after scripts are enqueued.

https://github.com/WordPress/wordpress-develop/blob/d270b29f15dcb3609fc98066fac134b279d34c87/src/wp-admin/widgets-form.php#L38

🟧 delete_widget - this can be added in the widgets controller, I'll do a PR for that.

Makes sense! This endpoint is in core now, though, so we'll need to make the change in Trac/SVN.

🟧 widgets_admin_page is supposed to fire just before the page content loads

This exists in wp-admin/widgets-form-blocks.php now, so no action is required.

https://github.com/WordPress/wordpress-develop/blob/d270b29f15dcb3609fc98066fac134b279d34c87/src/wp-admin/widgets-form-blocks.php#L65

🟧 sidebar_admin_page fires just before the admin footer

It looks like we're only calling this in wp-admin/widgets-form.php. We'll need to duplicate it in wp-admin/widgets-form-blocks.php so that it fires in both screens right before we require wp-admin/admin-footer.php.

https://github.com/WordPress/wordpress-develop/blob/d270b29f15dcb3609fc98066fac134b279d34c87/src/wp-admin/widgets-form.php#L569

@draganescu
Copy link
Contributor Author

Too late to the party @noisysocks answered everything! 🚀

@tellthemachines
Copy link
Contributor

PRs done in wp-develop:

WordPress/wordpress-develop#1308
WordPress/wordpress-develop#1309

@noisysocks
Copy link
Member

Closing this in favour of the trac tickets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. [Type] Overview Comprehensive, high level view of an area of focus often with multiple tracking issues
Projects
None yet
Development

No branches or pull requests

4 participants