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

Add availability checks to widget related code #15983

Merged
merged 1 commit into from Jun 7, 2019

Conversation

@jorgefilipecosta
Copy link
Member

commented Jun 4, 2019

Description

This PR adds availability checks for the function get_current_screen and for the globals wp_widget_factory, wp_registered_widgets.

The function get_current_screen should only be called in admin pages, but according to the docs, we may have admin pages without the function so I think a check should be added. For example, I guess it may be possible to have an admin page with admin_footer but without the function defined.

Regarding the globals wp_widget_factory, wp_registered_widgets used in gutenberg_get_legacy_widget_settings function, the function is called as part of the filter block_editor_settings, I guess the filter may be executed in a situation where the widget globals are not initialized e.g: in a custom plugin page with a custom block editor. I think it is better to check the availability of the globals. If the globals are not available we will not pass available legacy widgets to the front end.

How has this been tested?

I verified there are no noticeable changes to the way legacy widgets and block editor widget screen works.
I added || true and && false to the function checks and global checks respectively. And I verified the noticeable effect was that no available legacy widgets exist, and if I add the following widget code <!-- wp:legacy-widget {"identifier":"WP_Widget_Media_Image","instance":{"title":"My image"},"isCallbackWidget":false} /--> to the post editor nothing is rendered, the widget just renders hidden fields on the server and then the client renders the UI with JS but by not validating the condition gutenberg_is_block_editor we stopped loading that scripts. If I remove the condition the legacy widget should load correctly on the post editor.

@aduth

aduth approved these changes Jun 7, 2019

Copy link
Member

left a comment

This fixes #15209.

@@ -11,6 +11,11 @@
* @return boolean True if a screen containing the block editor is being loaded.
*/
function gutenberg_is_block_editor() {

This comment has been minimized.

Copy link
@aduth

aduth Jun 7, 2019

Member

Aside: This feels like it's not specific to widgets. Might make more sense surfaced up to the top-level gutenberg.php file. Then again, it's only necessary vs. WP_Screen::is_block_editor since the latter doesn't account for the widgets screen. I guess this is something we'll need to sort out sooner than later.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jun 7, 2019

Author Member

It was kept on the widgets.php file temporarily to make merging Gutenberg to core easier (this file is not merged). The final solution should be updating WP_Screen::is_block_editor when things get more stable.

'isCallbackWidget' => false,
'isHidden' => in_array( $class, $core_widgets, true ),
);
if ( ! empty( $GLOBALS['wp_widget_factory'] ) ) {

This comment has been minimized.

Copy link
@aduth

aduth Jun 7, 2019

Member

Minor: Would calling global $wp_widget_factory; when the global isn't assigned be problematic, and then proceed to check ! empty( $wp_widget_factory ) ? Is only a minor difference to avoid reference to the superglobal.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jun 7, 2019

Author Member

In my tests that approach also seems to work well, the code was updated 👍

@jorgefilipecosta jorgefilipecosta force-pushed the add/availability-checks-to-widgets-code branch from 8919827 to 5a8a715 Jun 7, 2019

@jorgefilipecosta jorgefilipecosta merged commit cbac39d into master Jun 7, 2019

1 of 2 checks passed

Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@jorgefilipecosta jorgefilipecosta moved this from Non UI Issues/Tasks to Done in Block Editor For Widgets Jun 7, 2019

@github-actions github-actions bot added this to the Gutenberg 5.9 milestone Jun 7, 2019

@youknowriad youknowriad deleted the add/availability-checks-to-widgets-code branch Jun 10, 2019

nicolad added a commit to nicolad/gutenberg that referenced this pull request Jun 15, 2019

Add availability checks to widget related code (WordPress#15983)
This commit adds availability checks for the function get_current_screen and for the globals wp_widget_factory, wp_registered_widgets.

The function get_current_screen should only be called in admin pages, but according to the docs, we may have admin pages without the function so I think a check should be added. For example, I guess it may be possible to have an admin page with admin_footer but without the function defined.

Regarding the globals wp_widget_factory, wp_registered_widgets used in gutenberg_get_legacy_widget_settings function, the function is called as part of the filter block_editor_settings, I guess the filter may be executed in a situation where the widget globals are not initialized e.g: in a custom plugin page with a custom block editor. I think it is better to check the availability of the globals. If the globals are not available we will not pass available legacy widgets to the front end.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.