wpdb code improvements

[hakre]

In Ticket #11644 we started to improve the general code qualitity of wpdb in multiple iterations. We continue here to not clutter the main ticket up any longer.

[nacin]

The title's fine, though I don't really need the self aggrandizing. Anyway, I'm uploading your most recent patch. Here is your most recent comment:

---

Thanks for the feedback. For the version number lookup, that's a job I'll do, no need for you to care. I thought for the same for sorting it, I just wanted to tell you about the Idea. But that's fine anyway now, so I do not need to do it.

Here is a new numbered list, some older unfixed are still on it, just ignore if you do not want to care, I keep them for reference. There are three or four more major points left over, for other just give me a hint what you prefer, sometimes I asked questions, would be great if you can answer them so I know better for upcomming patches we do together.

1. UNFIXED destruct() breaks with PHP5 defition of the return value (should be void, is true)
2. UNFIXED: in the variable definitions, the meta tables sometimes precede their main table. Also tables are not sorted by their names. Let me know if you would like to see that fixed.
3. UNFIXED: I'm not sure wether unsetting members of an array while iterating it is save with all the PHP version we aim to unset( $tables[ $k ] );.
4. QUERSTION: you've set access to wpdb::$blogid to public. why? should other parts of the program overwrite the blogid for a certain functionality? Same for wpdb::$siteid. A public setter for both is wpdb::set_blog_id(). Maybe a get_blog_id() function with an optional parameter so that it's a getter for siteid as well?
5. QUESTION: you skip the note about the problem of registering the wpdb instance with the register shutdown function call prevents it from unloading as expected. What's the problem with leaving a note there? Same for the hints given in wpdb::desctruct(). I found those thoughtfull so we at least have the discrepancies documented.
6. QUESTION: What's the problem with the scope 'now+old'? I mean there are at least two points in wpdb that will benefit from it.
7. QEUSTION: What's the problem with listing the scope values inside the docblock. I think that's easier to read than inside the @param description. And easier to maintain.
8. wpdb::tables() did not return an array as specified for certain scope values (the undefined ones were forgotten). Patched, will prevent notices/warnings inside the function as well.
9. There is no need to cast an array as array. wpdb::tables() always returns an array, you know that - well okay, not it is.

10. Since you did not want to have the merge inside wpdb::tables() (for a reason I do not understand), array_merge still actually works. I prefer to encapsulate that inside tables(), really.
11. All parameters in tables() are optional. @param should reflect that.
12. Scopes can be sorted by alphabet in the switch construct in wpdb::table()
13. @uses does not fit here, please use @see as already suggested in wpdb::_weak_escape().
14. INFO: for the same class you do not need to prefix wpdb:: for @see and @uses. Additionally PHP function names can be used with @see as well.
15. QUESTION: Sometimes @-tags are aligning their values, sometimes not. Any Idea? Normally alignment is missing in core code, so with these iteration I removed those where I saw them.
16. MINOR: If in for single quotes on 'foo', should be in for single quotes on 'SELECT... as well, right?
17. There is a problem with the preg_replace call in wpdb::prepare() reported on another ticket. I suggest to keep the old behaviour.
18. QUESTION: Since when is $this always a reference? Since the beginning or has that changed later?
19. SKIPPED: I did not care about error_log now. I'll do that later. I saw you changed something there, my Idea was to check against the base reference. I suggest to keep that the old code until we have fully clarified that to not break behaviour.
20. It's toally clear that some queries are made prior to load the plugin API. No need to comment, right? Or if so, why not comment the other parts I find totally clear as well (e.g. preventing the unload)
21. INFO: wpdb::insert( 'table', array( 'column' => 'foo', 'field' => 1337 ) ) was the original example. I just added one, I just saw no need to edit it (FYI).
22. INFO: I still think that constants are normally written uppercase, but I dropped that for this patch to reduce the FUZZ. This needs to be clarified elsewere anyway I assume.
23. There still was a parameter documented that does not exists in wpdb::has_cap(), you've forgotten a line to remove or was that by intention?
24. You made a mistake in changes applied to wpdb::get_caller(), $caller is not filled with values any longer. A line was missing. You can expand it if you like using a temporary variable like $function, but I just patched it.
25. Additionally I moved the join next to return (as always suggested) in wpdb::get_caller().
26. Another parameter that does not exists documented in wpdb::db_version() (see 23.)
27. QUESTION: Include files to not need control characters after the closing ?> at the end, right?

[nacin]

Replying to hakre:

1. __destruct() return value (should be void, is true)

Doesn't really matter though.

2. in the variable definitions, the meta tables sometimes precede their main table.

Sorted by names (which means "commentmeta" comes before "comments"), but I pushed multisite to the end. I think we're good.

3. I'm not sure wether unsetting members of an array while iterating it is save with all the PHP version we aim to unset( $tables[ $k ] );.

This should be no problem as-is.

4. you've set access to wpdb::$blogid to public.

We use it publicly during the MS loading process. I don't think it's necessary to create setters.

