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

[joelhardi]

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.

[joelhardi]

adds var statements, getter for db connection handle

[nvartolomei]

adds var statements with visibility

[nvartolomei]

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?

[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 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.

[nvartolomei]

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.

[joelhardi]

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.

[ryan]

Refreshed

[ryan]

Refreshed patch.

[nacin]

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

[pento]

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.

[nacin]

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.

[hew]

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.

[nacin]

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.

[pento]

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

[nacin]

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.

[nacin]

​[UT971]