Require UTF8 database encoding

+1. I'd recommend putting this check in the installer class's installable() method.

is this what we're looking for? I'm not sure if db_query and db_result are available at this stage but its a start

err, so turns out that installable() is called before the database credentials are given so this is a bad spot to put it. But as dicussed in #342950: Compatibility stored procedures should be defined in includes/database/pgsql, a driver should have a install() and update() type method where this encoding check could go

This changes the databaseinstaller class around a little bit and makes it a little more truer to its name. I renamed the test method to runTasks with the idea that tasks are run against the database rather than tests, tests become apart of tasks, but tasks also include functions which is where the databaseinstaller_pgsql extends itself and add a function task which ensures UTF8 is being used.

This also relates to #342950: Compatibility stored procedures should be defined in includes/database/pgsql in the fact that runTasks() can be used both as install and updates - updates could be run as function tasks which check that certain measures are inplace and if not, make them so.

This patch makes things a little more clearer, after talking with DamZ on IRC, I've merged tasks into being functions that can have parameters passed to them and removed db_result() as it is deprecated.

This is totally bizarre. But no more totally bizarre than what was already there, I suppose. :-) It's like our very own micro-simpletest.

I'd prefer to use call_user_func_array(array($this, $task['function']), $task['params']), rather than the direct call if we can. That lets us keep separate method parameters which in turn makes the code more self-documenting.

