Automattic / jetpack

Security, performance, marketing, and design tools — Jetpack is made by WordPress experts to make WP sites safer and faster, and help you grow your traffic.
https://jetpack.com/
Other
1.58k stars 797 forks source link

Jetpack Search: Ensure that the widget always persists an array #23476

Open jsnmoon opened 2 years ago

jsnmoon commented 2 years ago

Impacted plugin

Jetpack

Steps to Reproduce

Reported by one of our customers; exact repro steps are unknown.

What actually happened

It looks like a Jetpack Search widget was persisted into the options table as a non-array value, resulting in this warning notice being thrown:

PHP Warning: Undefined array key 'jetpack-search-filters-3' in /home/(redacted)/wp-includes/widgets.php on line 918

cc @StefMattana @kangzj

StefMattana commented 2 years ago

The error is reported in 4864304-zen. Not sure yet if it's useful to check for any plugin conflicts, I'm happy to follow up with the user about this if that's useful.

kangzj commented 2 years ago

PHP Warning: Undefined array key 'jetpack-search-filters-3' in /home/(redacted)/wp-includes/widgets.php on line 918

The error suggests the option value stored in jetpack-search-filters doesn't have the particular key and also the value is an array. If the value is not an array, the error would be like:

Warning: Trying to access array offset on value of type bool in php shell code on line 1

I looked at all the places where we change the value of jetpack-search-filters, no place seems to add a widget to sidebars without adding in jetpack-search-filters option unless the logic stops in the middle of running.

Other possibilities could be, the option value is changed elsewhere.

https://github.com/Automattic/jetpack/blob/d1dd13e65ce474859b462fdddd2a91be7fc7a603/projects/packages/search/src/instant-search/class-instant-search.php#L392

https://github.com/Automattic/jetpack/blob/d1dd13e65ce474859b462fdddd2a91be7fc7a603/projects/packages/search/src/instant-search/class-instant-search.php#L429

I'll close the issue for now, unless more cases come in.

Thanks.

Ipstenu commented 9 months ago

