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

Replacing widgets for Categories, Archives, and Text causes theme/plugin incompatibilities #4492

Closed
Sorata opened this issue Apr 1, 2020 · 11 comments · Fixed by #4515
Closed
Assignees
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. WS:Core Work stream for Plugin core
Milestone

Comments

@Sorata
Copy link

Sorata commented Apr 1, 2020

Plugin Version 1.5.1
WordPress Version 5.4

the file: amp/includes/widgets/class-amp-widget-categories.php
make my theme sidebar.php's WP_Widget_Categories not work.

<?php
if(is_home() || is_page()){
    the_widget( 'WP_Widget_Categories', array(
        'count'		=>	0,
        'dropdown'	=>	0
    ), array(
        'before_widget'	=>	'<aside id="categories" class="widget well widget_categories">',
        'after_widget'	=>	'</aside>',
        'before_title'	=>	'<h2 class="widget-title">',
        'after_title'	=>	'</h2>',
    ) );
}

Now I clear the content of amp/includes/widgets/class-amp-widget-categories.php
So that I can show the Categories in the index. Maybe you can check if there's some problem of this code.

@pierlon
Copy link
Contributor

pierlon commented Apr 1, 2020

Hi @Sorata,

Please post the URL to your site, along with your Site Health information.

Also, to get this issue resolved as quickly as possible, please enable Transitional mode so we can compare AMP and non-AMP.

@pierlon
Copy link
Contributor

pierlon commented Apr 1, 2020

I can verify this is an issue with the plugin. While our widgets are being registered, the ones we override are also being unregistered:

unregister_widget( $registered_widget_class_name );
register_widget( $amp_class_name );

An error will be thrown if one of those unregistered widget classes are used via the_widget().


For example, with the Illdy theme activated (and the blog-sidebar sidebar not active), the following error stacktrace can be seen when on the homepage:


[01-Apr-2020 13:35:52 UTC] PHP Notice:  the_widget was called <strong>incorrectly</strong>. Widgets need to be registered using <code>register_widget()</code>, before they can be displayed. Please see <a href="https://wordpress.org/support/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 4.9.0.) in /app/public/core-dev/src/wp-includes/functions.php on line 5178
[01-Apr-2020 13:35:52 UTC] PHP Stack trace:
[01-Apr-2020 13:35:52 UTC] PHP   1. {main}() /app/public/index.php:0
[01-Apr-2020 13:35:52 UTC] PHP   2. require() /app/public/index.php:24
[01-Apr-2020 13:35:52 UTC] PHP   3. require_once() /app/public/core-dev/src/wp-blog-header.php:19
[01-Apr-2020 13:35:52 UTC] PHP   4. include() /app/public/core-dev/src/wp-includes/template-loader.php:106
[01-Apr-2020 13:35:52 UTC] PHP   5. get_sidebar() /app/public/content/themes/illdy/front-page.php:33
[01-Apr-2020 13:35:52 UTC] PHP   6. locate_template() /app/public/core-dev/src/wp-includes/general-template.php:111
[01-Apr-2020 13:35:52 UTC] PHP   7. load_template() /app/public/core-dev/src/wp-includes/template.php:672
[01-Apr-2020 13:35:52 UTC] PHP   8. require_once() /app/public/core-dev/src/wp-includes/template.php:723
[01-Apr-2020 13:35:52 UTC] PHP   9. the_widget() /app/public/content/themes/illdy/sidebar.php:22
[01-Apr-2020 13:35:52 UTC] PHP  10. _doing_it_wrong() /app/public/core-dev/src/wp-includes/widgets.php:1146
[01-Apr-2020 13:35:52 UTC] PHP  11. trigger_error() /app/public/core-dev/src/wp-includes/functions.php:5178

The reason for that is because the WP_Widget_Categories and WP_Widget_Archives classes used by the theme are unregistered, causing WordPress to mark the_widget() as being incorrectly called.

