Option page as sub-page of another menu broken in 2.8.0

[JiveDig]

Describe the bug

I have a plugin that uses CMB2 to register an options page as a sub-menu of another custom admin page. This works in 2.7.0 but breaks the url in 2.8.0.

Steps to reproduce (I have confirmed I can reproduce this issue on the develop branch):

The following works with 2.7 producing an admin url with /wp-admin/admin.php?page=some_sub_page

but breaks in 2.8 producing an admin url with /wp-admin/some_sub_page

add_action( 'admin_menu', function() {
	add_menu_page(
		esc_html__( 'Some Page', 'textdomain' ),
		esc_html__( 'Some Page', 'textdomain' ),
		'edit_posts',
		'some-page',
		'prefix_render_admin_menu_page'
	);
});

add_action( 'cmb2_admin_init', function() {
	$cmb = new_cmb2_box( array(
		'id'           => 'some_sub_page_metabox',
		'title'        => __( 'Some Sub-Page', 'textdomain' ),
		'object_types' => array( 'options-page' ),
		'option_key'   => 'some_sub_page',
		'parent_slug'  => 'some-page',
	) );
});

function prefix_render_admin_menu_page() {
	echo '<h1>Some Page</h1>';
}

[tw2113]

I'd need to test and try to confirm, but I wonder if it's coming from the changes that were a part of #1380

[sonnysunzu]

I'm also having the same issue

[tw2113]

@sonnysunzu can you provide your CMB2 configuration code as well, in case variations are producing different results, and so we could just verify that a change is indeed affecting both.

[sonnysunzu]

as soon as i remove the 'parent_slug' the side menu will then show the link as a stand alone menu item outside of the submenu and works. so something has happened for submenu items to no longer work

wp add menu settings

add_menu_page('Woocommerce Image Sales', 'Image Sales', $settingsCap, 'wis', array($this, 'wis_about'), 'dashicons-products');

cmb2 setup

$cmb_options = new_cmb2_box(array( 'id' => 'wis_options', 'title' => 'Options', 'menu_title' => __('Options', 'Options'), 'object_types' => array('options-page'), 'option_key' => 'wis_options', 'parent_slug' => 'wis', 'capability' => 'manage_options', 'display_cb' => array($this, 'wis_options_page_output'), 'description' => '', ));

I hope this is what you asked for.

[tw2113]

yep that's the part i was looking for. thanks.

[sonnysunzu]

@tw2113, @JiveDig, I found the issue.

inside includes/CMB2_Options_Hookup.php line 71

add_action( $hook, array( $this, 'options_page_menu_hooks' ), $this->get_priority() );

if you remove $this->get_priority() from the above line. Submenus then work again.

so looking further into this. Too fix my issue without modifying the cmb2 direct i had to add 'priority' => 'low' to my new_cmb2_box

so like this

$cmb_options = new_cmb2_box(array( 'id' => 'wis_options', 'title' => 'Options', 'menu_title' => __('Options', 'Options'), 'object_types' => array('options-page'), 'option_key' => 'wis_options', 'parent_slug' => 'wis', 'capability' => 'manage_options', 'display_cb' => array($this, 'wis_options_page_output'), 'description' => '', 'priority' => 'low' ));

without going to far as its 1am here and i need to be up for work in the morning. I'm going to take a guess that the hook for the submenu is running before the hook for the parent menu item. I could be wrong I'm tired

[tw2113]

I must not be doing something right, because my submenu items with both code samples above put me on 404 pages on the frontend.

For example, I'm ending up on https://wds.test/wp-admin/some_sub_page for @JiveDig's code. I've reverted myself back to the 2.7.0 release as well, just to help. What URLs are you two getting for your submenus?

[JiveDig]

I'm on mobile at the moment, but the exact URLs I get with my reduced code example are all in the OP.

[sonnylloyd]

@tw2113 have you set the priority to low ? like i suggested as this solved the issue for me. (sorry i have two accounts one for work and a personal one. this is my personal one, and the sonnysunzu is my work)

[JiveDig]

Great find @sonnysunzu. Adding any priority works for me. I tested first with low, and it worked but was in the wrong place. Same with high. But just adding 'priority' => 10, put it in the correct place with a link that actually works without 404'ing.

So it looks like the change to get_priority() in 94e80d7 may be missing a fallback/default?

https://github.com/CMB2/CMB2/blob/develop/includes/CMB2_Options_Hookup.php#L71

Is currently:

add_action( $hook, array( $this, 'options_page_menu_hooks' ), $this->get_priority() );

But should be:

add_action( $hook, array( $this, 'options_page_menu_hooks' ), $this->get_priority( 10 ) );

[JiveDig]

Actually, the get_priority method has a default set like public function get_priority( $default = 10 ) { so I'm a bit lost without diving any deeper in.

[sonnylloyd]

@JiveDig @tw2113

I strongly believe the default priority of high is causing the admin_menu_hook hook found in CMB2_Options_Hookup.php on line 71. to run before the parent hook has had chance to run. And because of this the parent page does not exist so it cannot add the sub menu

hence when setting the priority to low corrected the order the hooks was run. meaning the hook for the parent was ran first then the hook for cmb2

This would explain why. when removing the parent id and having the menu option as standalone it worked. because it does not depend on a parent existing first

[tw2113]

@sonnylloyd i was trying with version 2.7.0 and no priority argument passed and still wasn't seeing successful option pages. I may try again later tonight or tomorrow with all this new information, but I was really hoping to have a git bisect chance to try and nail down the exact code change that introduced things.

[JiveDig]

@tw2113 Did you try my original code?

With a clean install of WP and Twenty Twenty One theme active, if I put the following code in functions.php it works in 2.7.0 and doesn't work in 2.8.0.

add_action( 'admin_menu', function() {
	add_menu_page(
		esc_html__( 'Some Page', 'textdomain' ),
		esc_html__( 'Some Page', 'textdomain' ),
		'edit_posts',
		'some-page',
		'prefix_render_admin_menu_page'
	);
});

add_action( 'cmb2_admin_init', function() {
	$cmb = new_cmb2_box( array(
		'id'           => 'some_sub_page_metabox',
		'title'        => __( 'Some Sub-Page', 'textdomain' ),
		'object_types' => array( 'options-page' ),
		'option_key'   => 'some_sub_page',
		'parent_slug'  => 'some-page',
	) );
});

function prefix_render_admin_menu_page() {
	echo '<h1>Some Page</h1>';
}

[JiveDig]

[jtsternberg]

I strongly believe the default priority of high is causing the admin_menu_hook hook found in CMB2_Options_Hookup.php on line 71. to run before the parent hook has had chance to run. And because of this the parent page does not exist so it cannot add the sub menu

That's exactly right. Priority defaults to high for boxes: https://github.com/CMB2/CMB2/blob/develop/includes/CMB2.php#L77

Which is translated to a hook priority of 5: https://github.com/CMB2/CMB2/blob/develop/includes/CMB2_Hookup.php#L814-L817

I didn't notice/realize that by adding this priority thing in 94e80d7 that I would be breaking back-compatibility. Ugh. I'll get this updated so that the priority is default for options pages, that way it will get a priority of 10, putting it back to what it was previously.

[tw2113]

@JiveDig i was only ever getting the second one, even though I'm pretty sure I checked out the right git tag to go back to 2.7.0.

Regardless it looks like Justin is on the case as well and has a good lead on things, so I'm going to let him take over from here.