I've just run into this. Actually I have no idea how long it's been going on for but at the very least since December 4th (that's as far back as my logs go). I last updated Jetpack on December 5th, so it's not 'new'.

My guess is that it has to do with the fact that the 3rd and 4th options (categories and tags) are NOT present on all post types, and it's just flailing miserably when that happens.

Edit: Nope, still happening :(

2023/12/11 00:02:40 [error] 23649#23649: *8152699 FastCGI sent in stderr: "PHP message: 

PHP Warning:  Undefined array key "jetpack-search-filters-3" in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920PHP message: PHP Warning:  Trying to access array offset on value of type null in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920
PHP message: PHP Warning:  Undefined array key "jetpack-search-filters-4" in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920PHP message: PHP Warning:  Trying to access array offset on value of type null in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920
PHP message: PHP Warning:  Undefined array key "jetpack-search-filters-3" in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920PHP message: PHP Warning:  Trying to access array offset on value of type null in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920
PHP message: PHP Warning:  Undefined array key "jetpack-search-filters-4" in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920PHP message: PHP Warning:  Trying to access array offset on value of type null in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920
PHP message: PHP Warning:  Undefined array key "jetpack-search-filters-3" in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920PHP message: PHP Warning:  Trying to access array offset on value of type null in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920
PHP message: PHP Warning:  Undefined array key "jetpack-search-filters-4" in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920PHP message: PHP Warning:  Trying to access array offset on value of type null in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920
PHP message: PHP Warning:  Undefined array key "jetpack-search-filters-3" in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920PHP message: PHP Warning:  Trying to access array offset on value of type null in /

The only part of MY code that touches Jetpack search is this:

    /**
     * Jetpack Search Filters
     * We want to make sure we get post types in there.
     */
    public function init_jetpack_search_filters() {
        if ( class_exists( 'Jetpack_Search' ) ) {
            // phpcs:disable
            \Jetpack_Search::instance()->set_filters( [
                'Content Type' => [
                    'type'  => 'post_type',
                    'count' => 10,
                ],
                'Categories'   => [
                    'type'     => 'taxonomy',
                    'taxonomy' => 'category',
                    'count'    => 10,
                ],
                'Tags'         => [
                    'type'     => 'taxonomy',
                    'taxonomy' => 'post_tag',
                    'count'    => 10,
                ],
            ] );
            // phpcs:enable
        }
    }

Source: https://jetpack.com/support/search/inline-search/customize-inline-search/#code-facets

However the error happens even with my code disabled.

github-actions[bot] commented 9 months ago

Support References

This comment is automatically generated. Please do not edit it.

pd-cm commented 9 months ago

Debugging the error is a bit challenging, as the reported error was 2 years ago and the version of WordPress is unknown. One could make some guesses based on changelog, but generally, this is the code section, with line numbers potentially shifted a bit, as there have been commits in widgets.php in WordPress core in the last two years.

https://github.com/WordPress/WordPress/blob/e99a11600602fbb5282b59227004d61c543d812d/wp-includes/widgets.php#L906-L929

function is_active_widget( $callback = false, $widget_id = false, $id_base = false, $skip_inactive = true ) {
        global $wp_registered_widgets;

        $sidebars_widgets = wp_get_sidebars_widgets();

        if ( is_array( $sidebars_widgets ) ) {
                foreach ( $sidebars_widgets as $sidebar => $widgets ) {
                        if ( $skip_inactive && ( 'wp_inactive_widgets' === $sidebar || str_starts_with( $sidebar, 'orphaned_widgets' ) ) ) {
                                continue;
                        }

                        if ( is_array( $widgets ) ) {
                                foreach ( $widgets as $widget ) {
                                        if ( ( $callback && isset( $wp_registered_widgets[ $widget ]['callback'] ) && $wp_registered_widgets[ $widget ]['callback'] === $callback ) || ( $id_base && _get_widget_id_base( $widget ) === $id_base ) ) {
                                                if ( ! $widget_id || $widget_id === $wp_registered_widgets[ $widget ]['id'] ) {
                                                        return $sidebar;
                                                }
                                        }
                                }
                        }
                }
        }
        return false;
}

In the original report, the error Undefined array key 'jetpack-search-filters-3' indicates an undefined array key is accessed, but line 918 is currently foreach ( $widgets as $widget ) {, making it an unlikely source for this warning. Mika's report of warning Trying to access array offset on value of type null from line 920 is similar... it could possibly be the same error from different PHP version's compilations, but taken literally, the first says an array key was accessed, but not found, while the second says a value within an undefined array key was accessed, but was nothing, not an array with an additional sub-key.

It is likely the file has since shifted down one or two lines, with the suspected code block being:

if ( ( $callback && isset( $wp_registered_widgets[ $widget ]['callback'] ) && $wp_registered_widgets[ $widget ]['callback'] === $callback ) || ( $id_base && _get_widget_id_base( $widget ) === $id_base ) ) {
    if ( ! $widget_id || $widget_id === $wp_registered_widgets[ $widget ]['id'] ) {

At the core, the problem is likely coming from the reality that this code segment is in a foreach which iterates each individual widget within each individual sidebar, but then attempts to get data about the widget from global $wp_registered_widgets.

The first of these lines checks that the desired value is set before accessing it:

if ( ( $callback && isset( $wp_registered_widgets[ $widget ]['callback'] ) && $wp_registered_widgets[ $widget ]['callback'] === $callback ) || ( $id_base && _get_widget_id_base( $widget ) === $id_base ) ) {

e.g., is there a global widget which has a callback set?

The second line does not check the same way -- it makes an assumption that if a callback is set, then so an id must also be set:

if ( ! $widget_id || $widget_id === $wp_registered_widgets[ $widget ]['id'] ) {

Therefore, it would likely resolve Mika's warning to verify id exists in $wp_registered_widgets[ $widget ] before accessing it.

The original warning may be the same thing, but generally, if being thorough, one might verify that $widget is an appropriately formatted array key which exists in $wp_registered_widgets and resolves to an array with both callback and id before manipulating the data or returning $sidebar, which, based on the function name, seems to be intended to indicate that the widget in question is active in a given sidebar.

This may be specific to Jetpack, but more generally seems to be a result of accessing data expected in a global based on a local list of widgets before verifying the global data takes the form expected from the last time the sidebar was saved.

One might expect this to occur in cases where the global registered widgets have changed in structure or do not have a widget ID defined. e.g., perhaps a widget was removed, a JetPack feature turned off when it previously was on, etc.

Ipstenu commented 8 months ago

Interestingly this has moved on:

2023/12/29 17:02:15 [error] 31060#31060: *11151161 FastCGI sent in stderr: "PHP message: PHP Warning:  Undefined array key "jetpack-search-filters-6" in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920; PHP message: PHP Warning:  Trying to access array offset on null in /home/wp_w9hpj2/lezwatchtv.com/wp-includes/widgets.php on line 920", client: IP-ADDRESS-REDACTED, server: lezwatchtv.com, request: "GET /post_type_shows-sitemap3.xml HTTP/1.1", host: "lezwatchtv.com"
davisshaver commented 4 months ago

I'm seeing the same issue as @Ipstenu, actually with two Jetpack widgets (Top Posts and Search).

I did some digging and I believe this issue stems from is_active_widget() being called in the constructor for these module files. As noted in the Codex for is_active_widget(),

To be effective this function has to run after widgets have initialized, at action ‘init’ or later.

I've tested locally with moving the is_active_widget() into init actions. https://github.com/Automattic/jetpack/files/15139701/mypatch.patch

--- a/public/site/wp-content/plugins/jetpack/jetpack_vendor/automattic/jetpack-search/src/widgets/class-search-widget.php
+++ b/public/site/wp-content/plugins/jetpack/jetpack_vendor/automattic/jetpack-search/src/widgets/class-search-widget.php
@@ -70,13 +70,7 @@ class Search_Widget extends \WP_Widget {
                'description' => __( 'Instant search and filtering to help visitors quickly find relevant answers and explore your site.', 'jetpack-search-pkg' ),
            )
        );
-
-       if (
-           Helper::is_active_widget( $this->id ) &&
-           ! $this->is_search_active()
-       ) {
-           $this->activate_search();
-       }
+       add_action( 'init', array( $this, 'init' ) );

        if ( is_admin() ) {
            add_action( 'sidebar_admin_setup', array( $this, 'widget_admin_setup' ) );
@@ -92,6 +86,15 @@ class Search_Widget extends \WP_Widget {
        }
    }

+   public function init() {
+       if (
+           Helper::is_active_widget( $this->id ) &&
+           ! $this->is_search_active()
+       ) {
+           $this->activate_search();
+       }
+   }
+
    /**
     * Check whether search is currently active
     *
diff --git a/public/site/wp-content/plugins/jetpack/modules/widgets/top-posts.php b/public/site/wp-content/plugins/jetpack/modules/widgets/top-posts.php
index 8e792438b..e2b54cf62 100644
--- a/public/site/wp-content/plugins/jetpack/modules/widgets/top-posts.php
+++ b/public/site/wp-content/plugins/jetpack/modules/widgets/top-posts.php
@@ -72,10 +72,7 @@ class Jetpack_Top_Posts_Widget extends WP_Widget {

        $this->default_title = __( 'Top Posts & Pages', 'jetpack' );

-       if ( is_active_widget( false, false, $this->id_base ) || is_customize_preview() ) {
-           add_action( 'wp_enqueue_scripts', array( $this, 'enqueue_style' ) );
-       }
-
+       add_action( 'init', array( $this, 'init' ) );
        /**
         * Add explanation about how the statistics are calculated.
         *
@@ -87,6 +84,12 @@ class Jetpack_Top_Posts_Widget extends WP_Widget {
        add_filter( 'widget_types_to_hide_from_legacy_widget_block', array( $this, 'hide_widget_in_block_editor' ) );
    }

+   public function init() {
+       if ( is_active_widget( false, false, $this->id_base ) || is_customize_preview() ) {
+           add_action( 'wp_enqueue_scripts', array( $this, 'enqueue_style' ) );
+       }
+   }
+

Should we open a new issue about this?

kangzj commented 4 months ago

Thanks for the feedback @davisshaver. I reopened the issue and it's now under our radar; however we are not sure about the timeline to fix it. Thanks for proposing the fix, which is very helpful.

Vrishabhsk commented 2 months ago

hi 👋 I've been encountering the same issue as reported here. Is there a fix available for this?