WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#11799 closed enhancement (wontfix)

WPDB code improvements

Reported by: hakre Owned by: ryan
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Database Keywords: has-patch
Focuses: Cc:

Description

While taking care about various places over the past weeks I somehow returned back more and more often to WPDB starting to beautify and refactor the code in there.

I create this ticket now to collect changes on wpdb to improve the code.

Related:

  • #6836 - Prepare queries using INSERT/UPDATE should use wpdb::insert() or wpdb::update()
  • #7171 - wpdb::insert() & wpdb::update() need to allow typecasting of params.
  • #11318 - $wpdb->prepare() Chokes on single literal percentage chars?
  • #11372 - wpdb::query returns FALSE on TRUNCATE
  • #11605 - wpdb::_weak_escape() is an alias to addslashes only
  • #11608 - wpdb->prepare() is broken
  • #11649 - Upgrades Fail Because of [11883]

The two most important things while I rewrite some parts of WPDB is code duplication and massive memory leaks in that class.

Attachments (3)

11799-bonus-mysql2date.patch (2.5 KB) - added by hakre 5 years ago.
Bonus refactoring - Orphaned code removed, Docblock update, propper returns in mysql2date()
11799.patch (31.6 KB) - added by hakre 5 years ago.
First one; Fix: Query $type was not case insensitive. Fixed.
11799.2.patch (44.5 KB) - added by hakre 5 years ago.
First one - rev 4; Fix: get_var / hakre; Fix: Comment / Denis; Fix: Query $type was not case insensitive. Fixed.

Download all attachments as: .zip

Change History (17)

@hakre5 years ago

Bonus refactoring - Orphaned code removed, Docblock update, propper returns in mysql2date()

comment:1 @hakre5 years ago

Todays Dev-Meeting feedback: wpmuguru merges in wpdb from WPMU the next day(s). I'll add my patch here when I at least get a first version on.

comment:2 @hakre5 years ago

  • Keywords has-patch added

Finally I'm able to upload a first patch. I did not implement all of the changes I suggested in other patches around so that the changes are not so immersive. The focus is more on the general memory problems WPDB is having and the code quality. I was able to remove duplicate code blocks and to reduce complexity of many functions. The readablity did get a benefit as well. Docblocks have been extended / created and I tried to adopt the code to the coding standards.

Under the hood the class internally now uses arrays to store the resultset. That will lower the required memory. From the outside there is no difference, every public member can be used as it could before.

When was it introduced (compiled this for some @since tags )?

While refactoring, I noticed the following things:

  • #11804 - Object Access Violation in repair.php
  • #11819 - mysql_real_escape_string available now / PHP 4.3 are minimum system requirements since 2.9

@hakre5 years ago

First one; Fix: Query $type was not case insensitive. Fixed.

comment:3 @Denis-de-Bernardy5 years ago

this is a bit odd:

  • initialise return

+ initi return values and objects state

comment:4 follow-up: @Denis-de-Bernardy5 years ago

isn't this missing a delimiter?

if ( preg_match( '(^\s*([a-z]{2,})\s+)i', $query, $matches ) ) 

also, I don't think it'll properly deal with:

query("
# some comments for debugging using SAVEQUERIES
INSERT INTO ...
");

but nor does the current version of the class, so that's probably another ticket...

also while we're at it, for reference, the code in wpmu does not deal with a tabbed update statements properly. (but yours does, since it follows the UPDATE with a \s instead of a space in the regexp.)

comment:5 in reply to: ↑ 4 @hakre5 years ago

Replying to Denis-de-Bernardy:

this is a bit odd: + initi return values and objects state ...

Fixed.

Replying to Denis-de-Bernardy:

isn't this missing a delimiter?

if ( preg_match( '(^\s*([a-z]{2,})\s+)i', $query, $matches ) ) 

No, it's not, delimitter is () and it's valid and nice as well. matches for the group 0 to give a hint :).

also, I don't think it'll properly deal with:

query("
# some comments for debugging using SAVEQUERIES
INSERT INTO ...
");

but nor does the current version of the class, so that's probably another ticket...

jup, would say other dimension, this requires a new algo to copy with that. maybe mysql itself can tell the name of the last command. that could solve it.

also while we're at it, for reference, the code in wpmu does not deal with a tabbed update statements properly. (but yours does, since it follows the UPDATE with a \s instead of a space in the regexp.)

I think that's a valuable hint for wpmuguru.

comment:6 @hakre5 years ago

I added another iteration of the patch. Now all functions and vars should have a properly @since tag. corrected some whitespace issues compared to the previous patch and fixed that comment.

@hakre5 years ago

First one - rev 4; Fix: get_var / hakre; Fix: Comment / Denis; Fix: Query $type was not case insensitive. Fixed.

comment:7 @hakre5 years ago

I updated the patch again because it had a bug in the get_var subroutine.

Related: #11921

comment:8 @Denis-de-Bernardy5 years ago

  • Keywords bug-hunt added

comment:9 @Denis-de-Bernardy5 years ago

  • Keywords featured added; bug-hunt removed

comment:10 @ryan5 years ago

Patch breaks multisite and needs refresh.

comment:11 @hakre5 years ago

Referenced: #10607

comment:12 @hakre5 years ago

  • Milestone changed from 3.0 to Future Release

Patch is stale and does not support multisite. I now merged my changes against the current trunk (which is multisite) so there still is some benefit of the work. I've attached it to #11644.

I'll put this ticket on future release because right now it's more important to get the multisite feature done.

comment:13 @hakre4 years ago

Related: #12257

comment:14 @nacin4 years ago

  • Keywords featured removed
  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

http://wpdevel.wordpress.com/2011/03/23/code-refactoring/

This ticket is horribly stale. Closing.

Note: See TracTickets for help on using tickets.