WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#14842 closed enhancement (fixed)

Efficiency improvement for is_super_admin()

Reported by: 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 4 years ago.
14842.2.diff (807 bytes) - added by mdawaffe 4 years ago.
14842.patch (415 bytes) - added by hakre 4 years ago.
PHP4
14842.next-iteration.patch (1.6 KB) - added by hakre 4 years ago.
+ WS fix / and two docblock changes

Download all attachments as: .zip

Change History (12)

mdawaffe4 years ago

mdawaffe4 years ago

comment:1 follow-up: nacin4 years ago

  • Milestone changed from Awaiting Review to 3.1

site_admins is a wpcom thing. Otherwise, looks good.

comment:2 in reply to: ↑ 1 mdawaffe4 years ago

Replying to nacin:

site_admins is a wpcom thing. Otherwise, looks good.

Yeah - bad patch. New one is better.

comment:3 nacin4 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.

hakre4 years ago

PHP4

comment:4 hakre4 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?

hakre4 years ago

+ WS fix / and two docblock changes

comment:5 hakre4 years ago

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

comment:6 follow-up: nacin4 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.

comment:7 in reply to: ↑ 6 hakre4 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?

comment:8 nacin3 years ago

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