r89409 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r89408‎ | r89409 | r89410 >
Date:11:43, 3 June 2011
Author:freakolowsky
Status:reverted (Comments)
Tags:
Comment:
* suggestion for using tables.sql parsing instead of database query for tables list needed for phpunit
* would be better option IMO since the database is not necesserally dedicated to MW install
Modified paths:
  • /trunk/phase3/includes/db/Database.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/Database.php
——@@ -2445,7 +2445,12 @@
24462446 * @param $fname String: calling function name
24472447 */
24482448 function listTables( $prefix = null, $fname = 'DatabaseBase::listTables' ) {
2449 - throw new MWException( 'DatabaseBase::listTables is not implemented in descendant class' );
 2449+ global $IP;
 2450+ //throw new MWException( 'DatabaseBase::listTables is not implemented in descendant class' );
 2451+ $tables = file_get_contents( "$IP/maintenance/tables.sql" );
 2452+ preg_match_all('/create table \/\*_\*\/([a-z0-9_]*)/i', $tables, $matches, PREG_PATTERN_ORDER);
 2453+
 2454+ return $matches[1];
24502455 }
24512456
24522457 /**

Sign-offs

UserFlagDate
Hasharinspected12:05, 3 June 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r89429Revert r89409: provided a default Database->listTables() implementation that ...brion18:39, 3 June 2011

Comments

#Comment by Freakolowsky (talk | contribs)   11:45, 3 June 2011

Keep in mind that db-specific overloads have to be fixed too.

#Comment by Hashar (talk | contribs)   12:05, 3 June 2011

Maybe the method should be marked final?

#Comment by Freakolowsky (talk | contribs)   12:20, 3 June 2011

Well i was thinking more in the direction of protected ... always better to leave "the door open" for extending classes to allow db-specific tricks.

Anyways i'd like to see some more comments/opinions on this topic.

#Comment by MaxSem (talk | contribs)   14:28, 3 June 2011

I disagree, there may be extension tables that we might want to duplicate.

#Comment by Freakolowsky (talk | contribs)   14:37, 3 June 2011

TRUE, but those tables could "register" their tables trough a hook like with ExtensionUpdater (also work in progress).

The final product of this thought pattern would be somthing like:

  • get standard tables from tables.sql (parsing is easy and only one point to maintain)
  • as the processing passes back trough db-specific overload it might want to change some things (not sure what ATM, but let's leave the option)
  • run hooks like the updater does and ask extensions that are "smart enough" to give their list of tables
  • do the disco-hustle

PS: damn you're trigger-happy on the 'fixme' button.

#Comment by MaxSem (talk | contribs)   14:53, 3 June 2011

This is a huge overcomplication for a superficial reason. For databases shared between MW and third-party apps there is database prefix - those who chose not to use it are already doomed for reasons unrelated to unit tests. Besides, nobody runs tests on production systems where we can expect shared databases. In any case, a fixme is warranted because some extensions already use both tests and extra tables (example: FlaggedRevs). They are broken or may be broken at any moment they may want to access database during tests.

#Comment by Freakolowsky (talk | contribs)   15:07, 3 June 2011

reading a file and triggering hook run is such an overcomplication ... you mean, unlike querying the DB, stripping existing prefix, adding new one, figuring out which tables are db-specific and should not be duped ... i think we have different views on the word superficial.

#Comment by MaxSem (talk | contribs)   15:21, 3 June 2011

Yes, current soultion does not require extensions to do extra stuff, your requires adding another hook to all extensions.

#Comment by Freakolowsky (talk | contribs)   15:35, 3 June 2011

erm ... all extensions that have custom tables an use unit test ... do not generalize here. And there are how many of those?

#Comment by 😂 (talk | contribs)   15:44, 3 June 2011

Don't see why we need a new hook, we should be able to make use of LoadExtensionSchemaUpdates here. I also disagree with your statement MaxSem that "people not using prefixes are doomed to failure"

Lets say you don't use a prefix on your tables, MediaWiki is in a DB *by itself.* You ran the unit tests (not with temporary tables, as I was day before yesterday to confirm some stuff). Subsequent runs would also duplicate your unittest_ tables. Edge case, sure...but checking tables.sql (and using our already-available-hook) prevents that and has us only duplicating tables we need.

#Comment by Brion VIBBER (talk | contribs)   18:40, 3 June 2011

Reverted in r89429; this implementation isn't compatible with DatabaseMySQL's implementation. Several differences:

  • listTables is expected to return tables that actually exist in the database -- this returns a fixed set of tables that MediaWiki core knows about
  • listTables is expected to return actual raw table names, including whatever prefix -- this wouldn't include a prefix
  • listTables $prefix parameter can be used to restrict which tables are pulled from the list, but this isn't respected here

Status & tagging log