Make WordPress Core

Opened 14 years ago

Closed 13 years ago

#14842 closed enhancement (fixed)

Efficiency improvement for is_super_admin()

Reported by: mdawaffe's profile mdawaffe Owned by:
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.0.1
Component: Multisite Keywords: has-patch
Focuses: Cc:

Description

is_super_admin() calls new WP_User() needlessly when running the test against the current user.

Attached cleans up the logic a bit.

Note: this inefficiency can cause an infinite recursion in certain (strange) scenarios when running HyperDB.

Attachments (4)

14842.diff (832 bytes) - added by mdawaffe 14 years ago.
14842.2.diff (807 bytes) - added by mdawaffe 14 years ago.
14842.patch (415 bytes) - added by hakre 14 years ago.
PHP4
14842.next-iteration.patch (1.6 KB) - added by hakre 14 years ago.
+ WS fix / and two docblock changes

Download all attachments as: .zip

Change History (12)

@mdawaffe
14 years ago

@mdawaffe
14 years ago

#1 follow-up: @nacin
14 years ago

  • Milestone changed from Awaiting Review to 3.1

site_admins is a wpcom thing. Otherwise, looks good.

#2 in reply to: ↑ 1 @mdawaffe
14 years ago

Replying to nacin:

site_admins is a wpcom thing. Otherwise, looks good.

Yeah - bad patch. New one is better.

#3 @nacin
14 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [15608]) More efficient is_super_admin(). Don't call new WP_User on the current user. props mdawaffe, fixes #14842.

@hakre
14 years ago

PHP4

#4 @hakre
14 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks for the initiative. In full respect of the close and honoring the fact that the current patch already improves the named topic, I gently re-open this ticket because of some minor stuff:

It's only a small issue, but following the PHP 4 related wordpress coding practices, the change makes no use of &new here which is common to prevent object clones on PHP4 installs.

I've added a patch that is reflecting this _really_ minor thing ($user is a local variable, so the copy here is limited to one per call until the garbage collector removes it again).

If it's considered to ignore the &new here, I could greatly appreciate that. For such alternative case, I've attached a second view on the function.

The second view might me interesting for the &new case as well but it needs some changes then.

Let me know if I should open a new ticket instead of re-opening this one.

As this ticket is about refactoring that function I thought it's worth to add some view. Becasue this is all minor I didn't want to clutter trac with a new ticket as there was already one.


A related question: Why don't we add a is_super_admin() function to the user class?

@hakre
14 years ago

+ WS fix / and two docblock changes

#5 @hakre
14 years ago

Just two additional changes to docblocks in that file I ran over and some ws fixes.

#6 follow-up: @nacin
14 years ago

I'm confused why your refactoring patches lately are overusing language syntaxes and styles that we would never leverage in core for readability purposes. (#14838)

You also missed the & in your second patch, which is no longer possible due to the syntax change.

We don't do by reference for new WP_User objects anywhere.

#7 in reply to: ↑ 6 @hakre
14 years ago

Replying to nacin:

I'm confused why your refactoring patches lately are overusing language syntaxes and styles that we would never leverage in core for readability purposes. (#14838)

Ah, interesting.

You also missed the & in your second patch, which is no longer possible due to the syntax change.

Not a miss, as explained, I preferred to not use it.

We don't do by reference for new WP_User objects anywhere.

Is there a difference between objects by their class?

#8 @nacin
13 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.