WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 15 months ago

Last modified 15 months ago

#16490 closed enhancement (wontfix)

Rename is_x_admin() functions to in_x_admin()

Reported by: apeatling Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

The functions is_admin(), is_blog_admin(), is_network_admin() and is_user_admin() all share the same problem -- they read like they are checking user capabilities.

This is made worse with genuine user capability checking functions like is_super_admin().

When writing code utilizing these functions it's simple to confuse other developers since they'd automatically assume that if ( is_user_admin() ) {} is checking if a user is an admin.

Since all of these functions other than is_admin() are new in 3.1, I'm hoping we can avoid compounding the problem in the next release and change them all to in_x_admin() since this more accurately reflects what the functions are doing -- checking if the request is inside an admin page.

I've attached a patch with the changes needed. I've also deprecated the is_admin() function and replaced it with in_admin().

Attachments (3)

in_admin.patch (41.4 KB) - added by apeatling 3 years ago.
in_admin_inc_deprecated.patch (42.1 KB) - added by apeatling 3 years ago.
Patch including deprecated is_x_admin() functions.
16490.diff (41.9 KB) - added by ryan 3 years ago.
Put deprecated functions in load.php

Download all attachments as: .zip

Change History (23)

apeatling3 years ago

comment:1 westi3 years ago

I'm on the fence for this change.

It is probably too late to do this now we are RC without adding the backcompat for the new functions in deprecated.php

Otherwise we are going to break any new code people have written during the beta/RC period which uses these functions.

So all those people who have updated there Multisite plugins to work with the network admin will be sad

comment:2 apeatling3 years ago

True westi, but it'll be fraction of the number compared with when 3.1 goes stable. I'm sure they won't mind a quick search and replace? I wish I could have flagged this earlier though, I've only come across it now.

comment:3 apeatling3 years ago

The patch also adds is_admin() into deprecated.php, so it's no hassle to move the others, just not ideal since they wouldn't even have made a final version.

comment:4 ryan3 years ago

It's RC4. We're stuck with them.

comment:5 ryan3 years ago

I might get behind moving all of the is_*_admin() functions to deprecated.php without a warning. We can't disappear these functions post RC4.

apeatling3 years ago

Patch including deprecated is_x_admin() functions.

comment:6 ryan3 years ago

Looks good. We can remove the warning from is_admin() on commit.

comment:7 ryan3 years ago

The deprecated versions might need to stay in load.php for load order reasons. That would be safest for 3.1, anyhow.

comment:8 scribu3 years ago

Since we have to deprecate them anyway, can't this wait until WP 3.2?

comment:9 nacin3 years ago

The other option, too, is we had discussed is_admin( $where = true ) with the option to specify 'user' or 'network'. Not that we plan on adding more admins beyond these, just throwing it out there.

I agree with scribu, also. Is it confusing? Yeah. But it's been building up to this for a while, so let's just sit on it.

is_admin() is actually in_admin()

is_blog_admin() is actually in_blog_admin()

is_site_admin() is actually is_super_admin()

is_network_admin() is actually in_network_admin()

is_super_admin() is the new name for is_site_admin()

ryan3 years ago

Put deprecated functions in load.php

comment:10 ryan3 years ago

I'm fine with waiting. The patch is here when we want it.

comment:11 nacin3 years ago

  • Keywords 3.2-early added
  • Milestone changed from Awaiting Review to Future Release

comment:12 scribu3 years ago

I like the is_admin( $where ) idea. You don't have to learn about new functions.

comment:13 apeatling3 years ago

If we're deprecating the functions anyway this can definitely wait. Although on the flip side it will expose functions that are possibly already deprecated to a significantly greater number of developers.

I'm not sure about is_admin( $where ) though. It doesn't fix the problem for me: if ( is_admin( 'user' ) ) {} is just as confusing.

comment:14 scribu3 years ago

in_admin( $where ) then. Best of both worlds.

comment:15 follow-up: apeatling3 years ago

+1 for in_admin( $where ).

comment:16 westi3 years ago

-100 for in_admin( $where ).

  1. We don't envisage adding any more $wheres in the near term
  2. It makes the code harder to read, scan for relevant checks etc.
  3. It doesn't really make things less confusing than in_admin() / in_user_admin() / in_network_admin()

comment:17 bananastalktome21 months ago

  • Cc bananastalktome@… added

comment:18 in reply to: ↑ 15 bpetty15 months ago

  • Cc bpetty added
  • Keywords close added; 3.2-early removed

Replying to apeatling:

+1 for in_admin( $where ).

If you prefer this style, you can actually now use $current_screen->in_admin($where) as WP_Screen was changed to do exactly this in 3.5 (see #21742).

As for the is_*_admin() methods, this ticket was totally ignored for 2 years, and those have been in use for a long time now, so now I really think this patch is completely pointless now, and not worth doing.

Proposing to close this ticket.

comment:19 scribu15 months ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Yeah, this ship has saled.

comment:20 bpetty15 months ago

  • Keywords close removed
Note: See TracTickets for help on using tickets.