Drupal does not install when auto-creating the MySQL database with special characters

Before the database is created the name is sanitized and '-' is eleminated (only alphanumerics and underscore are permited). That is the reason why the name of the database is 'drupal8dev' and in settings.php file is 'drupal8-dev'.
To remove any ambiguity I added a validation on database name.

I'm at DrupalCon Los Angeles and am going to be testing the patch in #1 with @kevinkhuat.

It looks straight-forward as the validation step should occur alerting the user of the invalid database name before even setting files or creating the database. This would certainly prevent the next steps from occurring which is the proper workflow.

I'm at DrupalCon Los Angeles and I am testing 8.0.x-dev to ensure that the issue is still present. After testing, I can confirm that the problem still exist.

I just got done testing the patch in #1 against the latest 8.0.x-dev. It works as expected and does not create settings.php or the database. Having some lunch and then I'll review the patch.

I improved the patch a bit, by using preg_match instead of preg_preplace, as it makes it a bit more clear what we are doing (detecting illegal chars).

#3 + #4 Hope it doesn't mess up the reviews you are doing, you can see the actual change I did in the interdiff.txt

@googletorp, that doesn't mess up the reviews that we're doing. I'll just review based on the latest patch you put up (#5) and then mark it as RTBC if everything passes the code standards. The functionality in the patch needs to be tested.. Thanks for a correction on that and clearing up the functionality.

After some consulting with the code and Kristen Pol we determined that the createDatabase function in the MySQL database classes run the escapeDatabase function. That removes hyphens or anything that doesn't match /[A-Za-z0-9_.]+/.

I'm just improving the readability of the patch (and adding in a + to the regex) and then will have kevinkhaut review it after I test.

Alright. Here's my take on the patch. It allows alphanumeric, underscore, and periods per the escapeDatabase method that is used in createDatabase.

I improved the patch a bit more.

+ is not needed, since we don't care if there is more than one illegal char.

'/a-zA-Z0-9_/' is the same as '/\w/'

You are correct. I was trying use regex that matched what was done in the escapeDatabase function.

MySQL doesn't allow for periods in the database name. When I attempted to do so in the installer there was also a MySQL error. We should just remove that check altogether and simplify the preg_match to just check for '/\W/' as that is any non alphanumeric plus underscore.

The patch I included has the change suggested and I have tested it a few times.

@dasginganinja

I tested your patch and it works. I wondering if we should keep the regex expression, '/a-zA-Z0-9_/'. The escapeDatabase method has this and I'm wondering if this was fully written out to make it easier for people, who are not familiar with regular expressions, to understand.

Since using a period throws an error, I'll go ahead and update the documentation for the database name requirements.

Thanks for the patch and review.

Good catch on the period. It's interesting that the escapeDatabase code includes a period when it appears like it shouldn't. Someone will need to open that as a separate issue.

I agree with @kevinkhuat that perhaps the other regular expressions were explicitly written out similar to:

'/a-zA-Z0-9_/'

in order to be more self-documenting. For \w or \W, I would need to go look up the regular expression syntax even though I have done many regular expressions but I wouldn't have to if it was '/a-zA-Z0-9_/'. For example, I don't think it's obvious that \w includes underscore. This would also be consistent with the escapeDatabase code if it was more explicit.

But, since the comment was added then maybe that is "good enough" for understandability. I personally would opt for '/a-zA-Z0-9_/' for consistency especially since the code right above that uses the more filled out regex.

I agree with you guys as well. For readability it should be clearly spelled out. The latest patch I attached fits the bill and clears up all of these concerns.

Rerolled the patch under the latest 8.0.x-dev.

Marking as Needs Review since we've addressed the concerns.

I guess that this issue might exist in Drupal 7 too.

By writing a test that extends the InstallerTestBase we can actually test this change.

