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

Comment(by nacin):

 Replying to [comment:1 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.)

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