@pierlon
Copy link
Contributor

pierlon commented Apr 1, 2020

@westonruter was there any particular reason for the parent widgets being unregistered? For context, see #870.

@pierlon pierlon added the Bug Something isn't working label Apr 1, 2020
@westonruter westonruter added this to the v1.5.2 milestone Apr 1, 2020
@westonruter
Copy link
Member

The reason is the plugin is unregistering that widget is that it is replacing it with AMP_Widget_Categories. It's also doing this for the Archives widget I believe. The reason why these need AMP variants is when you select the dropdown checkbox, as then an onchange attribute is added to submit the form when a selection is made. There is also a block sanitizer doing the same for the Archives and Categories blocks.

Nevertheless, it's clear replacing the widget class is not the correct way to go. I suggest eliminating the widget overrides and instead add the fixes for the widgets to the block sanitizer. Widgets are being migrated to blocks anyway, so it makes sense to be part of that class.

@westonruter
Copy link
Member

And the reason for unregistering is to prevent the user from seeing duplicated widgets in the admin. They should only see on Archives widget and one Categories widget.

@westonruter
Copy link
Member

Making this change will eliminate some messy initialization logic, where widgets_init does the AMP replacement even on non-AMP responses, since widgets are initialized before the query is parsed.

@pierlon pierlon self-assigned this Apr 1, 2020
@Sorata
Copy link
Author

Sorata commented Apr 2, 2020

Thank you for the reply of pierlon and westonruter.
Sorry about not reply in time. It looks like you found the key.
I'm looking forward to future release version. Have a nice day^_^

@westonruter
Copy link
Member

@Sorata While waiting for us to fix this, please use this workaround code in the mean time:

if ( is_home() || is_page() ) {

	$categories_class = 'WP_Widget_Categories';
	
	// Start temporary workaround code for AMP plugin issue.
	global $wp_widget_factory;
	if ( ! isset( $wp_widget_factory->widgets[ $categories_class ] ) && class_exists( 'AMP_Widget_Categories' ) ) {
		$categories_class = 'AMP_Widget_Categories';
	}
	// End temporary workaround code for AMP plugin issue.

	the_widget( $categories_class, array(
		'count'    => 0,
		'dropdown' => 1
	), array(
		'before_widget' => '<aside id="categories" class="widget well widget_categories">',
		'after_widget'  => '</aside>',
		'before_title'  => '<h2 class="widget-title">',
		'after_title'   => '</h2>',
	) );
}

@Sorata
Copy link
Author

Sorata commented Apr 5, 2020

@westonruter
Thank for your help ^_^

I use the same way to resolve the WP_Widget_Archives.

@westonruter westonruter modified the milestones: v1.5.3, v1.6 Apr 10, 2020
@westonruter westonruter changed the title includes/widgets/class-amp-widget-categories.php make sidebar.php WP_Widget_Categories not work. Replacing widgets for Categories, Archives, and Text causes theme/plugin incompatibilities Apr 21, 2020
@westonruter
Copy link
Member

@Sorata There is now a pre-release build with a fix available for testing: #4515 (comment)

@westonruter westonruter removed this from the v1.6 milestone Apr 25, 2020
@schlessera
Copy link
Collaborator

QA passed

  • Widgets render correctly on the AMP version of the page.
  • Video size is calculated correctly and is a pixel-perfect match in size to the non-AMP version (note that the player itself looks differently in terms of controls).
  • No validation errors on the page.
  • The original issue of producing an error because the widgets were unregistered is solved as well, the widgets can now be correctly referenced from anywhere in the code.

Image 2020-05-19 at 8 29 51 AM

Image 2020-05-19 at 8 48 29 AM

@kmyram kmyram added this to the v1.6 milestone May 27, 2020
@kmyram kmyram closed this as completed Jun 2, 2020
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 17, 2020
@kmyram kmyram added the WS:Core Work stream for Plugin core label Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants