menu_page_url does not return correct URL on network admin

[norcross]

the menu_page_url function calls admin_url to build the URL returned. it should check for network admin first and use network_admin_url if present

[norcross]

adds the additional check for is_network_admin

[Denis-de-Bernardy]

is_user_admin() check as well, maybe?

[Denis-de-Bernardy]

Looks lime a script is changing the version when replying on iPad...

[norcross]

not sure is_user_admin would be required, since it's not in the original function. any settings page should do be doing it's various user cap checks anyway (as some settings could be made available to editors, etc), unless I'm missing something that's unique to multisite

[Denis-de-Bernardy]

Replying to norcross:

not sure is_user_admin would be required, since it's not in the original function. any settings page should do be doing it's various user cap checks anyway (as some settings could be made available to editors, etc), unless I'm missing something that's unique to multisite

There's a blog/site admin area (wp-admin), a network admin area (wp-admin/network) and a user admin area (wp-admin/user or users, can't recollect off the top of my head). They use is_blog_admin(), is_network_admin() and is_user_admin() respectively, with is_admin() equivalent to OR'ing the three.

[norcross]

ahh. I misread the original comment (I thought you were doing a check of sorts for whether or not the user was an admin).

given that, do you think a separate function (something like network_menu_url) would be better than adding more checks into this one?

[DrewAPicture]

The extra conditional check in 28226-plugin.php.diff​ is unnecessary. network_admin_url() falls back to admin_url() if it isn't multisite, so just swaping "admin" for "network" in the two function calls should do it:

Version

@@ -1442 +1442 @@
         if ( isset( $_parent_pages[$menu_slug] ) ) {
                 $parent_slug = $_parent_pages[$menu_slug];
                 if ( $parent_slug && ! isset( $_parent_pages[$parent_slug] ) ) {
-                        $url = admin_url( add_query_arg( 'page', $menu_slug, $parent_slug ) );
+                        $url = admin_url( add_query_arg( 'page', $menu_slug, $parent_slug ) );
                 } else {
-                        $url = admin_url( 'admin.php?page=' . $menu_slug );
+                        $url = admin_url( 'admin.php?page=' . $menu_slug );
                 }
         } else {
                 $url = '';

[norcross]

@Drew but that would force the network admin URL on anyone using menu_page_url, which wouldn't always be needed (or wanted) on sites within a network.

[nacin]

This should probably use self_admin_url(). Or, yeah, new functions.

[DrewAPicture]

Replying to norcross:

@Drew but that would force the network admin URL on anyone using menu_page_url, which wouldn't always be needed (or wanted) on sites within a network.

You make a good point. That WikiFormatting is fancy though, amirite? :-)

[norcross]

Replying to nacin:

This should probably use self_admin_url(). Or, yeah, new functions.

the more I think about it, the more I like the idea of a new function. I'll put together a new patch

[norcross]

removes network check from existing menu_page_url, adds new network_menu_page_url

[norcross]

I've attached a first pass at a new function network_menu_page_url. it mimics the menu_page_url function but swaps the admin_url for network_admin_url.

in my testing, this functioned properly for any menu page function (such as add_submenu_page ) when used inside the network_admin_menu hook, which is the only place it should be used.

[DrewAPicture]

Using self_admin_url() here would be the DRY-est approach. I guess I don't see the benefit in duplicating 10 lines of docs and 20 lines of code just to change two function calls.

Also, the @since should read 4.0.0 for a new function.

[norcross]

removed new network_page_url function, added self_admin_url to existing

[norcross]

Replying to DrewAPicture:

Using self_admin_url() here would be the DRY-est approach. I guess I don't see the benefit in duplicating 10 lines of docs and 20 lines of code just to change two function calls.

Also, the @since should read 4.0.0 for a new function.

I see both sides. I know there are some network admin stuff that I'm not too familiar with, but I've added a patch removing the new function and adding the self_admin_url to the existing function. I'm good with either one

[Denis-de-Bernardy]

Either patch seem wrong though: the function is supposed to return the menu url of a page registered by a plugin. The current implementation is correct provided the menu page is not in user admin or in network.

The first patch seems incorrect, plain and simple. Same for the third one, though I'm less sure. Basically, if you're in e.g. the network admin and you call the function to get a menu page url that is registered for the blog admin, you'll get the url of the page for the network admin instead of the expected urlinthe blog admin.

The second patch yields the correct url but is missing the same function for the user admin area.

Methinks the ideal would be for menu_page_url() to conditionally yield the correct url, not based on whether you're in the blog, network or user admin area, but based on whether the menu page is registered in the blog, network or user admin area. But then, are these menu items even registered with the relevant context in the globals?

cc @nacin, as core dev input is needed here imho.

[Denis-de-Bernardy]

/me curses buggy trac js.

[DrewAPicture]

No action in 8 weeks and no consensus on the best approach. Punting.

[SergeyBiryukov]

#31805 was marked as a duplicate.