MySQL column and table comments are not truncated properly

I just tested against HEAD with MariaDB 5.5.35 and PHP 5.4.24 but the problem does not occur. D8 is the same as D7 in the schema definition for this column but yes, prepareComment() is different in D8. The D8 version uses substr to trim the comment to a maximum length but D7 uses truncate_utf8.

I performed an informal search of core for double-quotes used inside MySQL comments and found this:

core/modules/comment/comment.install:    'description' => 'Maintains statistics of entity and comments posts to show "new" and "updated" flags.',
core/modules/dblog/dblog.install:        'description' => 'Type of log message, for example "user" or "page not found."',

The definition in comment.install is the same as D7 but it is truncated wrongly for me in D8. So, I think we are looking at a regression in prepareComment().

Please test with this patch and see if it fixes this column in dblog and the table comment for comment_entity_statistics. If it does, we'll retitle this issue.

#1167144: Make cache backends responsible for their own storage changed from truncate_utf8() to substr - we probably should change to use Unicode::truncate()

Title change.

Do we need tests here?

Yes let's add tests.

Tests.

Removed a debugging line.

Can you please upload a -FAIL test to proove that the patch solves the problem?

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -492,7 +493,7 @@ public function prepareComment($comment, $length = NULL) {
-      $comment = substr($this->connection->prefixTables($comment), 0, $length);
+      $comment = Unicode::truncate($this->connection->prefixTables($comment), $length, TRUE, TRUE);

Do we really want to support unicodes here? Well, it is just on schema time, so it seems okay.

Rerolled

Just context changes in the patch, reroll looks good. And we can see the fail from the test-only patch in #17, fine that the new one doesn't apply. Thanks @marthinal!

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -492,7 +493,7 @@ public function prepareComment($comment, $length = NULL) {
-      $comment = substr($this->connection->prefixTables($comment), 0, $length);
+      $comment = Unicode::truncate($this->connection->prefixTables($comment), $length, TRUE, TRUE);;

Let's remove the double semi-colon.

Other than that looks good to me.

Thanks for reviewing @Cotsser :)

Done.

Thanks @marthinal.

Interdiff next time, please :)

Looks good and has test coverage, RTBC!

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -13,6 +13,7 @@
 use Drupal\Core\Database\SchemaObjectExistsException;

+++ b/core/modules/system/src/Tests/Database/SchemaTest.php
@@ -212,6 +213,12 @@ function tryInsert($table = 'test_table') {
+      $db_type = Database::getConnection()->databaseType();

Minor, but no need to assign this to a variable - can just use the method in the if statement directly.

Ok let's do it :)

Looks good, thanks @marthinal.

Committed 62b9d58 and pushed to 8.0.x. Thanks!