As explained in #140860, for consistency we need all tables to be in singular.

Here is a patch that renames user.module tables so {users} become {user} and {users_roles} become {user_role}.

This needs testing.

Files: 
CommentFileSizeAuthor
#47 330983-back-to-normal-for-now-brown-cow.patch79.49 KBJosh Waihi
Passed: 10268 passes, 0 fails, 0 exceptions
[ View ]
#44 330983-back-to-normal-for-now.patch79.43 KBDavid_Rothstein
Failed: 10231 passes, 37 fails, 13 exceptions
[ View ]
#41 330983-back-to-normal.patch78.93 KBJosh Waihi
Failed: 10250 passes, 37 fails, 13 exceptions
[ View ]
#27 fix_update_php.patch985 bytesDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#21 user_tables_singular.patch76.07 KBboombatower
Failed: Failed to apply patch.
[ View ]
#18 user_tables_singular.patch76.26 KBboombatower
Failed: Failed to apply patch.
[ View ]
#12 user_tables_singular.patch69.86 KBboombatower
Failed: Failed to apply patch.
[ View ]
#10 user_tables_singular.patch69.85 KBboombatower
Failed: 9705 passes, 31 fails, 4 exceptions
[ View ]
drupal_singular_user_tables.patch59.44 KBrecidive
Failed: Failed to apply patch.
[ View ]

Comments

cburschka’s picture

Wow, there are over 20 such tables with plural names... I'll test this in a bit.

recidive’s picture

Hi, I think it's better to work on a patch per module. See these two comments on the other issue:

http://drupal.org/node/140860#comment-1095452

http://drupal.org/node/140860#comment-1095516

Dries’s picture

I think this is a fine patch and I don't mind it being one patch -- it is _really_ easy to review. :-)

Dries’s picture

A visual review of this patch looked good. After applying this patch, I get a WSOD so I can't actually run update.php.

PHP Fatal error:  Uncaught exception 'PDOException' with message 'SELECT u.*, s.* FROM {user} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.sid = :sid -
SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal_cvs.user' doesn't exist' in /Users/dries/Sites/drupal-cvs/includes/database/database.inc:425
Stack trace:
recidive’s picture

Yes, I think this will be challenging, can't start session, therefore can't do access check.

There are several ways to workaround this, here are some I've thought of:

1- Implement a temporary set of session handling functions that uses the old table names.
2- Create prefixed temporary users and sessions tables copying only rows for uid = 1.
3- Rename tables before access check.

#1 is more sound, since it will not require changes to database before the access check.

Another thing that may impact the way we work around this, is if it's strictly necessary to support updating previous versions of HEAD or if we only need to care about Drupal 6 to 7 updates.

Thoughts?

Keeping as CNR, since we need to test this on a clean install too. And with the hope to get some ideas on how to make that update happen.

Status:Needs review» Needs work

The last submitted patch failed testing.

lilou’s picture

Status:Needs review» Needs work

The last submitted patch failed testing.

boombatower’s picture

Assigned:recidive» boombatower

Working on.

boombatower’s picture

Status:Needs work» Needs review
StatusFileSize
new69.85 KB
Failed: 9705 passes, 31 fails, 4 exceptions
[ View ]

Search for "users" resulted in 666 results. [ interestingly :) ]

After changing references to users and users_roles. The same search results in 527. So 139 references changed.

Also added update hook. Doesn't seem to make sense to try and allow update from HEAD -> HEAD and I think several other patches have already required a re-install.

Status:Needs review» Needs work

The last submitted patch failed testing.

boombatower’s picture

StatusFileSize
new69.86 KB
Failed: Failed to apply patch.
[ View ]

fixed an issue.

boombatower’s picture

Status:Needs work» Needs review
Dave Reid’s picture

Patch looks good. I couldn't find any more instances of users or user_roles. Testing locally was ok. I could not however, test a 6.x -> 7.x upgrade. All I got was a WSOD. :/ Once that can be tested successfully, this looks good to go.

recidive’s picture

Status:Needs review» Needs work

@boombatower: thanks for working on this.

As I've explained in #5, users and sessions tables need a special upgrade path, since upgrade.php performs a full bootstrap before initializing upgrades. And as you've renamed users table in session initializing queries, it will try to query new table names while they are still with old names in the database.

This is challenging, maybe it's better to upgrade users, sessions, blocked_ips and maybe language on one single patch, as their upgrade paths are not that trivial.

Anyway, I think it will need help from some bootstrap guru.

boombatower’s picture

I understand the issue as you described it and agree we need some other way to upgrade or something.

I think we can rename the tables in separate patches so as not to force an enormous one, but we can continue to add code to the same update hook or w/e we need to do.

boombatower’s picture

boombatower’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new76.26 KB
Failed: Failed to apply patch.
[ View ]

Fixed upgrade issue. Also change all upgrade hooks to use {user} since the tables will be renamed before they run.

RTBC per #14 as long as tests pass.

Dave Reid’s picture

