Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#47 | 330983-back-to-normal-for-now-brown-cow.patch | 79.49 KB | Josh Waihi |
#44 | 330983-back-to-normal-for-now.patch | 79.43 KB | David_Rothstein |
#41 | 330983-back-to-normal.patch | 78.93 KB | Josh Waihi |
#27 | fix_update_php.patch | 985 bytes | David_Rothstein |
#21 | user_tables_singular.patch | 76.07 KB | boombatower |
Comments
Comment #1
cburschkaWow, there are over 20 such tables with plural names... I'll test this in a bit.
Comment #2
recidive CreditAttribution: recidive commentedHi, 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
Comment #3
Dries CreditAttribution: Dries commentedI think this is a fine patch and I don't mind it being one patch -- it is _really_ easy to review. :-)
Comment #4
Dries CreditAttribution: Dries commentedA visual review of this patch looked good. After applying this patch, I get a WSOD so I can't actually run update.php.
Comment #5
recidive CreditAttribution: recidive commentedYes, 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.
Comment #7
lilou CreditAttribution: lilou commentedSee: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
Comment #9
boombatower CreditAttribution: boombatower commentedWorking on.
Comment #10
boombatower CreditAttribution: boombatower commentedSearch 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.
Comment #12
boombatower CreditAttribution: boombatower commentedfixed an issue.
Comment #13
boombatower CreditAttribution: boombatower commentedComment #14
Dave ReidPatch 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.
Comment #15
recidive CreditAttribution: recidive commented@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.
Comment #16
boombatower CreditAttribution: boombatower commentedI 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.
Comment #17
boombatower CreditAttribution: boombatower commentedDiscussing solution in #140860: Consistent table names and database handling of table names
Comment #18
boombatower CreditAttribution: boombatower commentedFixed 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.
Comment #19
Dave ReidIn the update.php segment, there's
+ini_set('display_errors', 1);
Is that intentional?
Comment #20
boombatower CreditAttribution: boombatower commentedarg...I knew I would leave that in.
During debug of WSOD I had that.
Comment #21
boombatower CreditAttribution: boombatower commentedRemoved ini_set() left over from debug, per #19.
Same reason as #18.
Good catch David.
Comment #22
webchickI fixed the spacing on one hunk...
...and committed this. Thanks!
Please update the documentation.
Comment #23
boombatower CreditAttribution: boombatower commentedThanks 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.
Comment #24
David_Rothstein CreditAttribution: David_Rothstein commentedHm, 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):
Comment #25
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, 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:
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.
Comment #26
David_Rothstein CreditAttribution: David_Rothstein commentedOK, file attachments seem to be back on, so here is the temporary fix attached as a patch.
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedLet's try again.
Comment #28
David_Rothstein CreditAttribution: David_Rothstein commentedResponding to my own comment:
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.)
Comment #29
boombatower CreditAttribution: boombatower commentedI 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.
Comment #30
webchickDang. Thanks for the patch!
Fixed.
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedSure, 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.
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commentedEither way, this should be "active", due to the problem at #140860: Consistent table names and database handling of table names if nothing else...
Comment #33
boombatower CreditAttribution: boombatower commentedComment #34
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #35
dwwThis 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".
Comment #36
Josh Waihi CreditAttribution: Josh Waihi commentedI second @dww's motion.
Comment #37
webchickCool. I will do this tonight. Hopefully we can work out another solution.
Comment #38
Dries CreditAttribution: Dries commentedMaybe the database layer can escape the table name (if not already)?
Comment #39
webchick@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.
Comment #40
boombatower CreditAttribution: boombatower commented@Dries: my sentiments exactly. http://drupal.org/node/140860#comment-1283474
Comment #41
Josh Waihi CreditAttribution: Josh Waihi commentedpatch to rollback
Comment #42
boombatower CreditAttribution: boombatower commentedBefore 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.
Comment #43
David_Rothstein CreditAttribution: David_Rothstein commentedThis first part of the rollback patch does not look right:
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.
Comment #44
David_Rothstein CreditAttribution: David_Rothstein commentedHere 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.
Comment #46
dww@boombatower: #140860-74: Consistent table names and database handling of table names
Comment #47
Josh Waihi CreditAttribution: Josh Waihi commentedComment #48
Josh Waihi CreditAttribution: Josh Waihi commentedset for testing
Comment #49
webchickOk, 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.
Comment #50
webchickComment #51
catchComment #52
boombatower CreditAttribution: boombatower commentedIndeed.
Comment #53
webchickComment #54
webchickThis never ended up making it into D7, so removing it from the "Needs Documentation" queue so as not to screw with people. ;P
Comment #55
mbrett5062 CreditAttribution: mbrett5062 commentedThis 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.
Comment #56
mgiffordComment #57
webchickBecause of reserved table names, ATM we can't do this, unless we also fix #140860: Consistent table names and database handling of table names, so marking this "won't fix" for now.