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

[Widgets editor] is_admin() is false in the API requests to /wp/v2/widget-types #33462

Closed
adamziel opened this issue Jul 15, 2021 · 10 comments
Closed
Labels
[Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets [Feature] Widgets Customizer Ability to add and edit blocks in Customize → Widgets. [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets

Comments

@adamziel
Copy link
Contributor

Description

The plugin Widget Visibility Time Scheduler registers the in_widget_form callback to augment the widget editing form. It does so in define_admin_hooks function which is called like this:

		if ( is_admin() ) {
			$this->define_admin_hooks();
		} else {
			$this->define_public_hooks();
		}

Unfortunately, is_admin() is false in context of /wp/v2/widget-types.

Step-by-step reproduction instructions

  1. Install the plugin referenced above
  2. Go to the new widgets editor

Expected behaviour

Additional form fields rendered:

Zrzut ekranu 2021-07-15 o 16 37 06

Actual behaviour

No additional form fields rendered:

Zrzut ekranu 2021-07-15 o 16 37 23

@adamziel adamziel added [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets [Package] Edit Widgets /packages/edit-widgets [Feature] Widgets Customizer Ability to add and edit blocks in Customize → Widgets. labels Jul 15, 2021
@adamziel
Copy link
Contributor Author

adamziel commented Jul 15, 2021

Related: #33454 (comment), #33443

Interestingly, this is a case of this @hellofromtonya :

It is also worth noting that technically the plugin could have already set up some hooks that were already executed by the time we got here, but I don't think it's possible to address every single corner here.

Here's the problem: WP_REST_Widget_Types_Controller is constructed before the hinjiwvts plugin registers the hooks.

Here's the stack trace from the plugin function that registers the hooks (note /var/www/html/wp-blog-header.php(13)):

#0 /var/www/html/wp-content/plugins/widget-visibility-time-scheduler/hinjiwvts.php(85): Hinjiwvts->__construct()
#1 /var/www/html/wp-content/plugins/widget-visibility-time-scheduler/hinjiwvts.php(89): run_hinjiwvts()
#2 /var/www/html/wp-settings.php(391): include_once('/var/www/html/w...')
#3 /var/www/html/wp-config.php(294): require_once('/var/www/html/w...')
#4 /var/www/html/wp-load.php(37): require_once('/var/www/html/w...')
#5 /var/www/html/wp-blog-header.php(13): require_once('/var/www/html/w...')
#6 /var/www/html/_index.php(17): require('/var/www/html/w...')
#7 /var/www/html/index.php(15): require_once('/var/www/html/_...')
#8 {main}%

Here's the stack trace from WP_REST_Widget_Types_Controller::__construct (note /var/www/html/wp-blog-header.php(16)):

#0 /var/www/html/wp-content/plugins/gutenberg/lib/rest-api.php(62): WP_REST_Widget_Types_Controller->__construct()
#1 /var/www/html/wp-includes/class-wp-hook.php(292): gutenberg_register_sidebars_and_widgets_endpoint(Object(WP_REST_Server))
#2 /var/www/html/wp-includes/class-wp-hook.php(316): WP_Hook->apply_filters(NULL, Array)
#3 /var/www/html/wp-includes/plugin.php(484): WP_Hook->do_action(Array)
#4 /var/www/html/wp-includes/rest-api.php(521): do_action('rest_api_init', Object(WP_REST_Server))
#5 /var/www/html/wp-includes/rest-api.php(347): rest_get_server()
#6 /var/www/html/wp-includes/class-wp-hook.php(292): rest_api_loaded(Object(WP))
#7 /var/www/html/wp-includes/class-wp-hook.php(316): WP_Hook->apply_filters('', Array)
#8 /var/www/html/wp-includes/plugin.php(551): WP_Hook->do_action(Array)
#9 /var/www/html/wp-includes/class-wp.php(388): do_action_ref_array('parse_request', Array)
#10 /var/www/html/wp-includes/class-wp.php(750): WP->parse_request('')
#11 /var/www/html/wp-includes/functions.php(1291): WP->main('')
#12 /var/www/html/wp-blog-header.php(16): wp()
#13 /var/www/html/_index.php(17): require('/var/www/html/w...')
#14 /var/www/html/index.php(15): require_once('/var/www/html/_...')
#15 {main}%

So it's something initiated in the line 13 of wp-blog-header (require_once __DIR__ . '/wp-load.php';) vs something initiated in the line 16 of wp-blog-header (wp()). WP_REST_Server is only initiated in the wp() execution graph, so also later than the plugin.

The two ways forward I can think of here are:

  1. Ensure that is_admin() returns true for these API endpoints by defining WP_ADMIN (or some new global) to true before the plugins are loaded. This would require a kind of "pre-router" that would analyze the request early, spot the widgets API endpoint, and act on it.
  2. Document this as a breaking change and recommend registering these hooks later on, perhaps in response to init hook?

cc @noisysocks @hellofromtonya @kevin940726 @draganescu @TimothyBJacobs

@codestylist
Copy link

Thank you guys for having a look into this. I'm pretty amazed and happy about this.

@TimothyBJacobs
Copy link
Member

Yeah this would be a documented breaking change. There were similar issues when the block editor was originally introduced in 5.0.

@codestylist
Copy link

Hello @noisysocks . I saw you closed this issue. May I ask if it is solved?
If yes, will it be in the next final release?
Is there a chance for me to test this fix with our plugin before it is published?
Sorry for these questions, but it is my first time beeing part of a fix before the next release ;-).
Thank you for all your effort!

@adamziel
Copy link
Contributor Author

@codestylist I think the consensus is not to solve this one, just document that it is a breaking change and recommend an alternative.

I am not sure if conditionally registering form hooks depending on is_admin() is even needed in the Widget Visibility Time Scheduler plugin – my intuition says that these forms are only ever rendered in the admin anyway (I didn't actually check so I might be wrong there though).

@codestylist
Copy link

Thank you @adamziel for explaining it. I understand. So, I will double check on my end and find another way to handle this.
I do much appreciate the effort of the team. Thank you very much!

@draganescu
Copy link
Contributor

Thank you @codestylist ! For context here is the discussion around the potential fix and the conclusion to revert on that path. 🚀

@codestylist
Copy link

codestylist commented Jul 19, 2021

Thank you @draganescu for pointing me to the slack discussion. In my opinion the explained issue is not the reason for problems with my plugins.
The initial reason for getting in contact was, when I start using my plugin with the block based widgets, I get a 414 error (Request-URI too long). My investiagtion showed me, the issue is not the missing is_admin() result (this is working) but too many parameters when the API is calling widget.php.

grafik

Do you recommand to open a new issue for this behavior?

@adamziel
Copy link
Contributor Author

@codestylist I opened an issue for that: #33540

@codestylist
Copy link

Thank you @adamziel . Thank you very much for your effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets [Feature] Widgets Customizer Ability to add and edit blocks in Customize → Widgets. [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets
Projects
None yet
Development

No branches or pull requests

5 participants