Opened 11 years ago
Last modified 6 years ago
#28226 new defect (bug)
menu_page_url does not return correct URL on network admin
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 3.0 |
Component: | Plugins | Keywords: | has-patch dev-feedback 2nd-opinion needs-testing |
Focuses: | administration, multisite | Cc: |
Description
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
Attachments (3)
Change History (22)
#3
@
11 years ago
- Version changed from trunk to 3.0
Looks lime a script is changing the version when replying on iPad...
#4
follow-up:
↓ 5
@
11 years ago
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
#5
in reply to:
↑ 4
@
11 years ago
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.
#6
@
11 years ago
- Keywords 2nd-opinion added
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?
#7
@
11 years ago
- Keywords 2nd-opinion removed
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] ) ) { 1443 1443 $parent_slug = $_parent_pages[$menu_slug]; 1444 1444 if ( $parent_slug && ! isset( $_parent_pages[$parent_slug] ) ) { 1445 $url = admin_url( add_query_arg( 'page', $menu_slug, $parent_slug ) );1445 $url = network_admin_url( add_query_arg( 'page', $menu_slug, $parent_slug ) ); 1446 1446 } else { 1447 $url = admin_url( 'admin.php?page=' . $menu_slug );1447 $url = network_admin_url( 'admin.php?page=' . $menu_slug ); 1448 1448 } 1449 1449 } else { 1450 1450 $url = '';
#8
follow-up:
↓ 10
@
11 years ago
@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.
#9
follow-up:
↓ 11
@
11 years ago
This should probably use self_admin_url(). Or, yeah, new functions.
#10
in reply to:
↑ 8
@
11 years ago
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? :-)
#11
in reply to:
↑ 9
@
11 years ago
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
#12
@
11 years ago
- Keywords 2nd-opinion needs-testing added
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.
#13
follow-up:
↓ 14
@
11 years ago
- Milestone changed from Awaiting Review to 4.0
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.
#14
in reply to:
↑ 13
@
11 years ago
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
#15
@
11 years ago
- Version changed from 3.0 to trunk
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.
adds the additional check for is_network_admin