Make WordPress Core

Opened 6 years ago

Closed 4 years ago

#46081 closed defect (bug) (fixed)

get_admin_page_title() returns nothing

Reported by: grapestain's profile grapestain Owned by: valentinbora's profile valentinbora
Milestone: 5.5 Priority: normal
Severity: minor Version: 5.0.3
Component: Administration Keywords: has-patch
Focuses: administration Cc:

Description

The function get_admin_page_title() returns nothing in case a menu page is registered using add_menu_page() and accessed with an url such as .../wp-admin/options.php?page=my_page_slug

Normally one should access the custom admin page using .../wp-admin/admin.php?page=my_page_slug, so it's not really that big of an issue. Actually, I've encountered this, because I've been using some old code samples, which pointed me to the options.php link. I didn't notice the difference, only that sometimes I have a title (from get_admin_page_title()) and sometimes not.

While debugging the issue, I found this:

On .../wp-admin/admin.php?page=my_page_slug the $title global already have the title populated at the time get_admin_page_title() is called, so it just returns that, all is good.

On .../wp-admin/options.php?page=my_page_slug however the $title global is empty, so the get_admin_page_title() function tries to determine the title from the $menu global, which contains all registered menu entries. Nice.

foreach ( (array)$menu as $menu_array ) {
        if ( isset( $menu_array[3] ) ) {
                if ( $menu_array[2] == $pagenow ) {
                        $title = $menu_array[3];
                        return $menu_array[3];
                } elseif ( isset( $plugin_page ) && ($plugin_page == $menu_array[2] ) && ($hook == $menu_array[3] ) ) {
                        $title = $menu_array[3];
                        return $menu_array[3];
                }
        /*...*/
        }
/*...*/
}

(from https://core.trac.wordpress.org/browser/tags/5.0.3/src/wp-admin/includes/plugin.php#L1608)

The problem, however, is that when it loops over the $menu array, it first compares $pagenow to $menu_array[2] (which contains the menu_slug), but as $pagenow is "options.php" this check fails, so then it moves on to another check.

Now this is the part I think the implementation has a mistake. The check consist of 3 parts:

  • isset( $plugin_page ) -> true, as this variable contains the page_slug (as well).
  • ($plugin_page == $menu_array[2] ) -> true, when the for loop is at the right menu entry, because $menu_array[2] has the page_slug too.
  • $hook == $menu_array[3] -> false, but it should be true. See the $hook contains the actual page hook, however the menu array does not store the page hooks under index 3, but index 5. Index 3 is actually the Title, we are trying to find out, as it is visible from being the element of the $menu_array being set as the global $title var and returned in the next line.

You can confirm this by looking at the add_menu_page() function, whihc defines the same array referenced above as $menu_array as:

$new_menu = array( $menu_title, $capability, $menu_slug, $page_title, 'menu-top ' . $icon_class . $hookname, $hookname, $icon_url );

(https://core.trac.wordpress.org/browser/tags/5.0.3/src/wp-admin/includes/plugin.php#L1112)

$hookname is the 6th element of the array, not the 4th as assumed in the check above.

Now, please note, that I'm not trying to say WordPress should support loading admin pages on the options.php the same way as admin.php, however:

  • The main thing is that L1612 above compares potatoes to apples when comparing $hook == $menu_array[3], so regardless of what special cases this code should run other than trying to load an admin page using options.php, it cannot be good. Either the whole criteria is pointless, or it should be $hook == $menu_array[5] instead, because in it's current form it'd not evaluate to true. (Or if it does, that indicates something else is being broken, and should be fixed too!)
  • If accessing an admin page over options.php is not adequate, maybe WordPress should make a warning or notice about it, or simply refuse to render the page. Normally I'd not suggest WordPress should be responsible for enforcing admin pages to be properly implemented, however, this is very misleading to be able to render an admin page almost correctly, in an incorrect way. I have WP_DEBUG on, but I saw no other symptoms of using a wrong URL, other than the missing Title. Everything else worked as expected.

Attachments (1)

46081.diff (641 bytes) - added by audrasjb 4 years ago.
Administration: Fixes an index inconsistency in get_admin_page_title function.

Download all attachments as: .zip

Change History (6)

#1 @valentinbora
5 years ago

  • Keywords dev-feedback 2nd-opinion added
  • Milestone changed from Awaiting Review to 5.5
  • Owner set to valentinbora
  • Status changed from new to accepted

Thanks a lot @grapestain for your ticket and excellent description. Welcome to WordPress Trac and sorry for the long delay.

I'll do my best to get this looked at as soon as possible.

#2 @SergeyBiryukov
5 years ago

Introduced in [3367] / #1447.

@audrasjb
4 years ago

Administration: Fixes an index inconsistency in get_admin_page_title function.

#3 @audrasjb
4 years ago

  • Keywords has-patch added; dev-feedback 2nd-opinion removed

This patch can be tested using this snippet.

<?php
function my_option_page() {
        add_menu_page( 'My option page', 'My option page', 'manage_options', 'my_option_page', 'my_option_page_callback' );
}
add_action( 'admin_menu', 'my_option_page' );

function my_option_page_callback() {
        echo '<h1>' . get_admin_page_title() . '</h1>';
}

It should display the same H1 text with the Menu page title on both wp-admin/admin.php?page=montheme_menu_admin and wp-admin/options.php?page=montheme_menu_admin screens.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#5 @whyisjake
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 48500:

Administration: Fix an index inconsistency in get_admin_page_title() function.

Ensure that get_admin_page_title() returns a value from pages registered using add_menu_page().

Fixes #46081.

Props grapestain, valentinbora, SergeyBiryukov, audrasjb.

Note: See TracTickets for help on using tickets.