WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#31556 closed enhancement (wontfix)

`$wpdb` global object should be retrieved via a helper function to help prevent global stomping

Reported by: jtsternberg Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Database Keywords: has-patch
Focuses: Cc:

Description

As a matter of convenience and even to provide a small amount of resilience to the $wpdb global, it makes sense that we provide a utility to function to return it. This would allow things like:

wpdb()->get_col( wpdb()->prepare() );

etc.

the entirety of the patch looks like:

function wpdb() {
	global $wpdb;
	return $wpdb;
}

Attachments (3)

31556.diff (593 bytes) - added by jtsternberg 4 years ago.
31556-2.diff (592 bytes) - added by jtsternberg 4 years ago.
fix typo in comment
31556-3.diff (587 bytes) - added by jtsternberg 4 years ago.
Switch to using $GLOBALS['wpdb']

Download all attachments as: .zip

Change History (19)

@jtsternberg
4 years ago

#1 @jtsternberg
4 years ago

  • Keywords has-patch 2nd-opinion added

#2 @johnjamesjacoby
4 years ago

I like this. It minimizes the risk of interacting with the $wpdb global object directly.

I've been including methods in classes to do something similar for a while, so a central wrapper in WordPress core would prevent me from redefining those methods for each class that interacts with it.

#3 @paulwilde
4 years ago

The patch has a typo, by the way. "wbdp".

@jtsternberg
4 years ago

fix typo in comment

#4 @pento
4 years ago

I'm not wild about this. It seems like it'd involve a lot of churn in both core (if we're providing this, we should use it), and in plugins, for when their minimum version catches up to this.

The only thing it prevents is clobbering $wpdb, which you're going to notice pretty quickly. It doesn't prevent more subtle errors which are harder to catch. (For example, changing $wpdb->charset.)

Also, is this proposal just for $wpdb, or all of the globals WordPress provides?

#5 @jtsternberg
4 years ago

It is a proposal for only $wpdb for now. Re: churn vs global stomping, I think the pros outweigh the cons, but ultimately that is not my call. I'd prefer to never call global $anything; in my code again, so something like this starts to get us there.

#6 follow-up: @jdgrimes
4 years ago

I realize performance may not be a big concern here, but FWIW, a function call takes over twice the amount of time as declaring a global. We might want to consider just replacing global $wpdb with $wpdb = wpdb(). Less churn, no global, more performant.

#7 in reply to: ↑ 6 @jdgrimes
4 years ago

Replying to jdgrimes:

I realize performance may not be a big concern here, but FWIW, a function call takes over twice the amount of time as declaring a global. We might want to consider just replacing global $wpdb with $wpdb = wpdb(). Less churn, no global, more performant.

To clarify, what I'm saying is that replacing every single use of the $wpdb variable with a call to a function will reduce performance. Calling the function just once, and using the variable as we already do will be more performant and won't require the code churn.

#8 follow-up: @jtsternberg
4 years ago

Along those lines, I'm curious if there are any performance implications to global $wpdb; return $wpdb; vs return $GLOBALS['wpdb'];.

#9 in reply to: ↑ 8 @jdgrimes
4 years ago

Replying to jtsternberg:

Along those lines, I'm curious if there are any performance implications to global $wpdb; return $wpdb; vs return $GLOBALS['wpdb'];.

return $GLOBALS['wpdb']; is about 15% faster, based on a similar benchmark I ran a while back.

@jtsternberg
4 years ago

Switch to using $GLOBALS['wpdb']

#10 @ericlewis
3 years ago

Tangentially related: #32468

This ticket was mentioned in Slack in #core by boone. View the logs.


3 years ago

#12 @boonebgorges
3 years ago

  • Component changed from Query to Database

#13 @pento
3 years ago

  • Keywords 2nd-opinion removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed
  • Version 4.2 deleted

I still have the reservations I expressed in comment #4. Lots of code churn for only a little extra protection doesn't seem like a good tradeoff.

#14 @pento
3 years ago

#37639 was marked as a duplicate.

#15 @pcfreak30
3 years ago

I think what @jdgrimes said is a good compromise for compatibility and performance though.

This ticket was mentioned in Slack in #core by drew. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.