db_find_tables() expects tables to be prefixed, inconsistent implementation and documentation [#851168]

The documentation of db_find_tables() is wrong:

/**
 * Finds all tables that are like the specified base table name.
 *
 * @param $table_expression
 *   An SQL expression, for example "simpletest%" (without the quotes).
 *   BEWARE: this is not prefixed, the caller should take care of that.
 *
 * @return
 *   Array, both the keys and the values are the matching tables.
 */

The base implementation of this (DatabaseSchema::findTables()), used by MySQL and PostgreSQL, pipes the $table_expression through buildTableNameCondition(), and then to getPrefixInfo(). As a consequence, $table_expression is *actually* prefixed.

More over, ModuleTestCase relies on this to verify that modules are installed correctly. As a consequence those tests fail on SQLite, which doesn't prefix $table_expression.

Let's pick one. Critical for the failure on SQLite.

Comment	File	Size	Author
#8	851168-reroll.patch	6.27 KB	Stevel
#8
#7	851168-param-default.patch	6.27 KB	Stevel
#7
#5	851168-leave-unprefixed-alone.patch	6.26 KB	Stevel
#5
#3	851168-find-tables.patch	1.67 KB	Stevel
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Damien Tournoud CreditAttribution: Damien Tournoud commented 11 July 2010 at 23:23

This was changed by #302327: Support cross-schema/database prefixing like we claim to. I originally made $table_expression not prefixed for a reason: it's *really hard* to support both prefixes and a LIKE expression.

This function was supposed to be used for one thing, and one thing only, the "clean-up" feature of Simpletest. I would recommend we remove that, and try to find a better solution for Simpletest. The Module tests should query the Drupal schema, not the SQL tables directly.

Comment #2

Stevel CreditAttribution: Stevel commented 12 July 2010 at 10:13

I would go for changing the documentation, as it's more consistent with other DBTNG functions that prefix the table name as well.

Comment #3

Stevel CreditAttribution: Stevel commented 12 July 2010 at 10:19

Status:

Active

» Needs review

File	Size
851168-find-tables.patch	1.67 KB

And here's a patch. The simpletest cleanup currently assumes that the table name must be prefixed, so changed that too.

Comment #4

Damien Tournoud CreditAttribution: Damien Tournoud commented 12 July 2010 at 10:37

@Stevel: yes, but in that case, this function is *completely* broken.

Suppose you have:

$prefixes = array(
  'default' => 'default_',
  'users' => 'shared_',
);

Then, db_find_tables('users%'); and db_find_tables('user%'); will never find anything. Only db_find_tables('users'); will find the table.

Comment #5

Stevel CreditAttribution: Stevel commented 12 July 2010 at 14:27

File	Size
851168-leave-unprefixed-alone.patch	6.26 KB

The other way around: make sure that the table name is not prefixed for db_find_tables().

db_find_tables (or at least findTables()) needs to stay within the database abstraction layer because the listing tables is database-dependent.

Comment #6

12 July 2010 at 15:00

Status:

Needs review

» Needs work

The last submitted patch, 851168-leave-unprefixed-alone.patch, failed testing.

Comment #7

Stevel CreditAttribution: Stevel commented 12 July 2010 at 15:03

Status:

Needs work

» Needs review

File	Size
851168-param-default.patch	6.27 KB

Forgot the default parameter in the mysql schema.

Comment #8

Stevel CreditAttribution: Stevel commented 12 July 2010 at 15:17

File	Size
851168-reroll.patch	6.27 KB

Reroll because #851140: DatabaseSchema::getPrefixInfo() is *severely* broken just got committed, so the previous patch didn't apply any more.

Comment #9

chx CreditAttribution: chx commented 13 July 2010 at 02:47

Status:

Needs review

» Reviewed & tested by the community

Yay for choice.

Comment #10

marcingy CreditAttribution: marcingy commented 29 July 2010 at 02:08

#8: 851168-reroll.patch queued for re-testing.

Comment #11

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 31 July 2010 at 13:26

Since this touches /includes/database/schema.inc, do we need Larry's sign-off here?

Comment #12

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 2 August 2010 at 05:41

Status:

Reviewed & tested by the community

» Needs review

When I talked to Damien the other day, he said the answer to that question was "yes".

Comment #13

Damien Tournoud CreditAttribution: Damien Tournoud commented 2 August 2010 at 09:04

Status:

Needs review

» Reviewed & tested by the community

Actually, I'm happy with this patch.

We might consider renaming DatabaseConnection::getPrefixInfo() to DatabaseConnection::getTableInfo(), but that's out of the scope of this particular patch.