if (isset($params['fatal']) && $params['fatal']) {

That can become if (!empty($params['fatal']).

In the postgres constructor, I don't think you need to bother with the array_merge. It's an associative array, so just assign a new key and be done with it.

I understand how tests are a special case of tasks, but I feel a little uneasy putting so much into one big array definition like that. I see why we're doing it, and can't think of a better alternative off the top of my head, but I'm still uneasy about it. (To be fair, I was uneasy about the code that's in there now, too, for much the same reason.) The best alternative I can think of is to model on simpletest with detecting task method names, most of which in this case would just call back to a common runTestQuery.

So yeah, I guess I'm OK with this as an extension of the current approach, unless we want to refactor the approach itself. CNW for the smaller stuff above.

I like the idea of a mini unit type test facility for the database drivers - I've changed a few more things around to make better sence.

retesting - does make sense that the fail was related to file naming

Crell? review?

In runTasks(), you assign values to $return but never use them.

Doxygen comments should wrap at 80 characters. One wraps at more, and several at considerably less than 80.

+ * Defines basic drupal requirements for databases.

Drupal is written with a capital D.

+        drupal_set_message(st('Failed to run all tasks against the database server. The task \'%task\' wasn\'t found.', array('%task' => $task['function'])), 'error');

Quotes around %task is not necessary.

+  public $error = array();

Arrays are usually named in plural AFAICT.

I think it is surprising (the kind of surprises that programmers don't like :-) that the return value of ->installable() depends on whether ->runTasks() has been run in advance.

+        // Returning false is fatal. No other tasls can run.

Typo: “tasls”.
“false” should be “FALSE”.

+   * Ensures the PDO driver is supported by version of PHP in use.

Should be “Ensure the PDO driver […]” – see http://drupal.org/node/1354#functions

+        if (FALSE === call_user_func_array(array($this, $task['function']),$task['params'])) {

Missing space after last comma.

+   * Run SQL tests to ensure database is a useable environment.

The method only runs one test, so “tests” should be “test”.

+      'function' => 'runTestQuery',
+      'params'   => array(
+        'query' => 'CREATE TABLE drupal_install_test (id int NULL)',
+        'command'  => 'CREATE',
+        'fail'  => 'Failed to <strong>CREATE</strong> a test table on your %name database server with the command %query. %name reports the following message: %error.<p>Are you sure the configured username has the necessary %name permissions to create tables in the database?</p>',
+        'fatal'    => TRUE,
+      ),

The use of an associative array for params gives the impression that the keys are used, but they are not – the order is important.
The spacing is a little weird.

+   * Each key has another keyed array within it, describing the 

I assume you mean “Each entry”.
Drupal comments usually refer to “associative” rather than “keyed” arrayes.

I have a patch ready, but can't test it since #349671: Make the postgresql driver independant of the schema needs to be commited first.

and now waiting on #330983: Rename user module tables to singular

OMG HEAD is stable. so here is the patch

This is a big patch and beyond my ability to review functionally. But I can give you some code-style quibbling.

   return $databases;
 }
-
+/**
+ * Database installer structure.
+ *
+ * Defines basic Drupal requirements for databases.
+ */

There should be a blank line between the end of the previous function and the doxygen comment of the class - this blank line is being deleted here.

+  /**
+   * Assert test as failed.
+   */
+  protected function fail($message) {
+    $this->results[$message] = FALSE;
+  }
+  

Last blank line contains trailing white-space.

Check Drupal is installable on database.

You can trivially turn this comment into a full sentence by adding "whether" and "the", which is always preferable. Same about "Drupal can operate on database environment".

make sure Drupal can install ok

"Drupal can install" covers it, I think.

+      $return = null;

You assign a value to $return several times, but don't seem to be reading it anywhere. If I'm wrong, you still need to capitalize NULL.

CONNECT to the database ok

"worked" would probably be a good idea. As you can see, I'm not overly fond of using colloquial language in user/admin-facing text. :)

+  
+  /**
+   * Run SQL test to ensure database can execute command with current user.
+   */

Trailing whitespace, and articles wouldn't hurt in the comment.

"Drupal can use %command commands on the database ok."

Beside the "ok", note that strings should be single-quoted unless \n, variables or apostrophes make it necessary to double-quote.

+class DatabaseInstallerException extends Exception {
+  
+}

Should the blank line be there? Even if so, it should probably contain no whitespace.

+    $this->tasks[] = array(
+        'function' => 'checkEncoding',
+        'arguments'   => array(),
+      );

Each level should be indented exactly two spaces - also, there should only be one space on either side of the =>.

+  /**
+   * Check encoding is UTF8.
+   */
+  protected function checkEncoding() {
+    try {
+      if (db_query('SHOW server_encoding')->fetchField() == 'UTF8') { 
+        $this->pass("Database is encoded in UTF-8");
+      }
+      else {
+        $this->fail(st("Drupal uses %encoding encoding with %driver however, %driver is not set to use UTF-8 encoding. Please create the database with %encoding encoding.", array('%driver' => $this->name())));
+      }
+    } catch (Exception $e) {
+      $this->fail(st("Drupal could not determine the encoding of the database was set to UTF-8"));
+    }  
+  }
 }

Multiple strings double-quoted - also, I pass() expects an st()-localized string like fail() does.

----

A functional question: You define multiple tests together with messages in a structured array. The messages are then loalized in runTestQuery as st($fail). This probably saves performance, but I'm not sure if the locale string extractor will like it. We normally pass direct string literals to t() to make sure they can be extracted - though the menu system does it differently since D6, so I don't know if this rule still applies.

(Argh, this really was a monster patch to comb through.)

rerolled patch to reflect suggestions from @Arancaytar

+    } catch (DatabaseInstallerException $e) {

Like if...elseif...else control structures, catch should go on its own line.

+   * @var Array structure that describes each task to run.
...
+ * @class Exception class used to throw error if the DatabaseInstaller fails.

I'm not sure whether api.drupal.org groks that formatting. Not relevant to this patch, just wanted to point this out.

+   * Each value of the tasks array is an associative array describing the function
+   * to call for task at hand, the any arguments to be passed to the function.

That reads b0rked somehow... ;)

+   * Run database specfic tasks to make sure Drupal can install ok.
+   *
+   * Called at install to run tests against the database to make sure Drupal can 
+   * install ok. Each test should append a message to the success or error array.
...
+      $this->pass(st('Drupal can use %command commands on the database ok.', array('%command' => $command)));

Equally.

+        drupal_set_message(st('Failed to run all tasks against the database server. The task %task wasn\'t found.', array('%task' => $task['function'])), 'error');

Whenever single quotes are required in strings, wrap it with double quotes.

+        $message .= '<p class="error">' . $result  . '</p>';