5. you skip the note about the problem of registering the wpdb instance with the register shutdown function call prevents it from unloading as expected. What's the problem with leaving a note there? Same for the hints given in wpdb::desctruct(). I found those thoughtfull so we at least have the discrepancies documented.

Not sure I really follow or see the need. We register it for PHP4, I imagine. No reason to document this.

6. QUESTION: What's the problem with the scope 'now+old'? I mean there are at least two points in wpdb that will benefit from it.

It's bulky and less clear at what is going on. The alternative, array_merge() on this->tables(blog) and this->tables(old) before looping through, seems unnecessary and also less clear at what is going on. (It's also slower, technically.) Currently it is very clear that we're setting up global tables, then blog tables, then the old tables.

7. What's the problem with listing the scope values for table inside the docblock.

We can move it into the docblock.

8. wpdb::tables() did not return an array as specified for certain scope values (the undefined ones were forgotten). Patched, will prevent notices/warnings inside the function as well.

Ok, I think we can just add default: return array(); . It only accesses private properties otherwise.

9. There is no need to cast an array as array. wpdb::tables() always returns an array, you know that - well okay, not it is.

Sure, we can pull those out.

10. Since you did not want to have the merge inside wpdb::tables() (for a reason I do not understand), array_merge still actually works. I prefer to encapsulate that inside tables(), really.

Ok, we don't have any merges outside of wpdb::tables() (see point 6).

11. All parameters in tables() are optional.

Ok.

12. Scopes can be sorted by alphabet in the switch construct in wpdb::table()

Sure.

13. @uses does not fit here, please use @see as already suggested in wpdb::_weak_escape().

Ok.

14. for the same class you do not need to prefix wpdb:: for @see and @uses.

Sounds about right.

15. Normally alignment is missing in core code, so with these iteration I removed those where I saw them.

Sure.

16. If in for single quotes on 'foo', should be in for single quotes on 'SELECT... as well, right?

I would rather not put queries in single quotes, because when a plugin author does that and uses a $wpdb->{table} reference, it doesn't work.

17. There is a problem with the preg_replace call in wpdb::prepare() reported on another ticket. I suggest to keep the old behaviour.

[13357]

18. Since when is $this always a reference? Since the beginning or has that changed later?

From the PHP 4 manual: "In PHP4, it was necessary to use a reference to create a callback that points to the actual object, and not a copy of it."

19. I did not care about error_log now. I'll do that later. I saw you changed something there, my Idea was to check against the base reference. I suggest to keep that the old code until we have fully clarified that to not break behaviour.

I went over the logic step by step -- my head hurt when I was done, as did westi's -- and I'm pretty sure I cleaned it up while keeping the same behavior.

20. It's toally clear that some queries are made prior to load the plugin API. No need to comment, right?

It's clear to you and me, but not to a run-of-the-mill plugin author. We only check whether the plugin API is loaded four times total.

21. INFO: wpdb::insert( 'table', array( 'column' => 'foo', 'field' => 1337 ) ) was the original example.

I know, I decided to change it to clarify how field types are handled.

22. INFO: I still think that constants are normally written uppercase, but I dropped that for this patch to reduce the FUZZ. This needs to be clarified elsewere anyway I assume.

Across core we consistently use true|false|null, not TRUE|FALSE|NULL, especially in inline docs. It reads better anyway.

23. There still was a parameter documented that does not exists in wpdb::has_cap(), you've forgotten a line to remove or was that by intention?

Ok, we can remove.

24. You made a mistake in changes applied to wpdb::get_caller(), $caller is not filled with values any longer. A line was missing. You can expand it if you like using a temporary variable like $function, but I just patched it.

Looks like I fudged the merge there. Will fix.

25. Additionally I moved the join next to return (as always suggested) in wpdb::get_caller().

See point 24.

26. Another parameter that does not exists documented in wpdb::db_version() (see 23.)

Ok.

27. QUESTION: Include files to not need control characters after the closing ?> at the end, right?

Pretty sure the correct way is the way it is. (We see editors trying to munge the end-of-file line endings all the time, hence why it's in the patch.)

[automattor]

(In [13386]) update wpdb blogid using setter, See #12362

[hakre]

Another patch now containing the missing version numbers. This time w/o a list, should be pretty straight-forward to apply. After this is comitted I'll create a last list as summary so unfixed stuff / related tickets won't get lost documentation wise.

[hakre]

Patch updated because of missing whitespace fixes.

[miqrogroove]

27. QUESTION: Include files to not need control characters after the closing ?> at the end, right?

Pretty sure the correct way is the way it is. (We see editors trying to munge the end-of-file line endings all the time, hence why it's in the patch.)

When necessary:

return;
?>

I never leave PHP tags open.

[nacin]

This looks good, though we need to leave $blog_id == 0 there. Thanks a lot for the version numbers, I know how annoying those are to do.

[nacin]

(In [13426]) @since version numbers and other docs for wp-db. props hakre see #12362

[nacin]

(In [13578]) Fix notice. See #12362