There are four issues for the same problem (#2229793, #2525906 and #2567609). I'm summarizing them here because there are interesting things in each one.

Basically, if you had a character that is different from A-Za-z0-9_. your installation will crash. The DB name in settings.php is escaped at runtime which result to the fact that the DB connection does not work.

Regarding MySQL documentation dash are allowed but database names need to be wrapped in-between quotes.

    Permitted characters in unquoted identifiers:
    ASCII: [0-9,a-z,A-Z$_]
    Extended: U+0080 .. U+FFFF

    Permitted characters in quoted identifiers:
    ASCII: U+0001 .. U+007F
    Extended: U+0080 .. U+FFFF

There are several problems we need to solve:
1) From an Ux point of view, we need add a description to the Database Name field on the form to inform the user about allowed characters (names must be strictly alphanumeric plus underscore).
2) Add a field validate to avoid the form to be submitted.
3) Write a test to ensure the form validate works
4) Updating the installer to through a message/error to the user (form validation are not enough in case Drupal is installed via Drush)
5) Write a test to ensure the install through the error

Pending questions :
- Does Postgres and SqlLite need the same update?
- How can a user install Drupal in the case of the DB name is generated by someone else and he has no way to change it (maybe on shared hosting solutions)?

Notes :
- The form construction is done by getFormOptions() in core/lib/Drupal/Core/Database/Install/Tasks.php. Drivers can implement more options.
- The escape database function escapeDatabase() is located in core/lib/Drupal/Core/Database/Connection.php

I'm leaving DC Barcelona and here is a status update.

I have to summarized all patches from other issues in on patch :

What this patch does:
1) Added a description to the form Database name field
2) Added a field validate function to generate a message to the user
3) wrote a test for point 2)
4) Added a Exception to the installer
5) wrote a test to ensure the install through the error

Credits goes to @stefanosala, @swentel, @jhedstrom, @cilefen, @JeroenT, @daffie, @dasginganinja for their work on all the patches. Sorry if I've forgot someone.

Users are now not able to create DB using non allowed symbols. This works with the UI and Drush

What's need to be done :
- Run tests for PostgresSql and SqlLite
- See if this need to be rollbacked to D7 (From what I remember D7 does not create DB)

I copied the credits request to the issue summary.

Just a followup that this is still in fact an issue. Just installed 8.0.0, and when choosing a DB that contains a hyphen, I get a WSOD, with this in the Apache log:
PHP Fatal error: Uncaught exception 'Symfony\\Component\\DependencyInjection\\Exception\\InvalidArgumentException' with message 'The service definition "renderer" does not exist.' in /var/www/drupal/drupal-8.0.0/vendor/symfony/dependency-injection/ContainerBuilder.php:796\nStack trace:\n#0 /var/www/drupal/drupal-8.0.0/vendor/symfony/dependency-injection/ContainerBuilder.php(440): Symfony\\Component\\DependencyInjection\\ContainerBuilder->getDefinition('renderer')\n#1 /var/www/drupal/drupal-8.0.0/core/lib/Drupal.php(158): Symfony\\Component\\DependencyInjection\\ContainerBuilder->get('renderer')\n#2 /var/www/drupal/drupal-8.0.0/core/includes/install.core.inc(1151): Drupal::service('renderer')\n#3 /var/www/drupal/drupal-8.0.0/core/includes/install.core.inc(1089): install_database_errors(Array, './sites/site...')\n#4 /var/www/drupal/drupal-8.0.0/core/includes/install.core.inc(366): install_verify_database_settings('sites/site...')\n#5 /var/www/drupal/drupal-8.0.0/core/includes/install.core.inc(113): install_begin_request(Object(Composer\\ in /var/www/drupal/drupal-8.0.0/vendor/symfony/dependency-injection/ContainerBuilder.php on line 796, referer: http://site.com/core/install.php

To fix, I just removed the generated code from the settings.php file and reran the installer.

Thanks,
-Tim

Thanks for the patch. Good stuff. Some minor things I noticed.

