includes/plugin.php remove_menu_page should check if $menu is array

[anonymized_18262720]

Depending on when remove_menu_page is executed global $menu many not be populated (mind you this shouldn't happen when using the correct hooks, but should be handled regardless), as a result can lead to warnings in PHP. A check should be added here to prevent an issue. The original code and proposed solution are below:

Old Code:

<?php

function remove_menu_page( $menu_slug ) {
        global $menu;

        foreach ( $menu as $i => $item ) {
                if ( $menu_slug == $item[2] ) {
                        unset( $menu[ $i ] );
                        return $item;
                }
        }

        return false;
}

New Code:

<?php

function remove_menu_page( $menu_slug ) {
    global $menu;

    // Empty check not required, type check only should suffice.
    if (is_array($menu)) { 
        foreach ( $menu as $i => $item ) {
            if ( $menu_slug == $item[2] ) {
                unset( $menu[ $i ] );
                return $item;
            }
        }
    }

    return false;
}

[SergeyBiryukov]

Hi there, welcome to WordPress Trac! Thanks for the report.

Just noting this was discussed a few times before, specifically in #23767 and #19937.

Adding an is_array() or ! empty() check would remove the warning, but would also just hide the fact that calling the function the function too early or on an incorrect hook still doesn't work as expected.

comment:1:ticket:23767 outlines a few scenarios where this warning can occur. All of them are developer errors, so the developer should see that something is wrong. Hiding the warning would just make debugging harder.

[anonymized_18262720]

Thanks for getting back to me so quickly!

However we saw a case related to admin_init hook where on a normal admin page (front-end) works as defined here: ​https://developer.wordpress.org/reference/functions/remove_menu_page/ (also recommended in the comments depending on how quickly you needed something removed, in our case early)

However when admin-ajax.php would be called it would try and run the admin_init hook and thus would fail since the menu isn't loaded. At the very least some sort of context detection would be optimal here if not a fix for detecting the object type prior to running. We have worked around the issue code wise on our end, but I would rather see it handled gracefully.

Replying to SergeyBiryukov:

Hi there, welcome to WordPress Trac! Thanks for the report.

Just noting this was discussed a few times before, specifically in #23767 and #19937.

Adding an is_array() or ! empty() check would remove the warning, but would also just hide the fact that calling the function the function too early or on an incorrect hook still doesn't work as expected.

comment:1:ticket:23767 outlines a few scenarios where this warning can occur. All of them are developer errors, so the developer should see that something is wrong. Hiding the warning would just make debugging harder.

[anonymized_18262720]

Reading through more, it seems like the usage of this function outside of admin_menu is ill-advised though recommended by some for certain reasons. However, I looked into gating this in an elegant way while still retaining functionality you all desire, but it would take some rearchitecting. We can leave this alone for now, but I recommend looking into something to resolve these weird behaviors with hooks in the future.