MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols)

This looks like it ties in directly to the discussion on #1309278: Make PDO connection options configurable

Reference manual: http://dev.mysql.com/doc/refman/5.5/en/charset-unicode-utf8mb4.html

So...

What we want: Allow to use utf8mb4 instead of utf8.

What we don't want: Allow to use non-utf8 character sets.

Tagging for backport.

This patch lets you set the 'names' connection option to utf8mb4, other charsets are defaulted back to utf8. This doesn't feel like the right way to approach this issue though.

In the case that we try to use utf8mb4 on an older version of MySQL we enter a scenario where Drupal cannot connect to the database, and we're brought back to the drupal install.php (which writes an updated settings.php w/o the utf8mb4 option). Not horrible, but not quite adequate either.

How should this kind of issue be handled? Should we...

• default to utf8mb4 on versions that support it
• only allow utf8mb4 on mysql versions that support it (5.5.3+), and fall back to utf8 for all other versions
• leave this patch as-is
• other solutions?

I think detecting wether it is compatible or not makes sense and throwing an error if it's not.

There are two ways to detect compatibility:

- Check the MySQL version (should be above 5.5.3)
- Run the query "show character set;"

The problem is that I think both of these things require a extra query :-/

Two comments about the above patch

1. "names" is a bit of a confusing variable name for the settings, how about "charset"?

2. I understand we want to limit mistakes, but is there a good reason to force only utf8 and utf8mb4 if the user is savy enough to edit their database connection settings? I think defaulting to utf8 might be good enough to limit mistakes and we should allow savy users to set the connection encoding to whatever they want if they so desire.

Finally, thank you *so* much basic for taking this on. Mucho mucho appreciation!

Moving to CNW since there's a patch here.

From sun:

What we want: Allow to use utf8mb4 instead of utf8.

What we don't want: Allow to use non-utf8 character sets.

1. I like the idea of changing 'names' to 'charset' (I agree that this is confusing)
2. I took @sun's post (#3) into consideration when creating the patch - this is why there are only two available options. I'll leave it up to the dbtng folks to decide what we want to allow. The biggest hurdle I believe is keeping people off of latin1 and on utf8*, by opening this back up we will also have to continue supporting latin1 and other unsupported charsets.

I am feeling that we should default to utf8mb4 where possible, and allow changing the charset back to utf8 if needed. This would cause mysql to behave the same as sqlite and postgres by default (assuming mysql >= 5.5.3), and use the legacy charset (utf8) if the mysql version doesn't support utf8mb4.

I would like to get some more feedback on this issue as well.

Another angle that this patch should address is the default table mysql character sets for tables. If we don't change the default character set for tables, setting the NAMES at connection-time will do nothing useful.

The table-level character-set is currently defined in includes/database/mysql/schema.inc line 85 and is currently hardcoded. This starts to get tricky as it seems dirty to pull this data from the mysql-*connection* configuration object. Instead, we need something like a variable that can be overridden in settings.php.

Thoughts?

I've ran into another issue with using utf8mb4 that we will need to address: index-size.

Using utf8mb4 increases the index size from 3 bytes per character to 4 bytes per character. This isn't really a problem for text-fields, but absolutely plays havoc with varchar fields who's allowed size is 191 characters or larger. The reason for the is that the varchar-index has a maximum size of 765 bytes. Normally this is fine as it is exactly three-times the size of a 3-byte-per-character 255 character varchar (767 / 3 = 255). When we switch to 4-bytes-per-character, the maximum number of characters a 765 byte index can support is 191 (765 / 4 = 191).

So we have two options here
1. Only support varchars up to 191 characters in length for utf8mb4. I *don't* think this is a very good idea as it breaks too many assumptions.
2. Only support utf8mb4 characters in text columns. I think this is entirely reasonable.

Implementing 2 will require a little more logic in the createField method, but I don't see this being problematic.

Here's a snippet you can run to update all your tables and text-field to utf8mb4 for testing:

EDIT: DONT USE THIS! It will also change the charset of varchar fields created after it was run (BAD!). Use the one pasted below...

  $schema = drupal_get_schema();  
  foreach ($schema as $table_name => $table) {
    db_query('ALTER TABLE '.$table_name.' DEFAULT CHARACTER SET utf8mb4');
    foreach ($table['fields'] as $field_name => $field) {
      if ($field['type'] == 'text') {
        db_change_field($table_name, $field_name, $field_name, $field);
      }
    }
  }

Here's a patch that collects up the above observations.

I've moved the configuration from the connection config to a general $conf setting. This is because, if we are connecting to multiple databases using slaves, we want to make sure that all connections are configured the same. Additionally, using this $conf setting ensures that we only have *one* setting to change in order to enable utf8mb4 and everything "just works".

Tables now always default to utf8 no matter what, and columns only get tagged with utf8mb4 if they are text fields.

Review and thoughts please!

Two more things that should be added to the above patch:

1. Link to http://dev.mysql.com/doc/refman/5.5/en/charset-unicode-utf8mb4.html in default.settings.php so a user can learn more
2. Check to make sure MySQL is compatible on *install*. I don't think we need to check on every request, because of:
A. We check it on install
B. If a site is migrated from one MySQL server to another that is non-compatible, the SQL import will fail when it encounters the utf8mb4 column charset

Huh, I guess the MySQL testbot is not running MySQL 5.5.3+

+++ b/core/includes/database/mysql/database.inc
@@ -52,14 +52,16 @@ class DatabaseConnection_mysql extends DatabaseConnection {
+    $charset = variable_get('mysql_textfield_character_set','utf8') == 'utf8mb4' ? 'utf8mb4' : 'utf8';

variable_get() is not allowed inside the DB layer. All configuration must come in through the database configuration array only. That's by design so that the DB system is not dependent on the variable system, which itself has a soft dependency on the database.

The Database layer right now is about 95% Drupal-agnostic. The goal is that by Drupal 8's release t is 100% Drupal agnostic.

Thanks Crell,

I will re-roll using global $conf;

No, global $conf is what we should not be using. :-) Such configuration should go into the $databases array, if anywhere.

Here's another patch. Please review.

I think we are getting pretty close. The user can configure the charset in the database settings. We store the charset in the Database connection object as a read-only property so other functions can get this data later.

In particular, I would like someone to review the validateDatabaseSettings method. not sure if I'm doing this right...

Blarg. Re-rolled above patch - removing danling global $conf

{sigh} - re-rolled again (typo).

My apologies for the all the bad patches. Let's try this again shall we.

Fixing notice that testbot found...

Also, just to note that this is going to run smack dab into #1321540: Convert DBTNG to namespaces; separate Drupal bits and conflict. I'd rather get reviews on that sooner than later so that we can get it in, and then do other API changes.

Just some comment cleanup. The interdiff is between patch in #26 and the attached patch.

For those who need this working against the latest stable in D7 (7.10 right now), here's a patch I cooked up. I hope people find it useful.

Also, once you install this patch and change your charset in your settings.php file, you can run this to upgrade all your text fields:

  $schema = drupal_get_schema();  
  foreach ($schema as $table_name => $table) {
    foreach ($table['fields'] as $field_name => $field) {
      if ($field['type'] == 'text') {
        db_change_field($table_name, $field_name, $field_name, $field);
      }
    }
  }

Thanks for the comment cleanup! Here's two other minor docs cleanups to incorporate:

