Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#37 | 349508-UTF8-to-make-installable-6.patch | 17.08 KB | Josh Waihi |
#36 | 349508-UTF8-to-make-installable-5-2.patch | 17.72 KB | stuzza |
#33 | 349508-UTF8-to-make-installable-5-1.patch | 17.43 KB | Josh Waihi |
#30 | 349508-UTF8-to-make-installable-4.patch | 15.06 KB | Josh Waihi |
#28 | 349508-UTF8-to-make-installable-3.patch | 15.01 KB | Josh Waihi |
Comments
Comment #2
Crell CreditAttribution: Crell commented+1. I'd recommend putting this check in the installer class's installable() method.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedProperly tag this issue.
Comment #4
Josh Waihi CreditAttribution: Josh Waihi commentedis 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
Comment #6
Josh Waihi CreditAttribution: Josh Waihi commentederr, 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
Comment #7
Josh Waihi CreditAttribution: Josh Waihi commentedThis 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.
Comment #9
Josh Waihi CreditAttribution: Josh Waihi commentedThis 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.
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedI like the approach, but this needs Crell review.
Comment #11
Crell CreditAttribution: Crell commentedThis 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.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.
Comment #12
Josh Waihi CreditAttribution: Josh Waihi commentedI 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.
Comment #14
Josh Waihi CreditAttribution: Josh Waihi commentedretesting - does make sense that the fail was related to file naming
Comment #15
Josh Waihi CreditAttribution: Josh Waihi commentedCrell? review?
Comment #16
c960657 CreditAttribution: c960657 commentedIn 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.
Drupal is written with a capital D.
Quotes around %task is not necessary.
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.
Typo: “tasls”.
“false” should be “FALSE”.
Should be “Ensure the PDO driver […]” – see http://drupal.org/node/1354#functions
Missing space after last comma.
The method only runs one test, so “tests” should be “test”.
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.
I assume you mean “Each entry”.
Drupal comments usually refer to “associative” rather than “keyed” arrayes.
Comment #17
Josh Waihi CreditAttribution: Josh Waihi commentedI have a patch ready, but can't test it since #349671: Make the postgresql driver independant of the schema needs to be commited first.
Comment #18
Josh Waihi CreditAttribution: Josh Waihi commentedand now waiting on #330983: Rename user module tables to singular
Comment #19
Josh Waihi CreditAttribution: Josh Waihi commentedOMG HEAD is stable. so here is the patch
Comment #20
cburschkaThis is a big patch and beyond my ability to review functionally. But I can give you some code-style quibbling.
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.
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.
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.
"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. :)
Trailing whitespace, and articles wouldn't hurt in the comment.
Beside the "ok", note that strings should be single-quoted unless \n, variables or apostrophes make it necessary to double-quote.
Should the blank line be there? Even if so, it should probably contain no whitespace.
Each level should be indented exactly two spaces - also, there should only be one space on either side of the =>.
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.)
Comment #21
Josh Waihi CreditAttribution: Josh Waihi commentedComment #22
Josh Waihi CreditAttribution: Josh Waihi commentedrerolled patch to reflect suggestions from @Arancaytar
Comment #23
sunLike if...elseif...else control structures, catch should go on its own line.
I'm not sure whether api.drupal.org groks that formatting. Not relevant to this patch, just wanted to point this out.
That reads b0rked somehow... ;)
Equally.
Whenever single quotes are required in strings, wrap it with double quotes.
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?
Why is this check only performed for Postgres? I'm pretty sure that MySQL can be installed with latin1 as default locale as well...
Comment #24
Josh Waihi CreditAttribution: Josh Waihi commented@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 :)
Comment #25
Josh Waihi CreditAttribution: Josh Waihi commentednew patch
Comment #26
Josh Waihi CreditAttribution: Josh Waihi commentedThis 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.
Comment #28
Josh Waihi CreditAttribution: Josh Waihi commentedComment #30
Josh Waihi CreditAttribution: Josh Waihi commentedComment #32
Josh Waihi CreditAttribution: Josh Waihi commentedmarking for postgreSQL code Sprint
Comment #33
Josh Waihi CreditAttribution: Josh Waihi commentedHere is the result from the code sprint - it works, its great. Not sure if testbot will like it as it changes install
Comment #35
Josh Waihi CreditAttribution: Josh Waihi commentedARGH!!!! hate when that happens, it passes, state changes and boom!
Comment #36
stuzza CreditAttribution: stuzza commentedAttached 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.
Comment #37
Josh Waihi CreditAttribution: Josh Waihi commentedLooking 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.
Comment #39
Josh Waihi CreditAttribution: Josh Waihi commenteddoesn't make sense, retest
Comment #40
Shiny CreditAttribution: Shiny commentedTested 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.
Comment #42
Shiny CreditAttribution: Shiny commentedI 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.
Comment #43
Dries CreditAttribution: Dries commentedI'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.
Comment #44
Josh Waihi CreditAttribution: Josh Waihi commentedI 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?
Comment #45
Josh Waihi CreditAttribution: Josh Waihi commentedthere is no issue here anymore.
Comment #46
j0rd CreditAttribution: j0rd commentedI'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.
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:
Comment #47
Damien Tournoud CreditAttribution: Damien Tournoud commentedPlease don't reopen old issues like this, especially when they are irrelevant to the problem you are having (this issue is for a problem with PostgreSQL).
Most likely, you have dumped/restored the database of this Drupal site using a broken tool (for example: old versions of Backup/Migrate), and you lost the encoding as a consequence. The settings in
my.cnf
do not affect the ability of Drupal to create tables using an UTF-8 collation.