Status:Reviewed & tested by the community» Needs review

In the update.php segment, there's
+ini_set('display_errors', 1);

Is that intentional?

boombatower’s picture

arg...I knew I would leave that in.

During debug of WSOD I had that.

boombatower’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new76.07 KB
Failed: Failed to apply patch.
[ View ]

Removed ini_set() left over from debug, per #19.

Same reason as #18.

Good catch David.

webchick’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs Documentation

I fixed the spacing on one hunk...

  *
  *   $db_prefix = array(
  *     'default'   => 'main_',
- *     'users'     => 'shared_',
+ *     'user'     => 'shared_',
  *     'sessions'  => 'shared_',
  *     'role'      => 'shared_',
  *     'authmap'   => 'shared_',

...and committed this. Thanks!

Please update the documentation.

boombatower’s picture

Status:Needs work» Fixed

Thanks for working on it!

I am assuming I will just make the section generic when I start renaming all the other tables, but here is the base. http://drupal.org/node/224333#rename-user-tables.

David_Rothstein’s picture

Title:Rename user module tables to singular» HEAD broken: Rename user module tables to singular
Category:task» bug
Priority:Normal» Critical
Status:Fixed» Needs review

Hm, this patch seems to have broken update.php. I was trying to test the D6->D7 upgrade for another patch and got a WSOD on update.php due to the code that was committed here.

The "patch" below fixes things (looks like we're temporarily unable to upload files due to the d.o. upgrade but the patch is small enough that it's easy just to paste here):

Index: update.php
===================================================================
RCS file: /cvs/drupal/drupal/update.php,v
retrieving revision 1.273
diff -u -p -r1.273 update.php
--- update.php 18 Feb 2009 15:19:54 -0000 1.273
+++ update.php 19 Feb 2009 06:03:56 -0000
@@ -588,12 +588,11 @@ function update_prepare_d7_bootstrap() {
   // Allow the database system to work even though the registry has not
   // been created yet.
   drupal_bootstrap(DRUPAL_BOOTSTRAP_DATABASE);
-  update_prepare_d7_bootstrap_rename();
-
   include_once DRUPAL_ROOT . '/includes/install.inc';
   drupal_install_init_database();
   spl_autoload_unregister('drupal_autoload_class');
   spl_autoload_unregister('drupal_autoload_interface');
+  update_prepare_d7_bootstrap_rename();
   // The new {blocked_ips} table is used in Drupal 7 to store a list of
   // banned IP addresses. If this table doesn't exist then we are still
   // running on a Drupal 6 database, so suppress the unavoidable errors
David_Rothstein’s picture

Also, although the above patch should obviously go in so as to fix HEAD, I think we should find a better approach than the one that was taken here. The code comments for update_prepare_d7_bootstrap() say this:

No access check has been performed when this function is called, so no changes to the database should be made here.

yet clearly that was violated by the code added in this issue. So we should either remove the comment or fix the approach...

I think the approach needs fixing because it seems like a bad idea to change the database schema before the site's actual D6->D7 upgrade is underway. Let's say some bot comes along and happens to hit /update.php while you're figuring out your update plans -- what happens then? More important, what about the case of an inexperienced user? They might visit update.php, get confused by the instructions there, and then want to backtrack. But if the mere act of visiting update.php causes their database to be put in an unusable intermediary state, they won't be able to easily back out and go back to their existing D6 site.

As for how to do this: Looking at the code in includes/session.inc, I was thinking maybe session reads could just be handled by using hook_query_alter() to redirect them to the old database table when appropriate? But then there's still the question of session writes... I wonder how much they can just be avoided during update.php? It seems like the {variable} table could be used as a substitute for some things (e.g., messages), but maybe that is too much of a hack.

David_Rothstein’s picture

OK, file attachments seem to be back on, so here is the temporary fix attached as a patch.

David_Rothstein’s picture

StatusFileSize
new985 bytes
Failed: Failed to apply patch.
[ View ]

Let's try again.

David_Rothstein’s picture

Responding to my own comment:

I was thinking maybe session reads could just be handled by using hook_query_alter() to redirect them to the old database table when appropriate?

Actually, this would be a pain since update.php isn't a module so it can't implement a hook without help. @recidive's original idea of overriding the session handling functions in update.php would probably work a lot better. (Seems like it would be pretty simple after some minor code refactoring, actually.)

boombatower’s picture

I think #27 is the simplest way that works to fix this.

People should already be aware that they should backup their database before update...it is all over the place. And a table rename won't hurt any of their data anyway. If necessary maybe just display a hard coded page dumped through the page.tpl.php file with a message saying something to that end.

webchick’s picture

Title:HEAD broken: Rename user module tables to singular» Rename user module tables to singular
Status:Needs review» Fixed

Dang. Thanks for the patch!

Fixed.

David_Rothstein’s picture

People should already be aware that they should backup their database before update...it is all over the place. And a table rename won't hurt any of their data anyway.

Sure, but for an inexperienced user, restoring a database backup or renaming a table by hand is not a trivial thing to do. And while they're figuring out how to do it, their site is inaccessible.

I think a good principle is that until they press the big red (er, "big gray") update button, their database should not be mangled in a way that affects the ability of their Drupal 6 site to work correctly. This seems to be the way things worked for the D5->D6 upgrade -- see the long description at http://api.drupal.org/api/function/update_fix_d6_requirements/6

Anyway, I was about to try writing a patch for this, but then noticed the discussion at #140860: Consistent table names and database handling of table names about why the {users}->{user} rename may have other problems, so I'll hold off for the time being to see how that shakes out.

David_Rothstein’s picture

Status:Fixed» Active

Either way, this should be "active", due to the problem at #140860: Consistent table names and database handling of table names if nothing else...

boombatower’s picture

Status:Active» Postponed
Damien Tournoud’s picture

Title:Rename user module tables to singular» Head broken: Rename user module tables to singular
Status:Postponed» Active
dww’s picture

This patch should be rolled back based on #371: resolve ANSI SQL-92/99/2003 reserved words conflict in query statements. We're not supposed to be using SQL reserved words for tables, period. That standard is more important than plural vs. singular. If by some miracle we come up with a solution that makes everyone happy over at #140860: Consistent table names and database handling of table names we can open a new issue about whatever is to become of the {users} table (for example, one proposal is to call it {drupal_user}). But, regardless, it shouldn't be {user}, so we should revert this patch and then mark this issue "won't fix".

Josh Waihi’s picture

Status:Active» Reviewed & tested by the community

I second @dww's motion.

webchick’s picture

Cool. I will do this tonight. Hopefully we can work out another solution.

Dries’s picture

Maybe the database layer can escape the table name (if not already)?

webchick’s picture

Status:Reviewed & tested by the community» Needs work

@Dries: That would be nice, but it's not trivial. There is a very long discussion about this at #371: resolve ANSI SQL-92/99/2003 reserved words conflict in query statements. Module developers have to tokenize the table/column names in queries so that the database layer can parse them out and escape them. That feels like a worse solution to me in terms of added DX burden than inconsistent table names.

Also, the patch no longer reverse-applies.

boombatower’s picture

@Dries: my sentiments exactly. http://drupal.org/node/140860#comment-1283474

Josh Waihi’s picture

StatusFileSize
new78.93 KB
Failed: 10250 passes, 37 fails, 13 exceptions
[ View ]

patch to rollback

boombatower’s picture

Before this goes in can I get an actually response to http://drupal.org/node/140860#comment-1283474. I have brought up the point over and over and everyone seems to just get caught up on the whole its a reserved word, crud changes and consistency are pointless.............

This is a real issue...renaming the users -> user just brought the BUG to the surface. Can we please fix the bug in one of the two possible manors and then go about this issue once we have decided upon that? In same way that not escaping strings can cause SQL injection this IS a BUG.

David_Rothstein’s picture

This first part of the rollback patch does not look right:

-  if (db_table_exists('users')) {
-    db_rename_table($ret, 'users', 'user');
-    db_rename_table($ret, 'users_roles', 'user_role');
+  if (db_table_exists('user')) {
+    db_rename_table($ret, 'user', 'users');
+    db_rename_table($ret, 'user_role', 'users_roles');

Presumably that whole update.php function would need to be removed, not edited?

For what it's worth, I think @boombatower is absolutely correct... hopefully this rollback is just a temporary measure until the other issue is worked out.

David_Rothstein’s picture

Status:Needs work» Needs review
StatusFileSize
new79.43 KB
Failed: 10231 passes, 37 fails, 13 exceptions
[ View ]

Here is the rollback patch with the above change... I have not tested this at all or even looked at it - I just made the change to update.php described above.

Status:Needs review» Needs work

The last submitted patch failed testing.

dww’s picture

Josh Waihi’s picture

StatusFileSize
new79.49 KB
Passed: 10268 passes, 0 fails, 0 exceptions
[ View ]
Josh Waihi’s picture

Status:Needs work» Needs review

set for testing

webchick’s picture

Status:Needs review» Postponed

Ok, given that this is breaking PostgreSQL/SQLite *today* and a more generalized solution is still under debate at #140860: Consistent table names and database handling of table names, I've rolled back this patch for now.

Marking postponed, pending a more general solution to solving the reserved SQL keyword stuff.

webchick’s picture

Title:Head broken: Rename user module tables to singular» Rename user module tables to singular
catch’s picture

Version:7.x-dev» 8.x-dev
boombatower’s picture

Indeed.

webchick’s picture

Category:bug» feature
Priority:Critical» Minor
webchick’s picture

Issue tags:-Needs Documentation

This never ended up making it into D7, so removing it from the "Needs Documentation" queue so as not to screw with people. ;P

mbrett5062’s picture

Issue summary:View changes

This issue is based off another where much discussion had been made, and was finally bumped to 9.x-dev, therefore I think this should also be bumped.