1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -211,10 +212,12 @@ public function databaseType() {
   +      throw new DatabaseInvalidNameException("$database is an invalid name.");
   

   Needs t()?

Note that when I checked core the use of t() in Exceptions is very inconsistent, e.g.

./contact/src/MailHandler.php:      throw new MailHandlerException('Unable to determine message recipient');
./contextual/src/ContextualController.php:      throw new BadRequestHttpException(t('No contextual ids specified.'));
./datetime/src/DateTimeComputed.php:      throw new \InvalidArgumentException("The definition's 'date source' key has to specify the name of the date property to be computed.");
./dblog/src/Plugin/rest/resource/DBLogResource.php:      throw new NotFoundHttpException(t('Log entry with ID @id was not found', array('@id' => $id)));
./dblog/src/Plugin/rest/resource/DBLogResource.php:    throw new HttpException(t('No log entry ID was provided'));

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -249,8 +250,9 @@ public function databaseType() {
   +      throw new DatabaseInvalidNameException("$database is an invalid name.");
   

   Needs t()?

3. +++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
   @@ -299,6 +300,11 @@ public function validateDatabaseSettings($database) {
   +      $errors[$database['driver'] . '][database'] = t('The database name you have entered, %database, is invalid. The database name can only contain alphanumeric characters or underscores.', array('%database' => $database['database']));
   

   Typo with:

   $database['driver'] . '][

   (single quote in wrong place)

4. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/mysql/Install/TasksTest.php
   @@ -0,0 +1,89 @@
   +        'mysql][prefix' => 'The database table prefix you have entered, <em class="placeholder">' . $invalid . '</em>, is invalid. The table prefix can only contain alphanumeric characters, periods, or underscores.',
   

   Needs t()?

5. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/mysql/Install/TasksTest.php
   @@ -0,0 +1,89 @@
   +        'mysql][database' => 'The database name you have entered, <em class="placeholder">' . $invalid . '</em>, is invalid. The name can only contain alphanumeric characters or underscores.',
   

   Needs t()?

6. +++ b/core/tests/Drupal/Tests/Core/Database/Stub/StubConnection.php
   @@ -57,6 +58,10 @@ public function databaseType() {
   +      throw new DatabaseInvalidNameException("$database is an invalid name.");
   

   Needs t()?

@Kristen Pol we shouldn't be using t() in exceptions - the Installer is one of the few places where we do this in HEAD but it is wrong - just wasn't fixed in time for D8.

1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -211,10 +212,12 @@ public function databaseType() {
   +    if ($database != Database::getConnection()->escapeDatabase($database)) {
   +      throw new DatabaseInvalidNameException("$database is an invalid name.");
   +    }
   
   +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -249,8 +250,9 @@ public function databaseType() {
   -    // Escape the database name.
   -    $database = Database::getConnection()->escapeDatabase($database);
   +    if ($database != Database::getConnection()->escapeDatabase($database)) {
   +      throw new DatabaseInvalidNameException("$database is an invalid name.");
   +    }
   
   +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -354,9 +355,15 @@ public function databaseType() {
   +    if ($database != Database::getConnection()->escapeDatabase($database)) {
   +      throw new DatabaseInvalidNameException("$database is an invalid name.");
   +    }
   
   +++ b/core/tests/Drupal/Tests/Core/Database/Stub/StubConnection.php
   @@ -57,6 +58,10 @@ public function databaseType() {
   +    if ($database != $this->escapeDatabase($database)) {
   +      throw new DatabaseInvalidNameException("$database is an invalid name.");
   +    }
   

   This change looks wrong because the docs for escapeDatabase say For some database drivers, it may also wrap the database name in database-specific escape characters. If it does that this can never be true.

2. +++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
   @@ -299,6 +300,11 @@ public function validateDatabaseSettings($database) {
   +    // Verify there are no illegal characters in the database name.
   +    if (!empty($database['database']) && preg_match('/[^a-zA-Z0-9_]/', $database['database'])) {
   +      $errors[$database['driver'] . '][database'] = t('The database name you have entered, %database, is invalid. The database name can only contain alphanumeric characters or underscores.', array('%database' => $database['database']));
   +    }
   

   Isn't this the only valid test - we should only be testing the user input on the db form.

This is a reroll. If I have any time today, I will look into the comments above.

It would be nice if we could use the driver's own escape function if possible instead of a strict default.

What is actually happening in head is that an invalid database name makes it to, say mysql/Connection::createDatabase() and is escaped and created, using the database's connection object escapeDatabase(). But, settings.php is written with the unescaped name. That is the real bug.

The exception for me is now:

Fatal error: Uncaught exception 'Symfony\Component\DependencyInjection\Exception\InvalidArgumentException' with message 'The service definition "renderer" does not exist.' in /Users/cjm/Sites/drupal8x/vendor/symfony/dependency-injection/ContainerBuilder.php:796 Stack trace: #0 /Users/cjm/Sites/drupal8x/vendor/symfony/dependency-injection/ContainerBuilder.php(440): Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition('renderer') #1 /Users/cjm/Sites/drupal8x/core/lib/Drupal.php(158): Symfony\Component\DependencyInjection\ContainerBuilder->get('renderer') #2 /Users/cjm/Sites/drupal8x/core/includes/install.core.inc(1151): Drupal::service('renderer') #3 /Users/cjm/Sites/drupal8x/core/includes/install.core.inc(1089): install_database_errors(Array, './sites/default...') #4 /Users/cjm/Sites/drupal8x/core/includes/install.core.inc(366): install_verify_database_settings('sites/default') #5 /Users/cjm/Sites/drupal8x/core/includes/install.core.inc(113): install_begin_request(Object(Composer\Autoload\ClassLoader), Arr in /Users/cjm/Sites/drupal8x/vendor/symfony/dependency-injection/ContainerBuilder.php on line 796

Priority changed to major, because the problem leads to failed Drupal install process with WSOD and without any glue for a solution.
For not familiar guys it will look like it's impossible to set up Drupal at all...

I agree with @alexpott in #25.
It looks like a misuse of escape method.

Validation is enough both for regular and drush installation processes.
Moreover, as it's designed to escape Database name, and it does not right now it lead to next problem.
If i insert DB name '123', it still be valid, but DB will not be created (see http://dev.mysql.com/doc/refman/5.0/en/identifiers.html, 'Identifiers may begin with a digit but unless quoted may not consist solely of digits.').
Because of this in my patch used escapeDatabase as designed — to escape name to make it possible to create the database.

And more thing, dot is also allowed (if quoted), that's why i turned it back for validation to be consistent with escaping.

Small change in phpDoc added.

There appears to be another manifestation of this same issue but it is resolved by the patch in #33.

When a DevDesktop user creates a new Drupal site a mysql user is created. That user has no password.

The database settings look similar to:

$databases = array();
$databases['default']['default'] = array(
  'driver' => 'mysql',
  'database' => 'd8',
  'username' => 'd8',
  'password' => '',
  'host' => 'localhost',
  'prefix' => '',
);

When that user is installing the site they hit the "Set up database" phase of the install and are prompted for a password. If the user fills out anything in the password field and submits, they hit the same exception: The service definition "renderer" does not exist.

I was able to confirm this same issue outside of DevDesktop but, the patch in #33 resolves the issue. Restoring the skip "Set up database" phase if the db creds are pre-fillled in settings.php. Not sure why it's fixing it, but it is.

I also used to use databases with dashes in the names and ran into this issue. Comment #19 explains that dashes are allowed by MySQL, but database names need to be wrapped in quotes in queries, hence probably the patch from #33 not supporting dashes.

I applied the patch and got a nice warning, could continue installation after changing the database name.

It would be nice if we could use the driver's own escape function if possible instead of a strict default.

Comment in #29 sounds like an elegant solution. How would that work? Should each connection extend escapeDatabase() like in patch #33? escapeDatabase() could modify the database name before writing it to settings.php.

1. The latest patch uses a hard restriction on all DB drivers. Should validation be done by the specific driver?
2. Should quotes only be added when there is a hyphen, dot, or leading #? Should the escaped value (which with the latest patch, now has single quotes) be written to settings.php?
3. If validation is going to allow dot and escaping is going to allow leading numbers (in mySQL), why not allow hyphens?

@lokapujya

About point 1, my point of view that, yes it could be done for all drivers, but it requires pretty much work on all currently implemented drivers. I.e. for MySQL, the set of rules are pretty complex (http://dev.mysql.com/doc/refman/5.0/en/identifiers.html), so validation will be a bit not trivial. In addition, it will require an implementation of separate validations per each driver (instead of the current common validation).

For point 2 — seems better to quote it always. I don't see the reason to add rules to make it conditional. There are no visible problems with always quoted DB name.

About point 3 — i also thought about this inconsistency but decided to keep it in terms of initial regExp (cut hyphens), because in the other case we will fall into all of the work in point 1 (complex separate validations per driver).

As a result, i think we need to push this problem forward and additional validations should be considered as a separate feature request (see #2644960: Specific DB name validation per DB driver). It's out of the scope of the initial defect.

I've just re-rolled patch from #33 to fix strange test-bot message about non-last patch.

Regarding the escaping mentioned in #36.2: Drupal will use the escaped name to create the database. The escaped name has single quotes around it. The database name in settings.php will not have the quotes. Will that be an issue anywhere further down the line?

So, the previous patch in #26 was handling this using exceptions in the createdatabase() function if escapeDatabase changed the user entered name. But then:

This change looks wrong because the docs for escapeDatabase say For some database drivers, it may also wrap the database name in database-specific escape characters. If it does that this can never be true.

Regarding the above comment in #25, it doesn't look like escapeDatabase() actually does wrap the database name. So, that code from the #26 path might be ok.

However, as of now, all drivers share the same escapeDatabase() method from the base class so I think it is ok to use the #40 patch. It does the validation in UI - validateDatabaseSettings(). Should get a 2nd opinion though.

A followup issue was created for specific DB validation which is more complicated. It seems like a good idea to not hold this fix up on that though.

Still needs tests.

About #26 — i totally disagree with the approach where DB name is modified somewhere.
It will lead to very not evident behaviour.
Examples of such behaviour.
1. If i install Drupal from UI, the real name of DB will be different from what i input in the form. When i going to some DB tool like phpMyadmin to check something in DB, i will not find my DB by exact name. I don't know to search for in this case.
2. Frequently a Drupal is not installed in regular way, developers just import existed DB (from other developer or environment) and manually fill settings.php file. In this case, it's also not evident which rules to new DB name. I could use any DB name while import DB (i.e. via phpMyadmin or terminal), and then i expect to fill exact DB name in settings.php

In addition, DB name escaping it's internal logic per certain driver and could be changed later, and it looks logical to incapsulate it in the core and not in the configuration/settings.
In the solution which i propose the logical flow is next:
Raw DB name in configuration (form or settings.php set up). It's always matched with real DB name. It's on a high level.
On lower level we just validate name, apply some escaping rules and etc. It's behind the scenes.

About tests — I agree.
But let's figure out what exactly we need to test.
Is it enough to test only validation step?

About #26 — i totally disagree with the approach where DB name is modified somewhere.

I don't think the intent is to modify the DB name. What it's doing is seeing if the escapeDatabase() changed the name. If it changed, then it's its rejected as an invalid name.

#43 1&2:If the database name is being escaped during createdatabase() (which would allow a DB to be created solely with digits), then does it also need to be escaped in Connection::open()? #26 patch has test for different DB names, but I can't tell if it that test passed or not because it didn't complete.

I don't think the intent is to modify the DB name. What it's doing is seeing if the escapeDatabase() changed the name. If it changed, then it's its rejected as an invalid name.

Yes, exactly.

@lokapujya

#43 1&2:If the database name is being escaped during createdatabase() (which would allow a DB to be created solely with digits), then does it also need to be escaped in Connection::open()? #26 patch has test for different DB names, but I can't tell if it that test passed or not because it didn't complete.

From what i see it's not needed in Connection::open(), because DB name here is not used directly in SQL query. It's wrapper in dsn and passed to PDO. I tested it with several invalid DB names(like only digits case) and it works.

@lokapujya + @cilefen
Sorry, I confused with another patch. About #26, I agree with @alexpott in #25.
The idea of escapeDatabase function to escape some chars. But if it escapes something it always leads to invalid DB name with this patch, why we need escape function then?

What the benefit of having extra exceptions if the validation handle all cases? And why to have only validation is not enough?
If validation is implemented properly, invalid name never will be passed to createDatabase().

If validation is implemented properly, invalid name never will be passed to createDatabase().

Because we actually are not implementing validation properly. We are being more strict than the driver (we are not allowing hyphens.) Ideally, we wouldn't have to validate; We would rely on the driver to validate (or we test to see if the CREATE worked.)

Anyways, doing our own validation (the latest patch) is better than a fatal error; It will prevent a fatal error for when someone tries to use a hyphen (which is how I came across this issue) and instead the UI will give a warning.

I agree that in ideal way validation should mirror real rules of DB name per driver, but as i mention in #38, actual rules a bit complicated.

That's why i suggest in terms of this issue provide fix of fatal error (tests still need to be done), but continue discussion about improvement of DB names support in #2644960: Specific DB name validation per DB driver

I'm on board with that approach.

Added tests(partially adopted from previous patches) for the last patch.

+++ b/core/tests/Drupal/Tests/Core/Database/Driver/mysql/Install/MysqlInstallTaskDatabaseNameTest.php
@@ -0,0 +1,105 @@
+namespace Drupal\Core\Database\Install {
+  if (!function_exists('t')) {
+    function t($string, array $args = []) {
+      return strtr($string, $args);
+    }
+  }
+}

is this needed?

I know this snippet looks strange, but there is a reason to add it.
As we testing installation phase, some parts of Drupal core is not included yet.
As a result, when we are running a test, we get a fatal error because function t() is not defined.

I took this solution from another test in the core – MachineNameTest.php or EntityViewsDataTest.php.
If someone has a better solution, I will be glad to apply it.

Basically, we don't need to use t(), and can just use sprintf() or strtr()?

Issue comes from the fact that we calling validateDatabaseSettings() function both from the test run and from regular Drupal run.
And t() is used inside of validateDatabaseSettings(). I don't think it's possible to use sprintf() instead, because translation will be missed in case of regular work.

1. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/mysql/Install/MysqlInstallTaskDatabaseNameTest.php
   @@ -0,0 +1,105 @@
   +  class MysqlInstallTaskDatabaseNameTest extends UnitTestCase {
   
   +++ b/core/tests/Drupal/Tests/Core/Database/Driver/mysql/MysqlConnectionTest.php
   @@ -0,0 +1,64 @@
   +   *   testEscapeDatabaseName. The first value is the expected value, and the second
   

   exceeds 80 characters.

2. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/mysql/MysqlConnectionTest.php
   @@ -0,0 +1,64 @@
   +   *   An indexed array of where each value is an array of arguments to pass to
   +   *   testEscapeDatabaseName. The first value is the expected value, and the second
   

   of where, or in which

3. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/mysql/MysqlConnectionTest.php
   @@ -0,0 +1,64 @@
   +      array('`123`', '123'),
   

   optional: use [] instead of array()

Thanks for notes. Points 2 and 3 corrected.
About point 1, I have doubts, because message string copied as is from the core. And from this point of view, we need to make same corrections within the core. But from what I see, there is no single standard/approach about single string split into multiple lines.
As a result, t() with string more than 80 chars is widely used in core.

Please post interdiff with new patches. It would be helpful if you can post one for the changes in #58.

In fixing #2, the length of the comment (which is what I meant in #1) is also fixed. RTBC from my point of view. We need a 3rd person to look at this.

Another possibility (just FYI, in case someone else hits this in a Google search): Your MySQL server might not be reachable. I was wondering why I kept hitting this error after trying a hundred different gyrations, and in the end, I realized my MySQL server was binding to the wrong IP address (on a separate server), so my database connection was incorrect, and drush si was showing this misleading error.

Using the web UI showed the correct error message:

Failed to connect to your database server. The server reports the following message: SQLSTATE[HY000] [2003] Can't connect to MySQL server on '[ip-here]' (111).

Anyone want to agree with #49 and RTBC this?

@lokapujya If we go with the form-based #49, we need a @todo comment pointing to #2644960: Specific DB name validation per DB driver near the new validation code.

WRONG PATCH uploaded.

Added the @todo. Removing needs tests since we have them.

IMHO, this is kind of a feature request and probably doesn't need to be backported to D7.

@lokapujya: I don't think this is a feature, since it leads to invalid states as explained in the issue body. That is, the patch does not need to be backported to Drupal 7 before it may be committed to Drupal 8. Only found two minor things:

1. +++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
   @@ -288,12 +289,18 @@ public function getFormOptions(array $database) {
   +    // @todo Implement #2644960: Specific DB name validation per DB driver.
   

   Instead of #, we usually include the full drupal.org link.

2. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/mysql/MysqlConnectionTest.php
   @@ -0,0 +1,64 @@
   +      // For now dash is also be stripped out.
   

   will also be?

The exception with the renderer service not being available was addressed in 8.1.x but its fix has not been backported. #2662552: Early use of render service in installer is problematic The current patch applies to all three branches so I've added test runs for each, as well as test runs for different DB environments (which we usually want to do for this kind of fix).

1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -218,6 +218,13 @@ public function databaseType() {
   +   * {@inheritdoc}
   +   */
   +  public function escapeDatabase($database) {
   +    return '`' . parent::escapeDatabase($database) . '`';
   +  }
   

   This seems to be a separate fix from the rest of the patch. Is it required to fx this?

2. +++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
   @@ -210,6 +210,7 @@ public function getFormOptions(array $database) {
   +      '#description' => t('The database name can only contain alphanumeric characters, underscores or dots.'),
   
   @@ -288,12 +289,18 @@ public function getFormOptions(array $database) {
   +    // @todo Implement #2644960: Specific DB name validation per DB driver.
   +    // Verify there are no illegal characters in the database name.
   +    if (!empty($database['database']) && preg_match('/[^a-zA-Z0-9_.]/', $database['database'])) {
   +      $errors[$database['driver'] . '][database'] = t('The database name you have entered, %database, is invalid. The database name can only contain alphanumeric characters, underscores or dots.', array('%database' => $database['database']));
   +    }
   

   Minor: This should have a serial comma.

3. We are adding two new test classes here, both of which are unit tests so they won't actually use any database. Can we look into whether we could extend an existing installer integration test in addition or instead? Not sure whether the DB validation can be tested in an integration test but we should check whether this already has test coverage of any sort at least.

Also tagging for subsystem maintainer review for this. I'm surprised this issue could exist in D7 and yet just surface now, so it's worth looking into more and getting a signoff on the approach. I am not sure about having different validation from the DB itself. Rather, we should make sure the DB exception if any gets reported, which is related to #2662552: Early use of render service in installer is problematic. So let's also test manually on 8.1.x and see whether this behaves acceptably there without the patch.

So do we want to backport #2662552: Early use of render service in installer is problematic to 8.0.x?

@Wim Leers I don't think so. We did not originally because it was potentially disruptive, and at this point backporting it to 8.0.x would only buy us two extra weeks of the bug being fixed since the soonest the fix would go out is April 6 and that is the final bugfix release of 8.0.x, with 8.1.0 coming April 20.

But it would be good to understand if this is a duplicate, or if some part of this issue needs to still be fixed in 8.1.x and/or 8.0.x.

When I try to install Drupal on D8.1 with a hyphen in the database name, it always answers "Drupal already installed." Well, that's better than a fatal error.

@lokapujya still a bug though, since it is not installed in fact ;)

Yeah, and it actually creates the database (without the hyphen) and then fails to install.

So this patch can solve that problem. The plan was to get this in now. Then, to allow the database do the validation in the spawned issue. Or, we could hold off for the ideal solution.

Well, I guess it CAN still go into 8.0.x also since it is not as disruptive as the other issue.

see #66

Closed #3157176: Wrong DB name on Set up database fails and leads to error but it should be handled., adding credit.

Worked on IS update.

#67
1 According to #32 this is needed to properly use the escape method.
2 comma added.
3. TODO

Rerolled the patch but the tests are failing.

Fix coding standard errors.

1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -282,6 +282,13 @@ public function databaseType() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function escapeDatabase($database) {
   +    return '`' . parent::escapeDatabase($database) . '`';
   +  }
   +
   

   I'm not sure this is correct or necessary. We've been quoting the database name since #2986452: Database reserved keywords need to be quoted as per the ANSI standard

2. +++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
   @@ -241,6 +241,7 @@ public function getFormOptions(array $database) {
   +      '#description' => t('The database name can only contain alphanumeric characters, underscores, or dots.'),
   
   @@ -322,12 +323,18 @@ public function getFormOptions(array $database) {
   +    if (!empty($database['database']) && preg_match('/[^a-zA-Z0-9_.]/', $database['database'])) {
   

   If we want to except dots then we need to change \Drupal\Core\Database\Connection::escapeDatabase(). Or allow the db driver to set the regex used it.