WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#28226 new defect (bug)

menu_page_url does not return correct URL on network admin

Reported by: norcross Owned by:
Milestone: Future Release 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)

28226-plugin.php.diff (1.0 KB) - added by norcross 4 years ago.
adds the additional check for is_network_admin
28226-plugin.php-2.diff (1.2 KB) - added by norcross 4 years ago.
removes network check from existing menu_page_url, adds new network_menu_page_url
28226-plugin.php-3.diff (741 bytes) - added by norcross 4 years ago.
removed new network_page_url function, added self_admin_url to existing

Download all attachments as: .zip

Change History (22)

@norcross
4 years ago

adds the additional check for is_network_admin

#1 @norcross
4 years ago

  • Focuses multisite added
  • Keywords has-patch dev-feedback added

#2 @Denis-de-Bernardy
4 years ago

  • Version changed from 3.0 to trunk

is_user_admin() check as well, maybe?

#3 @Denis-de-Bernardy
4 years ago

  • Version changed from trunk to 3.0

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

#4 follow-up: @norcross
4 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 @Denis-de-Bernardy
4 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 @norcross
4 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 @DrewAPicture
4 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

     
    14421442        if ( isset( $_parent_pages[$menu_slug] ) ) { 
    14431443                $parent_slug = $_parent_pages[$menu_slug]; 
    14441444                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 ) ); 
    14461446                } else { 
    1447                         $url = admin_url( 'admin.php?page=' . $menu_slug ); 
     1447                        $url = network_admin_url( 'admin.php?page=' . $menu_slug ); 
    14481448                } 
    14491449        } else { 
    14501450                $url = ''; 

#8 follow-up: @norcross
4 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: @nacin
4 years ago

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

#10 in reply to: ↑ 8 @DrewAPicture
4 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 @norcross
4 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

@norcross
4 years ago

removes network check from existing menu_page_url, adds new network_menu_page_url

#12 @norcross
4 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: @DrewAPicture
4 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.

@norcross
4 years ago

removed new network_page_url function, added self_admin_url to existing

#14 in reply to: ↑ 13 @norcross
4 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 @Denis-de-Bernardy
4 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.

#16 @Denis-de-Bernardy
4 years ago

  • Version changed from trunk to 3.0

/me curses buggy trac js.

#17 @DrewAPicture
3 years ago

  • Milestone changed from 4.0 to Future Release

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

#18 @SergeyBiryukov
3 years ago

  • Component changed from Menus to Plugins
  • Focuses administration added

#19 @SergeyBiryukov
3 years ago

#31805 was marked as a duplicate.

Note: See TracTickets for help on using tickets.