Make session table singular [#375367]

Comment	File	Size	Author
#8	session_tables_singular.patch	8.63 KB	boombatower

	session_tables_singular.patch	8.48 KB	boombatower

Comment #1

nedjo

he/him/his

English

CreditAttribution: nedjo commented 17 February 2009 at 02:45

Status:

Needs review

» Reviewed & tested by the community

I reviewed the patch. It is thorough and covers the various places where changes are needed:

* In db queries, both old-style like db_query() and new-style like db_select().
* In the table sharing example in default.settings.php. Existing sites upgrading from previous versions will retain incorrect sample code in their legacy settings.php files, but rewriting those in the update doesn't seem doable, given file permissions issues.
* In code comments that name the table.
* In the session table schema.
* In an update function.

Log in or register to post comments

Comment #2

nedjo

he/him/his

English

CreditAttribution: nedjo commented 17 February 2009 at 02:54

I guess my only question would be: does it make sense to put each of these table name changes in a separate update function, or should they be done in groups? In system_update_7012() we renamed three tables related to blocks. (Come to think of it, I'm not sure why that update is in system.install and not block.install.)

Should we put all changes to tables in system.install's schema in the same update? If so, should this patch include all remaining tables in system_schema() that need renaming?

Log in or register to post comments

Comment #3

boombatower CreditAttribution: boombatower commented 17 February 2009 at 03:23

If we choose to do that...I think we should add each of the table renames to the hook as we commit them... I would prefer not to make a mega patch...and since we aren't worried about HEAD-HEAD upgrades if we keep adding to the same hook should be ok.

@nedjo: Thanks for the thorough review.

Log in or register to post comments

Comment #4

recidive CreditAttribution: recidive commented 18 February 2009 at 04:00

Status:

Reviewed & tested by the community

» Needs work

As explained in #330983-15: Rename user module tables to singular, upgrade path will not work.

Log in or register to post comments

Comment #5

boombatower CreditAttribution: boombatower commented 18 February 2009 at 06:00

Status:

Needs work

» Postponed

Will add to update_prepare_d7_bootstrap_rename() once users patch gets in.

Log in or register to post comments

Comment #6

boombatower CreditAttribution: boombatower commented 20 February 2009 at 22:08

Status:

Postponed

» Needs review

other fixed.

Log in or register to post comments

Comment #7

20 February 2009 at 22:20

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Log in or register to post comments

Comment #8

boombatower CreditAttribution: boombatower commented 21 February 2009 at 05:46

Status:

Needs work

» Needs review

File	Size
session_tables_singular.patch	8.63 KB

Since user table rename patch got in this needs re-roll.

The session tables have same issue that user tables do and as such need to be hard-coded into update.php.

EDIT: The patch has a spacing issue in default.settings.php

'session'  => 'shared_',

Needs additional spacing.

Log in or register to post comments

Comment #9

Dries CreditAttribution: Dries commented 21 February 2009 at 06:10

I obviously support this patch but it needs a little bit more work per the discussion in this issue. Keep workin' it! :)

Log in or register to post comments

Comment #10

Dries CreditAttribution: Dries commented 21 February 2009 at 06:11

Status:

Needs review

» Needs work

Log in or register to post comments

Comment #11

boombatower CreditAttribution: boombatower commented 21 February 2009 at 06:37

Status:

Needs work

» Reviewed & tested by the community

#2 and #4 are both dealt with since #330983: Rename user module tables to singular made it in and this patch uses that format.

Since was originally reviewed in #1 and only two issues fixed marking as RTBC.

NOTE: Tests passed, but results were ignored since this issue was marked as needs work. http://testing.drupal.org/pifr/file/1/4462

Log in or register to post comments

Comment #12

Josh Waihi CreditAttribution: Josh Waihi commented 21 February 2009 at 07:46

no key-word conflicts with session in postgresql. I'm happy.

Log in or register to post comments

Comment #13

David_Rothstein CreditAttribution: David_Rothstein commented 21 February 2009 at 21:49

Note that there is a problem with the way the {sessions} table is being renamed in update.php, and this is being discussed at #330983: Rename user module tables to singular. However, the problem is the same with the {users} table, so this doesn't cause any additional problems that weren't already there, and it could probably be fixed all at once at the other issue. So this is probably ready to go in..

Log in or register to post comments

Comment #14

26 February 2009 at 18:55

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch failed testing.

Log in or register to post comments

Comment #15

jacob.embree CreditAttribution: jacob.embree commented 13 January 2015 at 04:01

Version:	7.x-dev	» 9.x-dev
Assigned:	boombatower	» Unassigned
Issue summary:	View changes
Parent issue:		» #140860: Consistent table names and database handling of table names