Not sure whether our theme classes are prepared to work on paragraphs... Has this been tested on all browsers? Are there other ways (markup) we could use?

 class DatabaseInstaller_pgsql extends DatabaseInstaller {
...
+   * Check encoding is UTF8.

Why is this check only performed for Postgres? I'm pretty sure that MySQL can be installed with latin1 as default locale as well...

@sun, thanks, I'll path those things up soon. If you want to test these things in MySQL, by all means, please add to the patch :)

new patch

This patch is going to hopefully solve more than this issue, also #342950: Compatibility stored procedures should be defined in includes/database/pgsql and moving things out of system_install
This patch suggests a function called db_run_tasks which is responsible for checking the state of the Database and making any alterations where needed to keep the Database's state uptodate. In other words, installing and updating will call this function, any database updates will need to be written as a task that checks if the update is implemented then runs the update if required.

Hope that makes sense, its 2:30am.

marking for postgreSQL code Sprint

Here is the result from the code sprint - it works, its great. Not sure if testbot will like it as it changes install

ARGH!!!! hate when that happens, it passes, state changes and boom!

Attached an updated patch hopefully addressing the previous failedness.

One thing that's changed is I've wrapped both calls to _install_settings_form_validate() in install.php with try statements to catch any DatabaseTaskExceptions. This is necessary because _install_settings_form_validate() is called twice during the install process, but only once that is relevant to database tasks. Inside _install_settings_form_validate() there is no way to determine the context in which the function is being called, so catching the exception one level up seemed more appropriate. Where _install_settings_form_validate() is called in install_verify_settings() (when database tasks aren't relevant), no action is taken.

Hope that makes (inelegant) sense.

Looking at this further I noticed that db_run_tasks was trying to get a connection before the tasks could be run which threw an uncaught error with bad db credentials so I fixed this and all the issues went away. Plus there were a few lines of code that got removed in the last patch which were needed to make an active connection.

doesn't make sense, retest

Tested with a SQL_ASCII encoded database.

The PostgreSQL database must use UTF8 encoding to work with Drupal.Please recreate the database with UTF8 encoding. See INSTALL.pgsql.txt for more details.

Changed encoding to UTF8, and installed proceeded.

So passed on Postgres 8.3.7

Tested again in SQLite - all installed happily.

I reran simpletest on mysql
Add feed functionality
Categorize feed item functionality
Remove feed item functionality
Remove feed functionality

All pass.

Not sure what the bot is smoking.

I've committed this to CVS HEAD but I'm marking this as 'code needs work' as we need to update the MySQL and SQLite backends accordingly.

I made this work in MySQL and SQLite, but unlike PostgreSQL, I don't believe they need to use the installer layer just yet. Am I missing something here?

there is no issue here anymore.

I've just run into this issue on a site I've created with Drupal 7.

It's a standard install of MySQL 5.1 on Ubuntu 10.04 LTS, so I assume others have this problem, although I've been using this site for 4 months before I ran into the issue. I assume this problem is widespread amoung those who use MySQL 5.1 under Ubuntu 10.04 LTS, but people are not running into it as they don't use non-latin1 characters when testing their sites.

I believe upon install, Drupal should fail, if it's not able to set the charset of the tables as UTF8, which is required for the site to function appropriately.

Also posted this problem here: http://drupal.org/node/756364#comment-5124986

I've got a Drupal 7 site, in which all the tables are CHARSET latin1;

I'm running into problems when saving nodes with non-latin1 characters.

PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect string value: '\xE2\x80\x8B&nb...' for column 'body_value' at row 1
...
in field_sql_storage_field_storage_write() (line 424 of /home/www/powerengineercentral.ca/htdocs/modules/field/modules/field_sql_storage/field_sql_storage.module

Ideally, Drupal would fail upon installation if for what ever reason, Drupal is unable to set tables are UTF8. This would allow the user to resolve the issue, before they create a site and later may have problems converting the tables to UTF8.

I do believe this is a problem, which should be looked at.

This is a site which I've setup on a standard Ubuntu 10.04 LTS with MySQL 5.1. Nothing wonky in my configs.

Here are the MySQL variables which may be causing this issue:

root@li361-6:# mysqladmin  variables | grep -i char
| character_set_client            | latin1 
| character_set_connection        | latin1
| character_set_database          | latin1
| character_set_filesystem        | binary
| character_set_results           | latin1
| character_set_server            | latin1
| character_set_system            | utf8 
| character_sets_dir              | /usr/share/mysql/charsets/