[wp-trac] [WordPress Trac] #12362: wpdb code improvements - nacin / hakre iterations

[WordPress Trac]

#12362: wpdb code improvements - nacin / hakre iterations
--------------------------+-------------------------------------------------
 Reporter:  hakre         |       Owner:  nacin   
     Type:  defect (bug)  |      Status:  accepted
 Priority:  normal        |   Milestone:  3.0     
Component:  General       |     Version:  3.0     
 Severity:  normal        |    Keywords:  featured
--------------------------+-------------------------------------------------
Changes (by nacin):

  * owner:  => nacin
  * status:  new => accepted


Comment:

 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?

-- 
Ticket URL: <http://core.trac.wordpress.org/ticket/12362#comment:1>
WordPress Trac <http://core.trac.wordpress.org/>
WordPress blogging software