+++ b/core/includes/database/mysql/database.incundefined
@@ -19,6 +19,28 @@ class DatabaseConnection_mysql extends DatabaseConnection {
+   * Flag to indicate our charset. It can either be utf8 or utf8mb4 (adds
+   * support for 4-byte UTF8 characters). See settings.default.php to learn more
+   * about utf8mb4. This is used in combination with the __get method below to
+   * make charset a read-only property of this object.
+   *
+   * @var string
+   */
+  protected $charset = 'utf8';
...
+  /**
+   * This is used to fetch readonly variables. You can not read the registry
+   * instance reference through here.
+   *
+   * @param string $var
+   * @return string
+   */
+  public function __get($property) {

These two docblocks should have one-line summaries. Also, the function's summary should start with a third-person verb. (See http://drupal.org/node/1354#classes).

+++ b/includes/database/mysql/database.inc
@@ -19,6 +19,28 @@ class DatabaseConnection_mysql extends DatabaseConnection {
+  /**
+   * This is used to fetch readonly variables. You can not read the registry
+   * instance reference through here.
+   *
+   * @param string $var
+   * @return string
+   */
+  public function __get($property) {
+    // Only allow access to the charset property.
+    if ($property == 'charset') return $this->$property;
+  }

Woah, woah, why? The magic methods are dangerous magic in most cases. This is a hack. Don't do that. If for some reason you need to expose the charset to the outside world, make it a method.

-18 days to next Drupal core point release.

Hi Crell,

Here's an updated patch with the magic methods removed and replaced with a "charset" method.

Whoops,

The above still had some comments pertaining to the __get method, those comments have been corrected in this one.

+++ b/core/includes/database/mysql/database.inc
@@ -19,6 +19,15 @@ class DatabaseConnection_mysql extends DatabaseConnection {
+  /**
+   * Flag to indicate our charset. It can either be utf8 or utf8mb4 (adds
+   * support for 4-byte UTF8 characters). See settings.default.php to learn more
+   * about utf8mb4. The charset() method can be used to read this property.
+   *
+   * @var string
+   */
+  protected $charset = 'utf8';

Variable docblocks should follow the same rules as any other docblock. Vis, one line summary, empty line, then long description.

This value actually could be, arguably, multiple different values, couldn't it? Not just the two utf8s? If someone sets the charset connection option, they can set it to whatever they want.

No need to mention the method. In fact, most of this documentation should move to the method. Remember from a API perspective, the method is the only thing that exists. There is no property. That's just an implementation detail that a caller should not be able to know about.

-25 days to next Drupal core point release.

Crell,

As per sun's comment above (#3), we only want to support utf8 and utf8mb4 charsets. If you want to override this decision I will make the necessary changes to the patch.

Drupal 7.15, with SQlite 3.7.7.1 or PosgreSQL 9.3.1 (apache 2.2.2),
displays correctly code point U+1D538

Postgresql 9.1: one unicode character UTF_8 can be 1,2,3 ou 4 bytes

http://www.postgresql.org/docs/9.1/static/multibyte.html

Conclusion: if you want to display code point in Supplementary Multilingual Plane, use Postgresql or Sqlite

Wanted to cleanup the patch documentation (see comment #35) and realized that the structure on D8 has changed since the patch so I wonder if this still applies to D8?

#34: mysql_utf8mb4_support-1314214-34.patch queued for re-testing.

Updating tags.

With the help of xjm i rebased it.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -30,6 +30,15 @@ class Connection extends DatabaseConnection {
+  /**
+   * Flag to indicate our charset. It can either be utf8 or utf8mb4 (adds
+   * support for 4-byte UTF8 characters). See settings.default.php to learn more
+   * about utf8mb4. The charset() method can be used to read this property.
+   *
+   * @var string
+   */
+  protected $charset = 'utf8';

See earlier review about how this docblock belongs on the method; this should just be a one line description.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -61,14 +70,16 @@ class Connection extends DatabaseConnection {
+    $this->charset = (isset($connection_options['charset']) ? ($connection_options['charset'] == 'utf8mb4' ? 'utf8mb4' : 'utf8') : 'utf8');

A double-ternary is way too hard to read. :-) Don't do that. Go ahead and spell this out long-form.

Actually, is there a reason to not just set a default for $connection_options['charset'], and then always use that?

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -205,6 +216,15 @@ class Connection extends DatabaseConnection {
+
+  /**
+   * This is used to fetch readonly charset property.
+   *
+   * @return string
+   */
+  public function charset() {
+    return $this->charset;
+  }

See above. The property is irrelevant. The method should contain the real documentation.

0 days to next Drupal core point release.

Thanks @simolokid! Untagging.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -205,6 +216,15 @@ class Connection extends DatabaseConnection {
+
+  /**
+   * This is used to fetch readonly charset property.
+   *
+   * @return string
+   */
+  public function charset() {
+    return $this->charset;
+  }

See above. The property is irrelevant. The method should contain the real documentation.
<?blockquote>
I don't understand this portion here but I made the other corrections.

Also the patch didn't apply when I tried so I rerolled to HEAD as well.

kbasarab: See the first quoted block from comment #45. Some version of that should be moved to the charset() method, which then replaces the "used to fetch readonly property" line (which is meaningless).

The property is not a flag, because it's not a boolean. It should likely be something like "The character set of this connection.\n\nEither utf8 or utf8mb".

The method then has the discussion of seeing default.settings.php and further explanation and so forth.

New patch that has no code changes - this is just my best attempt to fold in Crell's last documentation requests.

The comment in lines 82 and 259 are a bit repetitive (both point to comments in default.settings.php). Not sure if that's okay.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
@@ -252,8 +252,12 @@ protected function popCommittableTransactions() {
+   * Fetch the current character set for this connection.
+   * ¶

For most functions (see exceptions below), summary lines must start with a third person singular present tense verb, and they must explain what the function does. Example: "Calculates the maximum weight for a list."
http://drupal.org/node/1354#functions

so this should be:
Fetches ...

Also there are a few lines with an extra space at the end: trailing whitespace.

patch coming soon.

(The white space was easy to spot using the dreditor review button http://drupal.org/project/dreditor )

Ouch. Please, please, please *no*. Supporting utf8mb4 is great but supporting any non-utf8 charset is a massive pita that I would say we must avoid at all costs. At least search will break with non-utf8 but I can imagine other things too.

Edit: to make sure this is clear: utf8 and utf8mb4 are both fine, but koi8r and all that ancient ....charsets are not.

Solution: for the default connection throw an exception if the charset is set and does not start with utf8.

added summary of additional scope this issue needs in order to avoid a follow-up critical.

needs work to avoid critical follow-up issue.
updated issue summary.
updated issue title.

I wrote the documentation to reflect the code, which is currently very permissive. Upon reflection, it would even accept typos would it not?

If it's the case that it absolutely must be utf8 or utf8mb4 then it should test specifically for those, not just strings that begin with 'utf8'. That way it would catch the case where someone accidentally types 'utf8mb' (e.g. as Crell did in #48).

So it seems that it needs to
- test specifically for utf8 or utf8mb4.
- if it's not one of those, throw an exception and default to utf8

I. am fine with that -- once again, for default. The reason I emphasize default cos the any-charset-goes is an *excellent* opportunity for (careful) migrating from legacy databases.

Updated issue summary, point out need to avoid critical follow-up

I've updated the issue summary to make it easy to figure what's left to do.

I've started work on checking of the charset setting or MySQL and it's pretty much right I hope. The only concern I had is about throwing the exception. I've left the code commented out where I think the throw could happen, but if you let it throw there, then you end up with a white screen as the object is not returned and the error handler does not like null objects.

Perhaps the throw should be further up the chain (in the cache fetch)? Also the exception I've chosen is probably not quite right.

My thoughts after playing with this is that we should probably log it via watchdog as the connection should always work.

This probably needs a bit more thought. As I'm not 100% familiar with the new config stuff and I'm worried I have not respected @chx desire to only override the default setting.

I'd like some feedback, please.

Updated the issue summary

+++ b/core/lib/Drupal/Core/Database/Connection.phpundefined
@@ -135,20 +135,6 @@
-  /**
-   * The character set of this connection.
-   *
-   * @var string
-   */

I think that the interdiff was made a bit ... well, I dont think these were really removed in the patch in comment 81. wait, there is no 81! :) Probably a typo.
if a branch with the patch from 51 was called utf-51, and there was a branch with the patch in 58 called utf-58

then the interdiff could be made by: git diff utf-51 utf-58 > interdiff-51-58.txt (maybe the interdiff above reversed the arguments)

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
@@ -69,14 +68,29 @@ public function __construct(array $connection_options = array()) {
+      if (strcmp($connection_options['charset'], 'utf8') == 0
+        || strcmp($connection_options['charset'], 'utf8mb4') ==0) {

I think any charset starting with utf8 would be ok. That would be more general than just adding two specific allowed ones.

Thanks everyone for moving this forward.

> I think any charset starting with utf8 would be ok.

See #55 for the reasoning -- checking specifically for utf8 / utf8mb4 is a good idea to avoid typos.

Re the new patch ( I attached a real interdiff for easy review): we do not need a new allowedCharset property and method as a disallowed charset should terminate immediately. strcmp() is needlessly complicated, it can be a simply ==. If the error handling is not yet set up, then just print and die, this should be superb rare, you can't hit that error message without misediting settings.php no need to be nice. However, moving the charset method up to the base Connection class is good.

As for default connection, just add self::$databaseInfo[$key][$target]['key'] = $key into Database::openConnection before calling the constructor and then the constructor can easily test for the default key.

I'll try to make a patch tomorrow morning unless someone beats me to it.

The final patch needs/should/could...

1. Add the $key to the connectionOptions array. I have already done this, just need to merge it into the new patch.
2. Limit to utf8 or utf8mb4 for the default connection and be more permissive for all other connections. So for the default connection only it needs to test for the charset, and otherwise it's up to the user to get it right (I think this is reasonable givne that anyone who is defining multiple connections is or should be an advanced user and, as chx said, it offers possibilities for connecting to legacy databases that may be non-Drupal DBs. Useful for migration, for pulling data from some other dataset, etc.
3. Move the mysql-specific comments out of the base Connection class
4. RE "print and die" solution - I got hung up the other day on exception handling and if that's not truly necessary, then that simplifies things.

One comment on item #2 - there are only two charsets that begin with 'utf8' currently available in MySQL. There are many collations and there, if we're going to test for collations, it makes sense to take anything that begins with 'utf8', but I don't think it does with charsets. Consider that utf8mb4 requires special considerations. Let's say MySQL introduces a third utf8 charset - we can't know at this point what special considerations will be required to handle it correctly, so even if it's a valid charset, it should probably be disallowed until the implications are understood. Maybe that's wrong and it's up to the user to test it.

There is, however, an alias for the classic utf8 charset which can now be referred to as utf8mb3. So even though there are only two charsets with uft8 in the name, there are three valid handles for those two charsets. So perhaps utf8mb3 should be allowed too.

In a future version of MySQL, it is possible that utf8 will become the 4-byte utf8, and that users who want to indicate 3-byte utf8 will have to say utf8mb3. To avoid some future problems which might occur with replication when master and slave servers have different MySQL versions, it is possible as of MySQL 5.5.3 for users to specify utf8mb3 in CHARACTER SET clauses, and utf8mb3_collation_substring in COLLATE clauses, where collation_substring is bin, czech_ci, danish_ci, esperanto_ci, estonian_ci, and so forth

src: http://docs.oracle.com/cd/E17952_01/refman-5.5-en/charset-unicode.html

Sure, let's go with that too. And yes, if exception handling is broken then print and die -- it's just a failsafe and not an error anyone will ever see.

One more try. Since this is both my first core patch and I'm just getting started with D8, I hope this is moving things in the right direction. But I'm adding a lot of notes here to hopefully make it easier for people to decipher.

The main principles are the same
- restrictive for default connection
- permissive for all other connections
- added a $connection_options array element 'index' which is the first index of the $databases array. Sometimes this is called 'key' so perhaps that would be a better nomenclature, but I was just following advice from chx on IRC.
- probably other stuff

Exception Handling
The big thing still problematic is agreeing on an exception/error handling strategy. I understand why print and die might make the most sense, but I expect there are a lot of cases where just defaulting to UTF8 and warning the user would save some headaches.

Obviously, the best thing would be a try/throw/catch, but I could not get that to work. If I try to throw an exception (even just using \Exception, I get a fatal error:

( ! ) Fatal error: Call to a member function get() on a non-object in path/to/core/includes/theme.maintenance.inc on line 59

This is true even if I use the base \Exception handler or if I try to use one of the custom ones. There's too much I don't know about exception handling in D8!

That said, for now I have created a drupal_set_message() warning with the message

'The default connnection charset must be "utf8", "utf8mb3" or "utf8mb4". Currently defined as: ' . $connection_options['charset'] . '. Reverting to "utf8". Please check your charset definition in settings.php'

I know that's not the way this should be, but hopefully it moves this patch forward.

Relation to Patch 58

Sorry - I worked off my previous patch mostly and that's what's reflected in the interdiff. Unless I've been misunderstanding all along, I think damienwhaley misconstrued what chx was driving at with respect to the default connection. The point was not to allow a change only for the default connection, it was to be very restrictive on the changes allowed on the default connection and permissive on all other connections.

I did move the charset function and property to base connection class as in #58.

I removed the MySQL-specific comments and refer the user to documentation in default.settings.php. I think it's best for the general documentation for this problem to reside in one place anyway and not be repeated all over. That way if it changes (for example the reference URL) it only needs to be updated once.

I also did not include the allowedCharset() method and deleted the commented code and comments regarding exceptions in /core/lib/Drupal/Core/Database/Database.php.

Other Notes

In my patch, the code in /mysql/Connection.php lines 78-83 is unnecessarily repetitive, but given the long discussion here ( http://drupal.org/node/935284 ) I gather that especially for core it is preferred to keep lines short at the expense of inefficient code (and it does make it more readable, but it feels "wrong" to have to short conditionals that execute the same statement).

I have also added perhaps too much documentation in default.settings.php. I'm not sure how much is too much. Comments welcome.

changing to needs review to let the testbot try it.

Oops, I forgot to change that. But I was looking over the code and realize it still needs work. As is, the patch has this code in mysql/Install/Tasks.php:

    // If we are using utf8mb4 charset, make sure the database supports it.
    if (isset($database['charset']) && $database['charset'] == 'utf8mb4') {
      if (!db_query("SHOW CHARACTER SET WHERE Charset = 'utf8mb4'")->rowCount()) {
        $errors['mysql_charset'] = st('Your database does not support the utf8mb4 character set');
      }
    }

This should be generalized, because for non-default connections we're now allowing any charset.

It also raises another question: should the charset only be validated on install? If so, then the other validation code should be moved here. If not, this check should become part of the code that is validating the connection. Also, though it probably won't stay, the drupal_set_message() call does not wrap a t().

I just realized this can't be used on any column over 191 characters that will be an index because InnoDB only allows 767 bytes on an index. See phayes comments above in #10.

I think there's still a fair bit of work to be done here. Most importantly, any patch needs to be tested to make sure that it actually works with an interactive install and a custom charset/collation in settings.php

I'm not sure any of the patches submitted so far did, because the default collation was getting set to the custom collation and that doesn't work because you get a mismatch on VARCHAR columns. If you change it to use utf8mb4 on VARCHAR cols, then you get lots of crashes because several indexes exceed the 191 character limit (see comment above).

Also there remains the question of how to check that a given charset is in fact supported. This can be done on install, but if someone changes the settings.php file post-install, there's no test in here yet.

So this is probably the best effort I'm going to make without some guidance or someone else to step in and straighten things out. I hope I've made more fixes than problems.

Remove whitespace. Queued for review so it gets tested, but really it's "needs work" - see comments in previous post.

Caution, this is standards stuff, forgive me for distracting here. No need to worry about getting code perfect before posting. Really. I just started on some white space stuff and got carried away.

Patch attached.

+++ b/core/lib/Drupal/Core/Database/Connection.phpundefined
@@ -135,6 +135,14 @@
+  protected $charset = 'utf8';
+
+
   function __construct($dsn, $username, $password, $driver_options = array()) {

@@ -1172,4 +1180,17 @@ public function commit() {
+  }
 }
+
+

+++ b/sites/default/default.settings.phpundefined
@@ -165,6 +165,40 @@
+ * 'utf8mb4' character set on all text columns. More information on 'utf8mb4'
+ *  can be found here:
+ * http://dev.mysql.com/doc/refman/5.5/en/charset-unicode-utf8mb4.html

extra whitespace

+++ b/core/lib/Drupal/Core/Database/Connection.phpundefined
@@ -1172,4 +1180,17 @@ public function commit() {
+   * Fetch the current character set for this connection.

function one line summaries should begin with third person verbs, like: Fetches

+++ b/core/lib/Drupal/Core/Database/Connection.phpundefined
@@ -1172,4 +1180,17 @@ public function commit() {
+   * documentation in sites/default/default.settings.php for more information.
+   * @return string

@params have not newlines between them, but @return always has a newline before it. http://drupal.org/node/1354#order

And has a description on the second line.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
@@ -69,14 +69,35 @@ public function __construct(array $connection_options = array()) {
+// Default to 'utf8', but allow user to change this to
+    //  - any user-defined charset if this is not the default connection
+    //  - any allowed charset if this is the default connection
+    // See sites/default/default.settings.php for full documentation

missing spaces in the Default line, to align it.

missing periods at end of sentences.

I'm not sure of the format for lists inside inline comments.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
@@ -69,14 +69,35 @@ public function __construct(array $connection_options = array()) {
+    $default_connection_charsets = array('utf8', 'utf8mb4', 'utf8mb3'); //utf8mb3 is an alias for utf8

it's in core in some places, but http://drupal.org/node/1354#inline says to put in-line comments on its own line.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
@@ -69,14 +69,35 @@ public function __construct(array $connection_options = array()) {
+    if (isset($connection_options['charset']) && $connection_options['charset'] != 'utf8' ) {

expressions don't use space before the parens (). http://drupal.org/coding-standards#controlstruct

Also, I dont know the order of precedence between && and !=, so maybe an extra set of parens () would help make the meaning clearer. [I did not fix this one.]

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
@@ -69,14 +69,35 @@ public function __construct(array $connection_options = array()) {
+        drupal_set_message(t('The default connnection charset must be "utf8", "utf8mb3" or "utf8mb4". Currently defined as: ' . $connection_options['charset'] . '. Reverting to "utf8". Please check your charset definition in settings.php'), 'warning');

use arguments to t() to make it easier on translators.

http://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functio...

for example: $text = t("@name's blog", array('@name' => user_format_name($account)));

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.phpundefined
@@ -82,4 +82,21 @@ protected function connect() {
+        $errors['mysql_charset'] = st('Your database does not support the '.$database['charset'].' character set');

I'm not sure, but I think st() might be the same as t() in that should use args for variables instead of string cat. http://api.drupal.org/api/drupal/core%21includes%21install.inc/function/...

+++ b/sites/default/default.settings.phpundefined
@@ -165,6 +165,40 @@
+ * Finally, it is possible to use 'utf8mb3' which is currently simply an alias of

more than 80 chars.

+++ b/sites/default/default.settings.phpundefined
@@ -165,6 +165,40 @@
+ * 'utf8', but MySQL reserves the right at a future date to make 'utf8' default
+ * to a 4-byte character set at which point 'utf8mb3' would specifically
+ * indicate the legacy 3-byte version.

broke up this sentence into two, and added comma to separate phrase to add clarity.

+++ b/sites/default/default.settings.phpundefined
@@ -165,6 +165,40 @@
+ * utf8mb3. Other charsets are allowed on other connections at the users risk

missing period.

Sorry about that and thanks so much for doing all that cleanup. I'll try to be more careful in the future.

>>I dont know the order of precedence between && and !=

the != takes precedence, so this works as expected, but for clarity, perhaps the extra parens would help.

Next:

need a review from @Crell or someone to make sure previous concerns have been addressed in the right way, and/or maybe from @chx on the more recent bit added.

if the approach is ok, then a little cleaning up of the comments will be needed, there is some debug or other notes in there.

My feeling is that it needs more than cleaning up comments. The comments are long and have debug type stuff in there because I'm not sure of several things.

I stopped where I did because I had a headache and had reached the point where I felt like I was making things worse, not better. I think it needs a fair bit of massaging, but I can't really get back to it until next week.

It also needs either a very robust test or it needs to be tested by installing from scratch in a variety of scenarios (or both) - a small change here or there in this patch can easily cause a crash on install. If you get a column mismatch of any sort, MySQL will die.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -69,14 +69,36 @@ public function __construct(array $connection_options = array()) {
+        drupal_set_message(t('The default connnection charset must be "utf8", "utf8mb3" or "utf8mb4". Currently defined as: @name. Reverting to "utf8". Please check your charset definition in settings.php', array('@name' => $connection_options['charset'])), 'warning');

drupal_set_message() inside the Connection class is a no-go. Just throw an exception and let the global error handler deal with it.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -156,6 +162,15 @@ protected function createFieldSql($name, $spec) {
+    // If it's a text field, check to see if we should use utf8mb4 (4-byte UTF8)
+    // as the character set.
+    // InnoDB indexes have a max of 767 bytes. This means we can't use 4-byte
+    // charsets on VARCHAR because there are VARCHAR-based indexes of 255 chars.
+    if (in_array($spec['mysql_type'], array('TINYTEXT', 'MEDIUMTEXT', 'LONGTEXT', 'TEXT')) &&  Database::getConnection()->charset() == 'utf8mb4') {
+          //    isset($info['charset']) && $info['charset'] != 'utf8') {
+      $sql .= ' CHARACTER SET '.$info['charset'].' COLLATE ' . $info['collation'];

I don't fully understand the implications of this comment. Does that mean utf8mb4 + InnoDB + Varchar == kaboom? That would be... very very bad. I hope I'm misunderstanding that.

Have I mentioned how much I hate SQL databases?

Note we may want to change the SET NAMES call to something that the driver supports natively:

http://www.php.net/manual/en/ref.pdo-mysql.connection.php

(Yes, it's deeply buried in the docs. I didn't even know about it until today.)

Does that mean utf8mb4 + InnoDB + Varchar == kaboom?

If the column has more than 191 characters, yes, it goes kaboom. See phayes comments in #10.

That would be... very very bad.

Yes, it would be. Maybe it's just because I don't have the chops for this, but the more I look into this, the more utf8mb4 support seems like a morass.

> because there are VARCHAR-based indexes of 255 chars.

Then we should ban that and establish sensible indexes. There's no way we need more than, say, 32 chars to index and every db and Drupal totally supports index lengths. Might be a followup.

I was not reading my own comments - I was thinking the column length was the problem, but yes, it's just the index prefix length at issue and certainly 191 characters should be enough for that.

Re #75 it was not buried rather it didn't exist: Prior to PHP 5.3.6, this element was silently ignored. Note that Drupal at this moment is 5.3.5 so using charset in the DSN is not yet possible. Plans are to go 5.3.10 in March.

Roughly speaking

• 135-140 varchar cols that are indexed (see * below for how I arrived at this)
• 82 with lengths of 128 or more (so failure in a two-col index with any col over 63)
• 41 with lengths of 191 or more (so failure all by themselves)
• 0 currently specify prefix lengths

That's the rough scope of it, so it's not hard, though it does end up being a patch that affects a very large number of files. It will also require contrib authors to update their modules. I can try to come up with another patch, but before I go changing all those files, I just want confirmation that approach is sensible.

I don't find any indexes that are specified with a length currently, but as chx says, it's built into Drupal and is a simple matter of changing, for example, the key_value table

'primary key' => array('collection', 'name')

to

'primary key' => array(array('collection', 60), array('name', 128))

and choosing sensible values. In a default install of D8, the largest value for key_value.collection is 13 chars ("system.schema") so it seems to make sense there, for example, to leave name at 128 and then use what's left. There may be other cases where both columns need truncating. I haven't looked that carefully yet and wanted a reaction from others following the thread before going down that road.

PS I don't think just exempting the VARCHAR cols from utf8mb4 support as phayes suggested in #10 really makes sense. Some of these columns are things like users.signature and it seems like supporting a character set for user names is rather important.
---------

*I arrived at this very roughly by writing a small script that does a SHOW TABLES and then iterates through the tables and grabs all columns that are of type VARCHAR and which have a Key (as per the SHOW COLUMNS) output (and in the latter two cases, that have a length above the given size. I also just grepped the codebase using ^[^\*\n\r]+'(primary key|indexes|unique keys)' =>.*?[;,] and got a second count that way.

Do we have an actual use case for this? I could not see one by quickly skimming through the comments.

Until we have, it seems like the drawbacks clearly outweight the benefits here.

Also, a primary key cannot use length prefixing. (Or at least I hope that MySQL doesn't allow that, because it doesn't make any sense.)

It's probably worth tidying up our schema (for example: machine name-type keys should probably use a ascii character set so as to reduce the size of the index), but that belongs in another issue.

a primary key cannot use length prefixing

Actually, it can: http://dev.mysql.com/doc/refman/5.5/en/alter-table.html
I tested it and had no problem altering a table to use a 100 char PK based on a 255 char VARCHAR column.

And this is all related to #1852896: Throw an exception if a schema defines a key that would be over 1000 bytes in MySQL - it seems one way or another some sort of length checking on MySQL keys might be needed.

Do we have an actual use case for this?

I'm not sure what the OPs use case was nor that I understand the internationalization issues well enough, but I believe the use case goes something like this:

User wants to use a characters not included in the Basic Multilingual Plane but included in the Supplementary Linguistic Plane. This would include some CJK Unified Ideographs, obscure languages (Egyptian Hieroglyphics), and some modern character sets (emoji, emoticons).

- http://en.wikipedia.org/wiki/Plane_%28Unicode%29#Supplementary_Multiling...
- http://mzsanford.wordpress.com/2010/12/28/mysql-and-unicode/
- http://mathiasbynens.be/notes/mysql-utf8mb4

it seems like the drawbacks clearly outweight the benefits here.

That's sort of what I was driving at in #76 - this seems like a lot of trouble, but as it stands now (as I understand it), Drupal is not really compatible with 4-byte character sets.

It looks like over in the Wordpress community they're fighting with the same thing.

My use case of this is that we host a lot of mathematical scientific journals, there are a lot of high order characters that are used in this content for properly displaying math. I've been running the patch in #12 for about a year now in production with no problems.

Edit: Is there a reason why we don't just say we only support utf8mb4 on text fields (not varchar) and skip the whole key-length problem. It's not a 100% ideal solution, but it's better than providing no support for high order characters.

phayes: Mixed character set databases are a nightmare to manage and lead to all sorts of creative bugs unless you really know what you're doing. You may really know what you're doing, but I wager most people using MySQL don't know what a character set is, much less what happens when you try to mix them. :-)

I tested it and had no problem altering a table to use a 100 char PK based on a 255 char VARCHAR column.

I don't know what it does exactly, but it's probably not desirable.

I'm going to reclassify this as a feature request. There is no data loss related to this since Drupal 7, so it is not a bug per say.

In addition, I don't think we can support this until we introduce support for an ascii or binary charset and use it to tidy up our indexes and primary keys (especially machine names and UUIDs that have poped up all over the place in Drupal 8).

#1923406: Use ASCII character set on alphanumeric fields so we can index all 255 characters

Before I answer this I want to reemphasize - much of this is over my head in terms of both Drupal 8 internals and in terms of character sets (I only actually use French and English, so these issues don't affect me, I just ended up taking this on as somewhere to try to help with D8).
That said...

Edit: Is there a reason why we don't just say we only support utf8mb4 on text fields (not varchar)

Not using any Asian languages myself, I don't know how common the extensions beyond the Basic Multilingual Plane are. Mostly what you see in the SMP are special uses (Math, emoticons, alchemical symbols), dead languages (Lycian, Gothic, Cuneiform) and the unified Han characters.

But it seems to me that if you're supporting these characters in TEXT, they should be supported in VARCHAR for the cases where

• someone wants to use such a character in a node title (could there be a situation where a mathematical symbol would appear in the title too?)
• someone wants to use one of the Asian characters in a username (e.g. it's part of their real name)

Wouldn't it be confusing to a user that say that if you enter text into the body of a post, it's all good, but if you copy and paste it into the title of that post, it will truncate your data as soon as it encounters the first 4-byte character?

BTW - support for these character sets is shaky at best in the world at large. The emoji, for example, are supported in Win8 fully in IE, Firefox and Opera, but not in Chrome or Safari. Support on the Mac seems to be a little less. My Ubuntu is busy upgrading, so I can't tell.

Also, I came across one article using a 4-byte character in the title - it rendered fine in the Google search results, but the link was not clickable.

#75, #79:
#1800122: Bump minimum version of php required to 5.3.10 has been committed, so we can use DSN now, instead of 'SET NAMES'.

#80:
From what I got the 767 Bytes (= 191 chars) InnoDB restriction isn't about index length but about the length of a (single) prefix.
Otherwise, key length may be up to 1000 B (= 250 chars) as restricted by MyISAM, see #1852896: Throw an exception if a schema defines a key that would be over 1000 bytes in MySQL.
We might need to test this again, unless someone can clarify.

#81:
Another use case are LTR tags allowing English text be correctly embedded in an RTL environment. We're having bugs that can't be correctly solved without this, see #1165476: if t() string has no translation or fallback language, text should have lang attribute.

#86:
Agree that we should avoid length prefixing on PKs that would be opening up one more can of worms. The more we should go and shorten keys.
Plus: While It's certainly true that there is no data loss, which is the criterion for a critical bug though, not generally for a bug. Still I'm recategorizing only as task, because it's not like everybody would expect us to support 4 byte chars. But "feature" this isn't either.

#70: database-1314214-70.patch queued for re-testing.

@Pancho: LTR and RTL are both 3-byte UTF-8 characters. The linked issue describes the use of language tags from U+E0000 to U+E007F, which are totally deprecated anyway.

@Pancho

1. key length versus prefix length.

My mistake - you are correct

• By default, an index key for a single-column index can be up to 767 bytes. The same length limit applies to any index key prefix.
• The InnoDB internal maximum key length is 3500 bytes, but MySQL itself restricts this to 3072 bytes. This limit applies to the length of the combined index key in a multi-column index.

-- http://dev.mysql.com/doc/refman/5.5/en/innodb-restrictions.html

Great work here. This patch could fix this issue.

It might also need a follow up. I think we should reconsider this as a bug as Drupal isn't fully utf-8 compliant.

This issue is not only theory, but also relevant for
- emoji, these emoticons are supported by Google and Apple on mobile phones and use the 4 bytes utf-8. Some applications even use emoji as part of the username
http://blog.manbolo.com/2011/12/12/supporting-ios-5-new-emoji-encoding
- mathematics used in scientific journals: http://drupal.stackexchange.com/questions/50868/configuring-drupal-to-us...

Some questions
- could we require MYSQL >5.5.28 for Drupal 8?
- There is no workaround if people can't or don't use utf8mb4:
- The actual behavior with normal utf8 is for user content is that your text is lost and no user feedback is given. I think we should reconsider this as a bug and we need to give user feedback.
- when importing data from external sources the import fails. Instead we could skip, escape, or replace these characters when sending to the database. This needs further research. See for example this bug when importing Tweet data: #1824506: When importing Tweets: SQLSTATE[HY000]: General error: 1366 Incorrect string value
- we could clarify in the system report which characterset is used, and if utf8 with mysql is used place a warning that it isnt fully compatible.
- could we test if this bug also exists with other databasesystems like PostgreSQL, SQLite and Oracle?

As I said in #86:

In addition, I don't think we can support this until we introduce support for an ascii or binary charset and use it to tidy up our indexes and primary keys (especially machine names and UUIDs that have poped up all over the place in Drupal 8).

Increasing the size of all our indexes by 33% is not something we can afford. Let's open a separate issue for the tidying up and postpone this one on it.

- The actual behavior with normal utf8 is for user content is that your text is lost and no user feedback is given. I think we should reconsider this as a bug and we need to give user feedback.

This should not happen. If it does, it's a bug elsewhere. The database layer triggers an exception, that's all that it does. Catching it and processing it belongs in the upper layers. If they don't do that correctly, it's a bug there, not in the database layer.

This should not happen. If it does, it's a bug elsewhere.

Just checked with a fresh install and pasting a unicode 4 bytes character in a post gives a crash. Will post the bug in a seperate issue.
EDIT: #2002100: pasting text with 4 byte UTF-8 characters leads to broken screen

Similar ticket and discussion in Wordpress:
http://core.trac.wordpress.org/ticket/21212 MySQL tables should use utf8mb4 character set
Is the by Pento mentioned solution in that ticket to use the option innodb_large_prefix something to consider?

The 767 byte limit won't be increased any time soon, due to ​http://bugs.mysql.com/bug.php?id=32915

It turns out this is possible, thanks to the innodb_large_prefix option, introduced in 5.5.14:
​http://dev.mysql.com/doc/refman/5.5/en/innodb-parameters.html#sysvar_innodb_large_prefix

So, the requirements are:

MyISAM tables:
MySQL >= 5.5.3 (Assuming we don't add any indexes larger than 250 characters.)

InnoDB tables:
MySQL >= 5.5.14
innodb_file_format=barracuda
innodb_file_per_table=true
innodb_large_prefix=true
All tables with ROW_FORMAT=(DYNAMIC|COMPRESSED)

Any other table formats:
*head explode*

We can certainly detect these settings, it's just a question of whether that kind of complex (and edge-case-y) test should be in core.

Rerolled, wanted to try this out. These emoji characters are not going away any time soon it seems.

This doesn't work any more after the database connections have become static in #1953800: Make the database connection serializable.

So is there another way round this?

Updated issue summary, took out duplicate and integrated the added problem motivation

For prior art: the way this problem was handled in mediawiki was to store strings in VARBINARY and let the application handle character-sets. This was a pre MySQL 5.5 decision, and it's important to point out that it means no collation which is the bigger deal.

With collation:
'Montréal' == 'MONTREAL'
+ a consistent sorting order is provided with multi-byte characters.

It may be an option to 'degrade' to VARBINARY for versions older than MySQL 5.5. I'll leave this one for discussion.

RE: Index size limited to 191 characters discussion
There is actually a workaround to this problem via prefix indexes. i.e.
ALTER TABLE my_table ADD INDEX text_col_index (text_col(190));

This isn't suitable for PRIMARY or UNIQUE indexes since it breaks a constraint, but it should work fine for others.

I've updated the patch for Drupal 7 from #30:
- to Drupal 7.28
- applying all the remarks, but limited to utf8 and utf8mb4 (no custom encodings)

To apply:
1) backup the database
2) apply the patch
3) add the settings to your settings.php file
4) run this to upgrade the text fields:

  $schema = drupal_get_schema();
  foreach ($schema as $table_name => $table) {
    $repair_needed = FALSE;
    foreach ($table['fields'] as $field_name => $field) {
      if ($field['type'] == 'text') {
        db_change_field($table_name, $field_name, $field_name, $field);
        $repair_needed = TRUE;
      }
    }
    // According to http://mathiasbynens.be/notes/mysql-utf8mb4 when upgrading
    // it's also necessary to repair and optimize the tables.
    if ($repair_needed) {
      db_query("REPAIR TABLE $table_name");
      db_query("OPTIMIZE TABLE $table_name");
    }
  }

Dear All,

Thanks for the patch for changing character set to utf8mb for columns with Text data type. I believe this would allow supplementary characters to be added to the node body. However I believe we will have issues if the user enters these supplementary characters in Menu Title, Node Title, Tags etc. Do we have any solution to take care of these fields as well?

Thanks & Regards,
Deepan Choudhary

We do not have a solution. See the patch:

+    // InnoDB indexes have a max of 767 bytes. This means we can't use 4-byte
+    // charsets on VARCHAR because there are VARCHAR-based indexes of 255 chars.

What about the innodb_large_prefix and associated settings available in MySQL >5.5.14 mentioned in #97? Could this just be a manually enabled option and no autodetection or anything?

That would be swell. MySQL 5.1 was EOL'd on December 31, 2013. Maria will EOL 5.1 on 1 Feb 2015. Are we ready to raise the requirements to 5.5?

Edit: don't forget you need the Barracuda format for that and innodb table per file. Perhaps we need to test creating a table with a long index? Put barracuda in the table options?

Given the number of other legacy versions we're dropping in Drupal 8 (PHP, IE, etc.) I would be OK with moving to MySQL 5.5 as the minimum if it bought us something. Would switching to MySQL 5.5 make this problem effectively go-away? That's what it sounds like but I want to confirm...

Weeeeeell. If your MySQL 5.5 InnoDB is set to use innodb_file_per_table and uses the Barracuda innodb_file_format http://dev.mysql.com/doc/refman/5.5/en/innodb-parameters.html#sysvar_inn... then your problem goes away. Fun.

This is the default file format in MySQL 5.6 though (and also the default between MariaDB/MySQL 5.5.0 and 5.5.6 but since 5.5.7 has reverted back to Antelope).

@Crell: Yes, it would allow you to use utf8mb4 safely, with a couple of other issues to solve. To summarize the now quite long comment thread:

In comment #10 phayes noted that using utf8mb4 creates a secondary issue - it restricts the maximum index size of InnoDB tables to 191 characters. In comment #97 Hanno noted that there is an option innodb_large_prefix to supported larger indexes, but we should not assume all 5.5 users will be able to turn this on (InnoDB does not enable it by default for file-format backward compatibility).

There is a workaround suggested by ergophobe in comments #78 and #80, which is to use prefix indexes (index on the first 191 characters). There are little downsides to doing this: most strings will have enough selectivity in the first 191 characters. It may prevent 'covering index' optimizations, but I am not sure how many of these drupal has.

Also as chx notes in comment #108, upstream has dropped support for MySQL 5.1 (released November 2008). The latest Ubuntu release support MySQL 5.5 (2010) & 5.6 (2013). Red Hat 7.0 is MariaDB 5.5. So I would +1 a suggestion to make 5.5 the minimum.

@margantocker - thanks for the concise summary of a super long discussion.

There is the question you mention of whether 191 chars is enough to provide specificity (it's certainly conceivable that, say, some ridiculously long numeric series could have all the significant information at the end).

But in addition to that, I can't figure out from the documents what happens if I have a 191-char prefix and I search for a 200-char string. Does it return if the first 191 match?
http://dev.mysql.com/doc/refman/5.5/en/mysql-indexes.html

Please keep in mind #95. Long indexes are bad, they use more memory, storage and are less efficient. The limit in InnoDB is there for a reason.

What needs to happen is that we need to stop using Unicode columns (and indexes) where they are not necessary. This is a comprehensive change that cannot be workaround by simply bumping database version requirements.

@ergophobe: I have tested prefix-length on various string types on various data-sets before. If we take IMDB movie title names, the first 15 characters provides around 99% of the cardinality of an index on the full length. An edge-case where this is less-true is URLs, where uniqueness starts after the first 11 characters (http://www.). If the URLs all start with the same domain, this could be the first 50 characters... but I would say that's a less common case.

To answer your question:
In the event that MySQL can not filter rows via the index, it will filter at the row-level, so there is no risk of wrong results. It is simply a risk of following too many pointers from an index to a row, only to eliminate the need for it there (performance).

@Damien: I find mixing and matching character-sets to be a micro optimization. I have done it before, and no longer recommend it. InnoDB internally will use variable length storage for utf8 character-set, so from a storage perspective there is no added cost. In memory-temporary tables can not do variable length, and may take more memory or perhaps convert to disk more frequently.. but in the typical case I've found that drupal queries are well optimized here, and I would be surprised to see measurable difference.

The problem with shortening index prefixes, is that you need to fully examine the dataset to determine the prefix-length. And in many cases, Drupal users will have varying datasets. Long indexes are worst in the case of an index-scan (select avg(x), count(x) etc.), so more attention could be paid to optimization here. In other cases, InnoDB will usually only load the index pages into memory as required, so it largely becomes a storage problem. The worst situation I could see is a micro-optimization to shorten the prefix length, and some installations of Drupal can not use the index because it is not selective.

Thanks for the clarification - that's reassuring

Contrib related issue, saving tweets with Emoji as mentioned in #98. Adding to related issues.

Adding another contrib related issue with emojis

A little bit more MySQL version context on innodb_large_prefix:
- It was created so that MySQL downgrades would be possible (i.e. 5.5 -> 5.1)
- This purpose is now obsolete, since 5.5+ support large prefix (and 5.1 is no longer officially supported).
- We are proposing[1] to enable innodb_large_prefix by default in MySQL 5.7, since it is now a safe change.

So I realize it's a while out for many people, but the story should be simpler with MySQL 5.7.

[1] I have the call out on my blog here:
http://www.tocker.ca/2015/01/05/what-defaults-would-you-like-to-see-chan...

Hi

Now we do have a workaround module as a fix for this issue.

Strip 4-byte UTF8

https://www.drupal.org/project/strip_utf8mb4

We do have an interface for it too.

Rewarding time working on this fix :)

.

Maybe relevant: Wordpress managed to fix this 2 months ago: https://core.trac.wordpress.org/ticket/21212#comment:15

Also, the next development release for MySQL 5.7 will be a Release Candidate and will likely have the folllowing changes, supporting large enough indexes on utf8mb4 out of the box:

Important Change; InnoDB: The following changes were made to InnoDB configuration parameter default values:

• The innodb_file_format default value was changed to Barracuda. The previous default value was Antelope. This change allows tables to use Compressed or Dynamic row formats.
• The innodb_large_prefix default value was changed to ON. The previous default was OFF. When innodb_file_format is set to Barracuda, innodb_large_prefix=ON allows index key prefixes longer than 767 bytes (up to 3072 bytes) for tables that use a Compressed or Dynamic row format.

https://dev.mysql.com/doc/relnotes/mysql/5.7/en/news-5-7-7.html

Before we can support this though, if I can quote Damien Tournoud:

It's probably worth tidying up our schema (for example: machine name-type keys should probably use a ascii character set so as to reduce the size of the index), but that belongs in another issue.

Long indexes are bad, they use more memory, storage and are less efficient. The limit in InnoDB is there for a reason.

What needs to happen is that we need to stop using Unicode columns (and indexes) where they are not necessary. This is a comprehensive change that cannot be workaround by simply bumping database version requirements.

In addition, I don't think we can support this until we introduce support for an ascii or binary charset and use it to tidy up our indexes and primary keys (especially machine names and UUIDs that have poped up all over the place in Drupal 8).

Increasing the size of all our indexes by 33% is not something we can afford. Let's open a separate issue for the tidying up and postpone this one on it.

So anyone correct me if I'm wrong but I think this would have to be postponed on #1923406: Use ASCII character set on alphanumeric fields so we can index all 255 characters.

Further updating issue summary

We now have a patch over at #1923406: Use ASCII character set on alphanumeric fields so we can index all 255 characters so if anyone could chime in it might unblock this issue :)

Considering how prevalent UTF-8 characters from the additional character planes are these days (emojis, mathematical characters etc) shouldn't this actually be considered a bug?

Now that we have a workable patch in the blocking issue, would something like this work for a patch for this issue:

- If using MySQL, detect in the installer whether MySQL supports utf8m4, innodb_large_prefix=on and engine=Barracuda. If so:
- SET NAMES = utf8mb4 in the connection object and use utf8mb4 instead of utf8 in the schema, as well as row format = DYNAMIC.
- If MySQL has other settings or an override was provided in settings.php, use utf8 instead of utf8mb4.

So I've discussed this at the Dev Days and @Damien Tournoud and @pwolanin mentioned that after we get the ASCII issue in, this would actually be easily solved by raising the MySQL requirement to 5.5.3 and using 191 characters on the few remaining UTF-8 indexes that are left. We can then just use utf8mb4 by default in the MySQL driver, without worrying about InnoDB/row settings.

I have created a new issue to increase the minimum required version of MySQL.

This patch replaces all occurrences of utf8 with utf8mb4 when this is related to database encoding.

Increasing the size of all our indexes by 33% is not something we can afford. Let's open a separate issue for the tidying up and postpone this one on it.

I've clarified in #1923406 where the increase in size will be (it's not a direct 33% increase). I will comment in #2473301 on raising the minimum version - it's quite a good solution imho.

#129 seems close to what we want... After we fix:

1. #2473301: Raise MySQL requirement to 5.5.3
2. #1923406: Use ASCII character set on alphanumeric fields so we can index all 255 characters
3. Reducing index size to 191 characters on remaining non-binary/ascii indexed fields, or remove the index if it wasn't actually necessary.

1. This still needs a test to verify we store/display utf8mb4 characters correctly.

2. As soon as #1923406: Use ASCII character set on alphanumeric fields so we can index all 255 characters goes in it will also need to test for the standard utf8mb4 collation in SchemaTest.

3.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -85,27 +85,27 @@ public static function open(array &$connection_options = array()) {
 
     // Force MySQL to use the UTF-8 character set. Also set the collation, if a
-    // certain one has been set; otherwise, MySQL defaults to 'utf8_general_ci'
+    // certain one has been set; otherwise, MySQL defaults to 'utf8mb4_general_ci'
     // for UTF-8.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -101,22 +101,22 @@ protected function createTableSql($name, $table) {
     // By default, MySQL uses the default collation for new tables, which is

80 cols, s/UTF-8/utf8mb4/

Added note about Drupal system requirements as todo

That note can actually be removed, after #1923406: Use ASCII character set on alphanumeric fields so we can index all 255 characters goes in we can limit the remaining few UTF8 keys (if any) to 190 characters, that way we can also support installs without innodb_large_prefix.

Updated issue summary.

This option looks like a better option for D7. #2382707: UTF8MB4 for MySQL Maybe we can re-open it to get some support there too?

Reason I say that is because here we are setting the default to utf8mb4, where as there it's default stays utf8 but has the option to change it if a site wants to go through with it.

This patch adds a test, addresses comments in #134 and limits the following fields with unique constraints to 191 characters:

• title/name of website providing a feed
• description of block content field (widget is updated automatically)
• URI of file field
• username field (which is limited to 60 characters anyway)

If we have a problem with the URI field only allowing for 191 characters, we could also get rid of the database-level constraint and add it on the application level instead.

In core we don't use any non-ASCII primary keys anymore, nor do we need to explicitly define length for regular indexes in PHP, as MySQL will take care of that for us anyway; it will cut off regular indexes at 191 characters without innodb_large_prefixes and at 255 characters with innodb_large_prefixes.

This should fix some failing tests as we had skipped the test-only tables in #1923406: Use ASCII character set on alphanumeric fields so we can index all 255 characters

This should further decrease test failures by marking some more fields as ASCII.

Missed another test field.

@joelpittet as to the D7 version of this patch, @catch had proposed in #2473301: Raise MySQL requirement to 5.5.3 that we allow people to use either utf8 or utf8mb4 in D7, if @David_Rothstein is OK with that.

This removes the unique keys from feed title and block content info as reducing field size to 191 could break D6/D7 upgrades.

Looks like this test run failed because #1923406: Use ASCII character set on alphanumeric fields so we can index all 255 characters had been reverted.

Whenever that gets back in I'll also remove the length cutoff from the URI field on File entities. Instead we can make it an ASCII field and urlencode it, as both removing the unique constraint and cutting off /all/ URL's at 191 characters seem like even worse options.

After we do that I think the patch is close to where it needs to be, this will just need some feedback from @benjy regarding how this will fit in with migrate.

This implements urlencoding on the storage layer for non-ASCII URLs, as it got too ugly trying to do this on the entity layer. DownloadTest has coverage for non-ASCII URLs.

This still looks a bit ugly to me. Maybe it would be better to do a query looking for file entities with the same URI in File::preSave() and we just lose the unique constraint?

Actually I think it may be better if we add a uri hash field and put a unique constraint on that field instead?

This adds a hash as well as a test for the hash and the uniqueness constraint on the hash. The hashing still feels like a better solution than the urlencoding as it doesn't change what is stored in the URI field, as well as allowing us to save URLs longer than 255 characters (see #193954: {file}_uri and {file}_filename length limitations)

I think the new hash field would need a beta to beta upgrade hook though.

If we go for URI encoding,it should be storage layer, the application layer should not have to be bothered with this kind of technical details. So #157 was an improvement.

However, as URIs can indeed become incredibly long and only differ in some query parameter values after position 191, I think that using a hash might even be better. I suppose that performance wise, computing a hash is not that expensive. So continue with #160.

+++ b/core/modules/migrate_drupal/src/Tests/Table/d6/System.php
@@ -6,7 +6,7 @@
  *
  * THIS IS A GENERATED FILE. DO NOT EDIT.
  *
- * @see cores/scripts/dump-database-d6.sh
+ * @see core/scripts/dump-database-d6.sh
  * @see https://www.drupal.org/sandbox/benjy/2405029
  */
 
@@ -26,7 +26,7 @@ public function load() {

@@ -26,7 +26,7 @@ public function load() {
       ),
       'fields' => array(
         'filename' => array(
-          'type' => 'varchar',
+          'type' => 'varchar_ascii',
           'not null' => TRUE,
           'length' => '255',
           'default' => '',

- Don't bother changing generated files.
- This seems like a D6 table, so it should not be changed anyway.
- The typo should be corrected in core\scripts\migrate-dump-d6.sh,but not as part of this issue.

@fietserwin, thanks. The D6 table has to be changed as well, as it is being created as a utf8mb4 table with a primary key on the filename field. If we don't change it, it only allows 191 characters. This is being changed both in the generated script (so tests can pass) as in the generating script, but indeed the typo won't be picked up :)

Not sure why this test run is marked as failed, in the test log it just keeps terminating in the middle of the node tests, but it doesn't say which specific test is breaking. I'll log a ticket in the testbot queue.

A few nitpicks in the mean time

Apparently this may be a testbot problem as opposed to a problem in the patch: #2477583: Unexpected timeouts on testbot runs

I'll just keep re-queueing this patch then

Ah tests are green now, #166 is not that different to the previous patches so this may have been a testbot problem after all.

Updating issue summary.

Updating issue summary to clarify why we can lose the DB-level unique constraints on block description and feed title.

The uniqueness validation is just there because there's no way to distinguish between blocks/feeds in the UI other than by description, and we already have entity validators for this, nor is it a big deal if we still get two blocks with the same description in some edge case. All it'd be is an inconvenience, and workarounds exist (such as editing the description).

Just to add to this, the patch is MySQL only, but there are two tests which may affect PostgreSQL / SQLite: NodeViewTest and SaveTest. For now @amateescu has confirmed that SaveTest works with SQLite, the other 3 cases are yet to be confirmed.

Just tested now and both tests are fine on SQLite and Postgres.

Thanks @amateescu! That also proves the PostgreSQL and SQLite drivers aren't affected by this issue and already support full UTF-8.

Having done another review I'm happy with this patch now. Tiny coding standards fix attached.

Just waiting for one of the database people to have a look at this and for @benjy to check this will play well with migrate.

Oops

This all looks fine to me from a migrate POV.

Added "font icons" to the list of lacking support and in text references to prerequisite issues.

1. +++ b/core/modules/aggregator/src/FeedStorageSchema.php
   @@ -33,7 +33,7 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
   -          $this->addSharedTableFieldUniqueKey($storage_definition, $schema);
   +          $this->addSharedTableFieldIndex($storage_definition, $schema);
              break;
   

   Pass TRUE for the 3rd param (not_null)? (unique fields are not null by default, but for other fields you have to pass that explicitly)

2. +++ b/core/modules/file/src/FileStorageSchema.php
   @@ -30,6 +30,9 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
   +          $this->addSharedTableFieldIndex($storage_definition, $schema, TRUE);
   +          break;
   +        case 'uri_hash':
              $this->addSharedTableFieldUniqueKey($storage_definition, $schema, TRUE);
              break;
   

   addSharedTableFieldUniqueKey() does not accept a 3rd param (it is always not null).

3. +++ b/core/modules/file/src/FileStorageSchema.php
   @@ -30,6 +30,9 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
   +        case 'uri_hash':
              $this->addSharedTableFieldUniqueKey($storage_definition, $schema, TRUE);
              break;
          }
   

   idem.

4. +++ b/core/modules/file/src/Tests/SaveTest.php
   @@ -67,10 +70,25 @@ function testFileSave() {
   +      $this->assertTrue($exception_triggered, 'SQL uniqueness constraint is triggered');
   +    }
   

   warning: Variable 'exception_triggered' might not have been defined.

Thanks for the review and the issue summary update @fietserwin.

Actually 2 and 3 may be referring to the same line of code? Or maybe to the code we are removing, as it is currently unnecessarily adding a 3rd parameter in HEAD as well:

- $this->addSharedTableFieldUniqueKey($storage_definition, $schema, TRUE);

As to 4, I have added a variable definition so we don't trigger a notice whenever the assertion fails.

Had left an error in #178.

This is ready for further review now :)

- I would have assigned FALSE to $constraint_triggered, so that is a boolean and only a boolean, but I'm fine with this patch.
- 2 and 3 were indeed the same, my fault.

RTBC for me, but what about the 'D8 upgrade path' tag, does this patch need a hook_update?

Well assertTrue will never consider NULL a test pass, but indeed FALSE would have been more readable. If this goes back to "Needs work" at some point, I'll try to remember to fix.

As this adds a field on the File entity and changes a few unique keys, indeed this will need an update hook at some point. But as we don't yet support beta to beta upgrades, that's out of scope for this issue :)

Just 2 nits

1. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -22,7 +22,7 @@
   - *     "storage_schema" = "Drupal\block_content\BlockContentStorageSchema",
   + *     "storage_schema" = "Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema",
   

   this is a default, just remove the line

2. +++ b/core/modules/file/src/Entity/File.php
   @@ -194,7 +195,10 @@ public static function preCreate(EntityStorageInterface $storage, array &$values
   +    // Save the hash of the URI so we can enforce uniqueness.
   +    $this->get('uri_hash')->value = Crypt::hashBase64($uri);
   
   @@ -252,6 +256,15 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      // As this is a base64-encoded SHA-256 hash, we can limit the length to
   +      // 100 characters.
   +      ->setSetting('max_length', 100)
   

   why 100? sessions.sid uses 128 but hash('sha256', $data) is 256 bits so char(64)

As to 2, it's a base64'd hash which adds another ~33% to to the 64 characters, so I just rounded up to 100.

strlen(hash('sha256', 'hello')) == 64;
strlen(base64_encode(hash('sha256', 'hello'))) == 88;
strlen(base64_encode(hash('sha256', 'hello', TRUE))) == 44;

But I just noticed hashBase64 uses raw binary data, so actually ~44 chars may have been enough in this case. Let's set it to 128 though just to be consistent with what we use for sessions.sid and as I think this discussion is out of scope for this patch.

Our max_lengths make little sense in general, usually we set it to 255 or 128 for no reason and we define our fixed-length columns as varchar, so maybe let's discuss that, along with limiting both columns to ~44 characters in a separate followup issue?

Created a d7 backport issue. https://www.drupal.org/node/2488180

With the points raise in #177 and #184 solved or explained, this is OK now.

Patch no longer applies, so needs a reroll. In that case: can you also cover my 1st point from #182?

re-rolled

Thanks @fietserwin for the review. Patch is green again.

As per #190, as there are no real changes in this patch compared to #186.

I've given this a once over and I think it's OK.

Slightly concerned about the filename base64 encoding for uniqueness, but don't have a better idea (i.e. I think that's probably better than shortening the column). Want to think about that a little bit more before committing though.

Thanks. What specifically is a worry about the hashing? Having a column on the database that really doesn't "mean" anything?

Just to clarify for anyone reading this, the hash is a base64-encoded sha256 hash -- not just base64-encoded.