PHP Notice: Undefined index: href in ... html.php (when using `qm/collect/db_objects` filter)

jerclarke commented 4 years ago

I've been getting this error for a long time now. It happens in Html.php->do_panel_menu_item()

    protected function do_panel_menu_item( $id, array $menu ) {
        error_log(print_r($menu, 'return'));
        printf(
            '<li role="presentation"><button role="tab" data-qm-href="%1$s">%2$s</button>',
            esc_attr( $menu['href'] ),
            esc_html( $menu['title'] )
        );
...

This is the error:

PHP Notice:  Undefined index: href in /.../wp-content/plugins/query-monitor/dispatchers/Html.php on line 371

Here's the print_r for some normal $menu values:

[19-Feb-2020 16:05:20 UTC] Array
(
    [id] => query-monitor-caps
    [href] => #qm-caps
    [title] => Capability Checks
    [children] => Array
        (
            [qm-caps-concerned_hooks] => Array
                (
                    [href] => #qm-caps-concerned_hooks
                    [title] => Hooks in Use
                )

        )

)

[19-Feb-2020 16:05:20 UTC] Array
(
    [href] => #qm-caps-concerned_hooks
    [title] => Hooks in Use
)

Here's the $menu that triggers the problem:

[19-Feb-2020 16:05:20 UTC] Array
(
    [children] => Array
        (
            [0] => Array
                (
                    [id] => query-monitor-expensive
                    [href] => #qm-query-expensive
                    [title] => Slow Queries (1)
                )

            [1] => Array
                (
                    [id] => query-monitor-db_dupes
                    [href] => #qm-db_dupes
                    [title] => Duplicate Queries (1)
                )

            [2] => Array
                (
                    [id] => query-monitor-db_callers
                    [href] => #qm-db_callers
                    [title] => Queries by Caller
                )

        )

)

As you can see, it's related to slow and duplicate queries. The errors only show in my logs when I'm on the frontend and there are queries it considers slow.

Clearly the real problem isn't the code generating the error, but the fact that at item is coming in with missing "parent" information. Curious if you have any idea why this is happening to me and not others.

Thanks!

Here's how it looks when I load a page:

jerclarke commented 4 years ago

On further inspection, it's probably because I have a second DB loaded. If I disable my qm/collect/db_objects the notice and problems go away.

Here's my filter code:

function gv_query_monitor_track_lingua_db($globals_wpdb) {
    global $gv_lingua;

    /**
     * If the lingua db object exists insert it with (meaningless) lingua_db slug
     */
    if (is_object($gv_lingua) AND isset($gv_lingua->db) AND is_object($gv_lingua->db))
        $globals_wpdb['lingua_db'] = $gv_lingua->db;

    return $globals_wpdb;
}
add_filter('qm/collect/db_objects', 'gv_query_monitor_track_lingua_db');

From what I can tell, it matches the instructions for multiple DBs and the other features related to it are working:

Is it possible that anyone with a second DB is affected?

johnbillion commented 4 years ago

Thanks, the secondary database support isn't very widely used so I wouldn't be surprised if there are bugs there. I'll try to find some time to take a look!

jerclarke commented 4 years ago

Thanks so much! For now I've added the following code in Html.php in case anyone else has this problem and wants a hotfix (bad idea of course, but hopefully the next update will fix it :) )

    protected function do_panel_menu_item( $id, array $menu ) {

        /**
         * JERHACK: Replace missing href and title to avoid constant notices
         * only applies when our `qm/collect/db_objects` filter is in place
         * Full details: https://github.com/johnbillion/query-monitor/issues/499
         */
        if (!isset($menu['href'])) :
            $menu['href'] = "#qm-missing";
            $menu['title'] = "Missing Title Fix";
        endif;

        printf(
            '<li role="presentation"><button role="tab" data-qm-href="%1$s">%2$s</button>',
            esc_attr( $menu['href'] ),
            esc_html( $menu['title'] )
        );

jerclarke commented 4 years ago

Alright, on further actual usage, I realized there was another spot in QM that was triggering Notice errors because of the missing href and title (it was during the rendering of a <select> so XDebug wasn't showing them on screen)

After more testing, I realized I can fix this using the qm/output/panel_menus filter, and fixing up the broken array when it appears.

i.e. If anyone has this same problem, you should be able to fix it by installing the filter below somewhere on your site :)

/**
 * Query Monitor - Fix bug with panel menu that causes notices in logs and breaks QM popup
 * 
 * Bug seems to be triggered by having multiple db's tracked, i.e. gv_query_monitor_track_lingua_db()
 * 
 * Effect is that there's a panel item with the faulty id 'qm-db_queries-$wpdb' and
 * a `children` sub-array, but missing it's id/href/title properties, which causes errors
 * when the menu is rendered (triggered in multiple places, all fixed by this filter). 
 * 
 * TEST AND REMOVE THIS ONCE IT'S FIXED IN THE PLUGIN
 * 
 * Filters 'qm/output/panel_menus' from before_output() in /query-monitor/dispatchers/Html.php
 *  
 *  $this->panel_menu = apply_filters( 'qm/output/panel_menus', $this->admin_bar_menu );
 * 
 * @see https://github.com/johnbillion/query-monitor/issues/499
 * @param array $panel_menu list of panel menu item arrays, all of which should have id/href/title
 */
function gv_filter_query_monitor_panel_menus($panel_menu) {
    if (isset($panel_menu['qm-db_queries-$wpdb']) AND !isset($panel_menu['qm-db_queries-$wpdb']['title'])) :
        $panel_menu['qm-db_queries-$wpdb']['id'] = 'query-monitor-db_queries';
        $panel_menu['qm-db_queries-$wpdb']['href'] = '#qm-db_queries-MISPLACED';
        $panel_menu['qm-db_queries-$wpdb']['title'] = 'Queries [MISPLACED]';
    endif;
    return $panel_menu;
}
add_filter('qm/output/panel_menus', 'gv_filter_query_monitor_panel_menus', 100);

Cause of the problem

As implied by the filter, I isolated the "source" of the problem in terms of the HTML output to the before_output() method and the qm/output/panel_menus filter, which is where the bad array starts appearing:

    /**
     * Filters the menu items shown in the panel navigation menu in Query Monitor's output.
     *
     * @since 3.0.0
     *
     * @param array $admin_bar_menu Array of menus.
     */
    $this->panel_menu = apply_filters( 'qm/output/panel_menus', $this->admin_bar_menu );

Specifically, from what I can tell, the bad element isn't there after the admin_bar_menu filter runs, but only after the panel_menus filter.

Here's the full array within $panel_menus that causes the problem:

  [qm-db_queries-$wpdb] => Array
        (
            [children] => Array
                (
                    [0] => Array
                        (
                            [id] => query-monitor-db_dupes
                            [href] => #qm-db_dupes
                            [title] => Duplicate Queries (3)
                        )

                    [1] => Array
                        (
                            [id] => query-monitor-db_callers
                            [href] => #qm-db_callers
                            [title] => Queries by Caller
                        )

                    [2] => Array
                        (
                            [id] => query-monitor-db_components
                            [href] => #qm-db_components
                            [title] => Queries by Component
                        )

                )

        )

Orphaned panel items

My conclusion with this is that these children are effectively orphans. They were supposed to be added to a parent that already existed in the panel_menus array, but the qm-db_queries-$wpdb key didn't exist, so they get added to an empty parent and all the errors end up triggering at render time.

I don't know the intention of the original code, or how it was modified, but it seems like it got confused at some point.

When I have no second-db set up, this is the sub-array of panel_menus for the main db, notice that it has qm-db_queries-$wpdb as the key:

 [qm-db_queries-$wpdb] => Array
     (
         [id] => query-monitor-db_queries
         [href] => #qm-db_queries-wpdb
         [title] => Queries
         [children] => Array
             (
                 [0] => Array
                     (
                         [id] => query-monitor-db_dupes
                         [href] => #qm-db_dupes
                         [title] => Duplicate Queries (3)
                     )
                 [1] => Array
                     (
                         [id] => query-monitor-db_callers
                         [href] => #qm-db_callers
                         [title] => Queries by Caller
                     )
                 [2] => Array
                     (
                         [id] => query-monitor-db_components
                         [href] => #qm-db_components
                         [title] => Queries by Component
                     )
             )
     )

When I turn on my second db, I get the following, notice that in this case they both have what I'd consider more coherent keys, without the $:

    [qm-db_queries-wpdb] => Array
        (
            [id] => query-monitor-qm-db_queries-db-wpdb
            [href] => #qm-db_queries-wpdb
            [title] => Queries: $wpdb
        )

    [qm-db_queries-lingua_db] => Array
        (
            [id] => query-monitor-qm-db_queries-db-lingua_db
            [href] => #qm-db_queries-lingua_db
            [title] => Queries: lingua_db
        )

So the question is, is the $ supposed to be there, or not?

Searching the code for qm-db_queries- turned up a variety of places where qm-db_queries-$wpdb is used with single-quotes, and thus it seems like that exact string is the only intended use, and that those sections of code were only ever meant to support the single-database:

Here's an example of one of those bits of code (they all match in essence, with no hint of a $wpdb variable that could have rendered as something else):

    public function panel_menu( array $menu ) {
        $id = $this->collector->id();
        if ( isset( $menu[ $id ] ) ) {
            $menu[ $id ]['title'] = $menu[ $id ]['title'];

            $menu['qm-db_queries-$wpdb']['children'][] = $menu[ $id ];
            unset( $menu[ $id ] );
        }

So to summarize:

It seems like the existing codebase assumes the main database will always be in the qm-db_queries-$wpdb sub-array, which is hardcoded in various places
When a second DB is filtered in, qm-db_queries-$wpdb becomes qm-db_queries-wpdb in the panel_menus array, thus triggering a bunch of problems.
The quick fix would be to make it so that the default WPDB array is always in qm-db_queries-$wpdb even if there's other db's being filtered in.

That said, maybe this points to the need for deeper multi-db support in QM, since the various functions that use qm-db_queries-$wpdb should maybe be doing the same job for the other DBs, etc.

Either way I'm glad I found a solution with my filter code, and hope it helps anyone else who gets stuck!

Thanks as always for the amazing plugin! Hope my long analysis doesn't bog you down. 🙏🏻

jerclarke commented 3 years ago

Not trying to nag, just posting to update that as of v 3.6.7 the problem still persists, and my filter solution still fixes it. Did some testing and updating and figured I'd make a note here 🙏🏻

johnbillion / query-monitor