When installing this module on a site using a PostgreSQL database, the module throws an error:

PDOException: SQLSTATE[42601]: Syntax error: 7 ERROR: syntax error at or near "CONVERT" LINE 1: ALTER TABLE s3fs_file CONVERT TO CHARACTER SET utf8 COLLATE ... ^: ALTER TABLE {s3fs_file} CONVERT TO CHARACTER SET utf8 COLLATE utf8_bin; Array ( ) in s3fs_install() (line 195 of /Applications/MAMP/htdocs/find/sitename/sites/all/modules/s3fs/s3fs.install).

CommentFileSizeAuthor
#3 non-mysql-install-2295157-3.patch2.31 KBgeodaniel
#1 non-mysql-install-2295157-1.patch677 bytesgeodaniel

Comments

geodaniel’s picture

geodaniel commented
Status: Active » Needs review
StatusFileSize
new non-mysql-install-2295157-1.patch677 bytes

Patch attached which should do the trick.

coredumperror’s picture

coredumperror commented

That character set setting is essential to the operation of s3fs (it makes the 'uri' field case sensitive), but your patch takes it away from non-MySQL databases. Is that OK? I know nothing about PostgreSQL.

The reason that alter table call is even needed is because of a bug in Drupal's database driver that prevents the 'collation' setting in s3fs's hook_schema() implementation from being applied. Looking at the PostgreSQL driver, it doesn't appear to mention collation at all, and I'm not sure why.

If PostrgeSQL is inherently case-sensitive, then I suppose setting a collation (if that's even possible) isn't useful. Which would make your patch just fine.

geodaniel’s picture

geodaniel commented
StatusFileSize
new non-mysql-install-2295157-3.patch2.31 KB

I've had a bit of a read around, and it seems like Postgres is case sensitive by default, and SQLite is too, so they don't need to run this ALTER statement.

The attached patch also checks the database type when creating the s3fs_file_temp table, which was preventing the library from being used on a Postgres site.

With the patch, I can successfully store files in my S3 bucket, and I can upload files with different capitalisation of the same name (e.g. TEST.jpg and test.jpg both work). I haven't tested this on a SQLite database.

coredumperror’s picture

coredumperror commented
Status: Needs review » Fixed

Looks good to me! Just pushed this up to git.

geodaniel’s picture

geodaniel commented

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

edaa’s picture

edaa commented

Another way to solve the collation issue is:

/**
 * Implements hook_schema().
 */
function s3fs_schema() {
  $schema = array();
  $schema['s3fs_file'] = array(
    'description' => 'Stores metadata about files in S3.',
    'fields' => array(
      'uri' => array(
        'description' => 'The S3 URI of the file.',
        'type' => 'varchar',
        'length' => 255,
        'not null' => TRUE,
        'default' => '',
        'binary' => TRUE,
      ),

Note: 'binary' => TRUE, is added, see 'file_managed' schema definition in system.install.

coredumperror’s picture

coredumperror commented

I dunno... that seems like it might not be the best idea. The 'binary' => TRUE mechanism is implemented by making the column BINARY, which (as I understand it) prevents indexing. I'm not really up-to-snuff on low-level database stuff like that, though, so maybe it's OK? Interestingly enough, the docs do not accurately describe what this flag does.

:30 minutes of research later:

OK, so I think that a VARCHAR (50) BINARY column is actually identical to a VARCHAR (50) CHARACTER SET utf8_bin column. Maybe. The docs don't make this remotely clear (they don't even describe what VARCHAR (50) BINARY actually means), so I'm forced to assume it means the same thing as VARBINARY (50) (based on some StackOverflow answers I found).

So I think you're right that changing the hook_schema() implementation to use 'binary' => TRUE is the right way to do this. But then the question becomes: "Is that a safe thing to change for existing users?" I'll have to do some testing. Fortunately, the big project I've been working on is currently on a short hiatus, so I actually have time!

coredumperror’s picture

coredumperror commented

Hah, well there we go! Turns out that executing ALTER TABLE s3fs_file CHANGE uri uri VARCHAR(255) BINARY actually just sets the column to COLLATE utf8_bin. So that means 'binary' => TRUE has been the right answer all along, and I just never discovered it in the API docs. Silly me. I've pushed a new dev version that converts to use 'binary' => TRUE.

edaa’s picture

edaa commented

Thanks, a discussion: DB Case Sensitivity: Allow BINARY attribute in MySQL