Illegal characters in database names break database auto-creation on install [#2525906]

Follow-up to #203955: Create database at installation time

Expected Behavior

Illegal characters in database names (those not passing this regex: /[^A-Za-z0-9_.]+/) are handled appropriately for automatic database creation stage of the installer. "Appropriately" could mean different things—validation or normalization of the typed input.

Actual Behavior

If illegal characters are used in the database name, the user sees the "Drupal is already installed message", which is not true. Drupal is not installed and things are quite broken. Actually - the will have to recopy the settings files and start over. But Drupal doesn't explain what is happening.
Error now appears as (to screen or in logs depending on settings):

Uncaught exception 'Symfony\Component\DependencyInjection\Exception\InvalidArgumentException' with message 'The service definition "renderer" does not exist.' in core/vendor/symfony/dependency-injection/ContainerBuilder.php:868 
Stack trace: #0 core/vendor/symfony/dependency-injection/ContainerBuilder.php(478): Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition('renderer') 
#1 core/lib/Drupal.php(157): Symfony\Component\DependencyInjection\ContainerBuilder->get('renderer') 
#2 core/includes/install.core.inc(1152): Drupal::service('renderer') 
#3 core/includes/install.core.inc(1090): install_database_errors(Array, './sites/default...') 
#4 core/includes/install.core.inc(369): install_verify_database_settings('sites/default') 
#5 core/includes/install.core.inc(113): insta in core/vendor/symfony/dependency-injection/ContainerBuilder.php on line 868

Steps to Reproduce

As described in the original report #203955-68: Create database at installation time:

Try to install Drupal with a database by the name of test-database
A database called testdatabase will be created
'database' => 'test-database' will be inserted into settings.php
Installation will fail due to Drupal not being able to connect to the database

Because database creation occurs BEFORE install_settings_form_validate we can't just inform the user that the name is invalid, rather we have to ensure that the same escaped name is placed into settings.php. As a caveat, perhaps this should only be escaped if Drupal creates the database.

We also need to ensure that the SQLite implementation is not escaped as an escaped filepath (stripped of /) causes install errors for obvious reasons. So we cannot simply run some REGEX over $database['database'] before being written to settings.php.

Rather I have a feeling we may have to call escapeDatabase so that each of the database drivers may handle the escape in the same way they do when createDatabase is called.

Comment	File	Size	Author
#22	interdiff.txt	468 bytes	jeroent
#22	illegal_characters_in-2525906-22.patch	9.12 KB	jeroent
#14	illegal_characters_in-2525906-14.patch	9.12 KB	jeroent
#13	illegal_characters_in-2525906-13.patch	8.42 KB	jeroent
#8	illegal_characters_in-2525906-8.patch	7.31 KB	cilefen
#8	interdiff-2525906.txt	5.16 KB	cilefen
#5	non_alphanumeric-2525906-5.patch	6.93 KB	cilefen

Comments

Comment #1

cilefen commented 4 July 2015 at 20:06

Some patches were offered after comment #68 in the parent issue: #203955-68: Create database at installation time

Comment #2

cilefen commented 6 July 2015 at 00:41

Priority:	Normal	» Major
Issue summary:	View changes

I am bumping this to major because it should be. Anyone in this situation will have a broken install and won't know what to do to fix it.

Comment #3

cilefen commented 6 July 2015 at 00:42

Issue summary:

View changes

Comment #4

cilefen commented 6 July 2015 at 00:50

Title:	Hyphens in database names break database auto-creation on install	» Non-alphanumeric characters in database names break database auto-creation on install
Issue summary:	View changes

Commas also cause this.

Comment #5

cilefen commented 6 July 2015 at 02:22

Title:	Non-alphanumeric characters in database names break database auto-creation on install	» Illegal characters in database names break database auto-creation on install
Issue summary:	View changes
Status:	Active	» Needs review

Status	File	Size
new	non_alphanumeric-2525906-5.patch	6.93 KB

Comment #6

cilefen commented 6 July 2015 at 02:26

I feel it is wrong to escape the typed database name without telling the user. So #5 throws a new exception that is caught in the database install task and is sent up to the install form.

Comment #7

larowlan

🇦🇺🏝.au GMT+10

commented 6 July 2015 at 02:32

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php
@@ -83,10 +83,19 @@ protected function connect() {
+          switch (get_class($e)) {
+            case 'Drupal\Core\Database\DatabaseInvalidNameException':
+              $this->fail(t('The database name contains illegal characters'));
+              break;
+
+            case 'Drupal\Core\Database\DatabaseNotFoundException':
+              $this->fail(t('Database %database not found. The server reports the following message when attempting to create the database: %error.', array('%database' => $database, '%error' => $e->getMessage())));

This feels wrong. Why not set the message on the Exception when it is created and then use $e->getMessage()?

Comment #8

cilefen commented 6 July 2015 at 02:52

Status	File	Size
new	interdiff-2525906.txt	5.16 KB
new	illegal_characters_in-2525906-8.patch	7.31 KB

Comment #9

cilefen commented 7 July 2015 at 00:05

I feel that we should add an isValidDatabaseName() method to the Connection class to improve readability.

Comment #10

daffie commented 7 July 2015 at 09:48

Status:	Needs review	» Needs work
Issue tags:		+Needs tests

1 file was hidden/shown/deleted

Status	File	Size
hidden	non_alphanumeric-2525906-5.patch	6.93 KB

The patch looks good to me. I do have some questions:

I am missing tests to make sure that this patch fixes the problem and to make sure that the problem does not return.
In the patch there is only a change for MySQL and PostgreSQL. Does SQLite not have the problem?

Comment #11

justachris commented 13 August 2015 at 22:01

Comment #12

justachris commented 15 August 2015 at 15:29

Issue summary:

View changes

Updated Actual Behavior based on most recent HEAD

Comment #13

jeroent

Dutch

🇧🇪

commented 22 August 2015 at 16:09

Status:	Needs work	» Needs review
Issue tags:	-Needs tests

Status	File	Size
new	illegal_characters_in-2525906-13.patch	8.42 KB

Wrote my first test for Drupal. Patch attached.

Comment #14

jeroent

Dutch

🇧🇪

commented 22 August 2015 at 16:12

Status	File	Size
new	illegal_characters_in-2525906-14.patch	9.12 KB

1 file was hidden/shown/deleted

Status	File	Size
hidden	illegal_characters_in-2525906-13.patch	8.42 KB

Wrong patch. This is the right one...

Comment #15

stefan.r commented 22 August 2015 at 16:22

+++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
@@ -40,7 +40,7 @@
+        'Failed to <strong>CREATE</strong> a test table on your database server with the command %query. The server reports the following message: %error.<p>Are you sure the configured username has the necessary permissions to create tables in the database and that the database name contains no illegal characters?</p>',

I don't see how creating a table has anything to do with having an invalid database name, shouldn't this have its own test and its own error message?

+++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
@@ -311,4 +312,41 @@ public function testFilterComments($expected, $comment) {
+    } catch (DatabaseException $e) {

missing newline after the bracket

Comment #16

cilefen commented 22 August 2015 at 16:26

We need to think about #10-2, sqlite.

Comment #17

cilefen commented 22 August 2015 at 16:36

#15-1 That's not new to this patch.

Comment #18

22 August 2015 at 16:41

The last submitted patch, 13: illegal_characters_in-2525906-13.patch, failed testing.

Comment #19

22 August 2015 at 16:44

Status:

Needs review

» Needs work

The last submitted patch, 14: illegal_characters_in-2525906-14.patch, failed testing.

Comment #20

stefan.r commented 22 August 2015 at 16:52

@cilefen yes it's arguable whether it's in scope here, but what is appended to the error message is new. So if possible it would still be good if we could fix this here instead of making that error message even longer and more confusing...

Comment #21

cilefen commented 22 August 2015 at 17:09

I have not thought about this issue in a while. From what I understand from looking at the surrounding code is that a table create is the ultimate test of whether the database is *really* ready. This patch could maybe do a better job of handling the exceptions as they bubble up.

Comment #22

jeroent

Dutch

🇧🇪

commented 23 August 2015 at 15:56

Status:

Needs work

» Needs review

Status	File	Size
new	illegal_characters_in-2525906-22.patch	9.12 KB
new	interdiff.txt	468 bytes

15.2: Fixed.

Patch attached.

Comment #23

23 August 2015 at 16:21

Status:

Needs review

» Needs work

The last submitted patch, 22: illegal_characters_in-2525906-22.patch, failed testing.

Comment #24

JulienD commented 26 September 2015 at 11:20

Status:	Needs work	» Closed (duplicate)
Related issues:	-#2229793: Installation - Database name gets escaped for creation, but not for settings

Closing the issue because an older one is already existing #2443839

I have summarized patches and conversations over there

Illegal characters in database names break database auto-creation on install

Expected Behavior

Actual Behavior

Steps to Reproduce

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Comment #19

Comment #20

Comment #21

Comment #22

Comment #23

Comment #24

Parent issue

Related issues

Referenced by

News items

Our community

Documentation

Drupal code base

Governance of community