WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 21 months ago

Last modified 21 months ago

#18510 closed enhancement (fixed)

Class properties dbpassword, dbname, dbhost and dbh are undefined in wpdb

Reported by: joelhardi Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: minor Version: 3.3
Component: Database Keywords: has-patch
Focuses: Cc:

Description

The class members $dbuser, $dbpassword, $dbname, $dbhost and $dbh are set by wpdb's constructor but only one of these ($dbuser, since 2.9.0) is declared with a var statement.

I think these should be explicitly declared, both for basic documentation and to declare their visibility so developers know what they can use. I know in the past there have been issues with developers using supposedly private properties/methods, which then turns into a defacto API that is hard to deprecate. In this case, these properties aren't marked private and are just magically created in __construct without any documentation as far as I can tell.

Personally I don't care whether they are declared using var and marked as private or declared as private/protected. I would prefer explicit private but the rest of wpdb still looks PHP4 so I've used var statements in the attached patch.

What I propose is for these properties to be private, since they are passed at runtime to the constructor and immediately used to establish a database connection ($dbh). Subsequently changing the value of $wpdb->dbhost, for example, doesn't alter the existing database connection ($wpdb->db_connect() would have to be re-run to do that). Also, $dbuser is already labeled private so this is consistent.

Although private, I think these properties could/should have getters so that they're effectively read-only public. In particular, there needs to be a getter for the database connection handle $dbh (I'm a plugin author and I currently grab $wpdb->dbh to access the database) if it's going to be private. In the patch I've added a getter (get_db_connect(), but I don't really care what the method is called).

A little more background: In an analogous situation, previously I used $wpdb->prefix to retrieve the WordPress table prefix, but since 3.0 there is a getter, get_blog_prefix(), for this (and simply retrieving $wpdb->prefix isn't compatible with multisite, so that "old" API is broken). So this is why I suggest giving wpdb a clearer API. Other people may have suggestions, and if the task is blessed I could give wpdb a deeper look (another thing that might be helpful would be to add an interface). But I thought I'd submit this minor enhancement first.

Attachments (5)

wp-db.php.18596.diff (1.0 KB) - added by joelhardi 3 years ago.
adds var statements, getter for db connection handle
wp-db.php.diff (1.1 KB) - added by nvartolomei 3 years ago.
adds var statements with visibility
18510.diff (1.1 KB) - added by ryan 2 years ago.
Refreshed
18510.2.diff (924 bytes) - added by pento 21 months ago.
wpdb-magic.diff (1.4 KB) - added by pento 21 months ago.

Download all attachments as: .zip

Change History (19)

joelhardi3 years ago

adds var statements, getter for db connection handle

nvartolomei3 years ago

adds var statements with visibility

comment:1 nvartolomei3 years ago

  • Cc me@… added

Since WordPress from version 3.2 supports PHP 5.2+, I think it's time to set visibility to wp-db members, maybe it's time to refactor entire wp-db class and to add visibility to all members?

comment:2 follow-up: joelhardi3 years ago

I totally agree with you, but that should probably be submitted as a separate issue ... you can create a new ticket and link to it here.

I think it's better to keep this issue focused on adding the docstrings and variable declarations for the missing vars.

WP core team has a particular strategy for when they want to move things to PHP5 (i.e. only when it really adds something, not just to break compatibility with PHP4), and I don't think that discussion makes sense here where there's a much narrower bug (missing vars).

Also, in your patch you might as well replace all the var statements with whatever the visibility is supposed to be (since this info is already in the doc strings) ... no point in doing it halfway, mixing var with public/protected/private is just going to confuse anyone else working on the file. Like I said, personally I agree with you and support your patch, the sooner WP starts enforcing member visibility the better IMHO.

comment:3 in reply to: ↑ 2 nvartolomei3 years ago

Replying to joelhardi:

I totally agree with you, but that should probably be submitted as a separate issue ... you can create a new ticket and link to it here.

I added it just to get things moving. Let's wait for reply from somebody related to core team, if they are happy with adding visibility rules, I'm ready to make a patch.

comment:4 joelhardi3 years ago

Sure ... I'm just saying you're more likely to get a response if you submit a new ticket with a full patch. Adding as a comment on a mostly unrelated ticket is usually not going to get you any traction. It works to your advantage and keeps discussion here from spiraling off topic. My ticket may go nowhere whereas yours might. Or, if my conservative patch is applied, this ticket is closed and your idea/work is buried.

My ticket/patch doesn't change any functionality, yours does, that's why they're not the same thing. My goal is to fill in some missing documentation, yours is to move WP to PHP5 (or something like that).

You have to understand that enforcing visibility will break many plugins, the issue isn't PHP4 vs. PHP5. WordPress is all about having things "just work" and generally not about breakage for its own sake. A change to the API like yours that removes access has to be phased in gradually and broadcast to the community. (For example, one strategy would be to add an accessor method 12 months before enforcing private access to a member property.) Anyway ... this is the discussion I was trying to avoid so I'll cut it off there. Of course it's up to you, just trying to advise you about how things normally work with core.

ryan2 years ago

Refreshed

comment:5 ryan2 years ago

Refreshed patch.

comment:6 nacin2 years ago

What's the use case for grabbing the DB connection handle from a plugin?

pento21 months ago

comment:7 pento21 months ago

Refreshed patch. You probably shouldn't be using the DB handle directly, but we're not going to stop you. :)

I've removed the getter in this ticket, it'll be available as $wpdb->dbuser, etc, thanks to the magic getter in #20838.

comment:8 nacin21 months ago

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

In [21473]:

Declare, document, and protect $dbuser, $dbpassword, $dbname, $dbhost and $dbh in wpdb.

These properties, while protected, are still accessible thanks to the magic getter added in [21472].

props pento, nvartolomei, joelhardi. fixes #18510.

comment:9 SergeyBiryukov21 months ago

  • Milestone changed from Awaiting Review to 3.5

comment:10 hew21 months ago

Just a heads up, r21473 broke some unit tests we were running. This unset() started PHP fataling after the change:

unset( $wpdb->dbh ); 

Westi suggests a general plugin scan for similar possible breakage.

comment:11 nacin21 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I chatted about this with pento after [21473] landed.

I think it is really nice to declare things as protected so people know what is protected, and then expose them (as necessary from a pre-protected standpoint) with __get(). But this also means we should probably expose them with __set(), __unset(), and __isset().

I do think that declaring things as protected and including these magic methods outweighs "var" and no magic methods.

Last edited 21 months ago by nacin (previous) (diff)

pento21 months ago

comment:12 pento21 months ago

  • Keywords dev-feedback removed

wpdb-magic.diff adds the magic functions to wpdb. I also renamed the parameter for __get() to watch the PHP docs.

Version 0, edited 21 months ago by pento (next)

comment:13 nacin21 months ago

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

In [21512]:

Add magic set, isset, and unset to wpdb. props pento.

These magic methods allow us to mark properties as protected or private, without breaking compatibility, as they were once accessible. The joys of PHP4.

fixes #18510.

Note: See TracTickets for help on using tickets.