Require UTF8 database encoding
Damien Tournoud - December 20, 2008 - 17:13
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | postgresql database |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Josh Waihi |
| Status: | needs work |
| Issue tags: | PostgreSQL Code Sprint, PostgreSQL Surge |
Description
In D6, we had an install time check that the database is correctly created using an UTF8 encoding [1]. Drupal will not correctly without it (and we even have a test for this) when the database uses a different encoding. We should put this test back at install time.
#2
+1. I'd recommend putting this check in the installer class's installable() method.
#3
Properly tag this issue.
#4
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
#5
The last submitted patch failed testing.
#6
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
#7
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.
#9
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.
#10
I like the approach, but this needs Crell review.
#11
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.<?phpif (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.
#12
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.
#13
The last submitted patch failed testing.
#14
retesting - does make sense that the fail was related to file naming
#15
Crell? review?
#16
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_set_message(st('Failed to run all tasks against the database server. The task \'%task\' wasn\'t found.', array('%task' => $task['function'])), 'error');+ public $error = array();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.“false” should be “FALSE”.
+ * Ensures the PDO driver is supported by version of PHP in use.+ if (FALSE === call_user_func_array(array($this, $task['function']),$task['params'])) {+ * Run SQL tests to ensure database is a useable environment.+ '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 spacing is a little weird.
+ * Each key has another keyed array within it, describing theDrupal comments usually refer to “associative” rather than “keyed” arrayes.
#17
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.
#18
and now waiting on #330983: Rename user module tables to singular
#19
OMG HEAD is stable. so here is the patch
#20
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.
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".
"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.)
#21
#22
rerolled patch to reflect suggestions from @Arancaytar
#23
+ } 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...
#24
@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 :)
#25
new patch
#26
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_installThis patch suggests a function called
db_run_taskswhich 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.
#27
The last submitted patch failed testing.
#28
#29
The last submitted patch failed testing.
#30
#31
The last submitted patch failed testing.
#32
marking for postgreSQL code Sprint