Cache not working properly with DB table prefixes

i didn't do testing, but this one smells like a critical.

Remember to db_escape_string()...

Feels sane to me.

I think should be ported down to 5.x once its committed for the current multisite lovers.

The patch looks clean and simple but it would be great if you could explain why the reordering fixes the problem. (Depending on how important it is, we also might want to document that in a code comment.)

Because table names are having %s inside the {} after recent cache.inc changes (Specifically, the "Split cache into multiple tables" stuff), and this makes doing the array form of db_prefix to have table specific prefixes per table useless in this case, because the table is being prefixed before the %s is expanded to the table name, so it's looking for $db_prefix['%s'] instead of $db_prefix['cache_page'].

Alright, I got it now. I've committed the patch to DRUPAL-5 and CVS HEAD. Thanks.

This broke 3 of my sites. All file syste settings on the site where restored to 'default', did not check the others.

I have my filesystem not on the default location 'files' but on 'sites/files' but the site insisted on going fo 'files' the theme was also reset. Content was still present however.....

I did not check what else was wrong/ok but I moved back the single line from this patch and all was OK again....

I have no shared tables....

This patch should never have been committed:

• Running db_prefix_tables after inserting data will remove {}, e.g. breaking serialized data (like variables).
• Using %s to escape table names does not provide any security. It only protects against escaping from a quoted string, but table names appear literally in the SQL already.

Reverting this patch. The original issue needs work.

Anyone has objections against the first patch?

I no longer object to the non-escaping of the table name on the grounds that if modules really want to mess around with caching so much that they use dynamic table names, it's their problem and they are responsible for not tainting $table. Any module that lets something taint $table already HAS a security problem, whether or not $table is escaped, so it doesn't matter one bit if it's escaped or not.

Anyway, 99.999% of cases would be using a constant string for $table.

I second RTBC. Apologies for the fiasco I caused by balking at the original patch and contributing to causing the serialized data corruption issue :(

I committed this to CVS HEAD. I think this needs to be backported to D5, although it might be a API change. :/

Either way, not marking this fixed until the upgrade instructions have been ugpraded.

How would this be an API change? Who would put something that isn't a string in $table?

Committed to 5.

After applying the latest updates to a Drupal-5-dev site I am getting screenfuls of MySQL errors where the table prefix is ignored, and I suspect this patch.

Warning: Table 'dbname.cache' doesn't exist query: SELECT data, created, headers, expire FROM cache WHERE cid = 'variables' in ~public_html/drupal-5/includes/database.mysqli.inc on line 151

Warning: Table 'cache' was not locked with LOCK TABLES query: UPDATE cache SET data = ....etc

There is not really a 'cache' table. It should use 'dru_cache' (as it did before the update). On another site running from the same Drupal-5-dev code base but without a table prefix the problem does not appear.

The sites are on PHP5, MySQL 4.1.

Rolled back in 5. This seems to be working properly in 5 now.

This should be back-ported to HEAD as well. I have some pending patches I need to fix to follow whatever final decision is made in this thread. My patches are located at: http://drupal.org/node/137415

Testing the latest HEAD I faced a problem with cache tables while using prefixes, see http://drupal.org/node/142439
It appears that "{$table}" string get evaluated first before passed to db_prefix_tables, so cache tables doesn't get prefixed.

This patch correct this behaviour.

PS: I couldn't find clear documentation about the use of variables and curly braces inside strings, but there's a lot of comment regarding this behaviour at http://www.php.net/manual/en/language.variables.php

Regarding #25, I knew I had seen that patch before... it was proposed back in March at http://drupal.org/node/126973 but as you can see the issue was ignored.

Here is a reroll from the root dir. This also removes an introduced, unneeded space in one of the queries. Should be RTBC but I leave it for someone else as I've had a couple of malformed patches as of late.

Well a nearly identical patch just got committed from http://drupal.org/node/136837 - closing this issue.

As tostini noted, the problem with simply having "{$table}" is that PHP replaces that with the table name (without the curly braces). This is documented here: http://www.php.net/manual/en/language.types.string.php#language.types.st... So we need to do '{'. $table . '}' to actually get the table name with curly braces to the db handler. That got included in Drupal 6 today already thanks to this patch: http://drupal.org/node/136837

Drupal 5.x-dev now has "{%s}", $table which replaces the $table variable *after* the database prefixing is run, so it is not possible to have prefixes for the cache table if you use the array syntax for prefixes (which would only identify the table name if already there). That said, Drupal 5 still need the patch applied. The issue pointed to by ChrisKennedy has a backported patch for Drupal 5.x (although it has some coding style issues now). I hope Niel can take a look at it and fix this issue for Drupal 5 too.

Any news for Drupal 5 patch for cache.inc?
You cannot even install Drupal 5 with multisites and prefixed arrays :(
Is there anything that can be done to get this fixed?

Sincerely,
Delf.

Any news for Drupal 5 patch for cache.inc?
You cannot even install Drupal 5 with multisites and prefixed arrays :(
Is there anything that can be done to get this fixed?

Sincerely,
Delf.

If you want it fixed in 5, please help review http://drupal.org/node/136837 so we can get it fixed for 5.

Marking this issue as duplicate for now, meba's patch is likely to be committed if it gets some eyes.

Thanks to everyone for the work on this.

--Brandon