I was asked to make changes in installer to create database in installation time. It is convinient for developers, who deploy sites on local computers. Usually they open phpMyAdmin to create database. Please, look at the attached patch.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andrewsl’s picture

FileSize
10.68 KB

New patch contais code improvements and support for postgresql database type.

Pancho’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs review » Needs work

As this is a feature request plus a pretty big one, I bump it to D7.

Also, a more generic db creation (not only mysql/i and pgsql) would be necessary. Not sure how much more of a db abstraction layer we'll have in D7 though.
Also, admins might want more control on the configuration, we'd need to elaborate on those details a little further.

Otherwise this seems to be a good thing, but we need to do a lot of testing to ensure it properly works in different scenarios.

chx’s picture

Status: Needs work » Closed (won't fix)

This is not too feasible. It's unlikely you have the privileges to do it.

Jeff Veit’s picture

I think this depends really: for proper installs, this is exactly right - your database user won't/shouldn't have the ability to create a database.

But it ignores an important part of the process of choosing Drupal: at some point someone tries Drupal for the first time. That person will probably do it in a protected local environment. And they will use the easiest/most powerful options so that they don't have installation problems; they'll install using the database root user. (Have you never done this? I've just done this as part of an D7 RC1 test.)

I think the person who takes this step is a key in increasing Drupal adoption. I can't say that this is critical to D7, because it's not necessary to attempt to create the database, so I'm not going to re-open, but I think that making this initial test of Drupal as easy as it possibly can be lowers the barrier to entry, which can only be good for Drupal adoption.

I do think that this needs to be accompanied by a warning when the db user has CREATE rights.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Closed (won't fix) » Needs work

I think it's true that there are cases where this makes sense, and the installer could attempt to do it (but only under the right set of conditions, and only if accompanied by a warning).

Definitely Drupal 8 material, though. It is way too late for this in Drupal 7 core. I think someone could theoretically write an install profile that used hook_install_tasks() or hook_install_tasks_alter() to accomplish this in Drupal 7 contrib, though.

amontero’s picture

Hi David.

I'm working in a custom 7.x install profile and I've borrowed andrewsl's code. Currently I'm working on it and trying to fit it.
If 8.x patch makes it into core, would this be a valid backport to 7.x? I would find it useful to my install profile, and I think it will be a worthwile feature for more people to backport and contribute, too. I would try to do it.
Thus, we can "kill two birds with one shot" :)
What do you think?

Regards.

David_Rothstein’s picture

Since this is a major change in functionality which also would likely require major changes to the user interface, I don't think it has much chance of being backported to Drupal 7.

However, it's great that you're trying to get it working in an install profile for Drupal 7! I would say that if there's a reason that turns out to be impossible (e.g., some aspect of the installer isn't alterable enough to do it entirely via a profile) then it might be possible to backport some small change to Drupal 7 core which would enable install profiles to alter the behavior to do something like this. However, I think there's a good chance it's already possible to do this entirely in a contrib install profile already... It would be great to hear how it turns out for you.

amontero’s picture

Hi David,

Thanks for your comments. I was afraid of this not being a backport candidate, but as you suggested I thought to give it a try as a contrib. Finally I've got some time and I've put together a D7 install profile based on andrewsl's code and seems to work (thanks Andrew!).
I've also used a trick you suggested in #1153646: Installation profile should be able to use hook_form_alter() on install_settings_form(). I think it can be sort of useful with a Drush makefile.

I have a side question: when creating a sandbox project, should I choose module or distribution? I see no 'install profile' and I think IPs now are named distributions, but I'm not sure. Are the two terms used for the same?
Thanks.

David_Rothstein’s picture

I think it's the same thing. "Install profile" is really more accurate (since a distribution contains an entire Drupal codebase also), but the project creation page might not be making that distinction.

amontero’s picture

Hi David.

I have created a sandbox project at http://drupal.org/sandbox/amontero/1535766.
Input will be much appreciated and so we will be able to discuss the issue for 7.x there and leave this issue for 8.x.
Feel free to be my first guest to my brand new issue queue :)
Thanks.

amontero’s picture

D7 contrib port now has a Drush makefile in case you want to check it out.

webchick’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7
FileSize
4.08 KB

The fact that the installer doesn't auto-create databases (despite my database user having proper credentials to do so) continues to slow me down whenever I reinstall from scratch, and is extremely frustrating. It'd be nice to get this issue fixed before its 5-year anniversary. :)

Here's a patch that works, for root/root, and for MySQL. It definitely still needs work, but marking needs review to see if I'm wildly off-base in this approach or not. I chose not to introduce any new UI here, but rather simply to create the database behind the scenes if it doesn't exist yet and if the user/pass given has permissions to do so. In this way, I think this change could be backportable to D7. Tentatively tagging so.

I started out by putting this logic in Tasks::validateDatabaseSettings(), since that's a one-time (ish) operation. However, you don't have a database object at that point. So instead, I added a createDatabase() method to the Database class, which toggles a new $connection_option called 'create_database'. If present, then the driver's __construct() method will create a PDO $dsn without the database, so that you can attempt to create it.

I'm sure this is a huge hack, but ... maybe someone can make it better. :D

chx’s picture

Can't we determine from the exception thrown in the mysql consructor that the database doesn't exist and if it doesn't, then reconnect w/o database, try to create and if exception then give up? Seems to me that $e->getCode() == 1049 would do it.

webchick’s picture

Well that certainly cleaned this up a lot. :) Thanks!

The only problem with this approach is that I could forsee a problem where you go from a localhost install of Drupal to a proper dev/stg/live environment version, and although you want to name your staging DB "8x_stg" you forgot to change settings.php accordingly, and it's still pointing at 8x_dev, which is where your localhost was pointing previously. All of a sudden you start getting a 8x_dev database created in your staging environment, and it telling you you suddenly have no tables, which would be a big WTF.

In other words, it'd be really nice to be able to do this only during install time, rather than every time the database connection is loaded. I looked through includes/install.core.inc and couldn't really derive an easy way to do that, though. The other options is moving it into MySQL's Tasks.php. I tried playing around with this a bit, but couldn't really figure out a way to do it without duplicating half of the MySQL Connect::_construct() function in Tasks::connect(). :\

Anyway, here's the patch I came up with based on chx's direction. Thoughts?

xjm’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
@@ -59,7 +59,32 @@ class Connection extends DatabaseConnection {
+    try {
...
+    catch (Exception $e) {
...
+        try {
...
+        catch (Exception $e) {

Might be good to explain the logic and assumptions before the try/catches. Also, should we be using any more specific exception than Exception?

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
@@ -59,7 +59,32 @@ class Connection extends DatabaseConnection {
+      // Database was not found; attempt to create it.
+      // @todo Include check here for "Drupal is currently being installed."
+      // Otherwise, this could potentially start creating rogue databases in
+      // environments with temporarily-incorrect $databases arrays.
+      if ($e->getCode() == 1049) {

I suggest defining a constant for the 1049 (see POSTGRESQL_NEXTID_LOCK for example), but at a minimum the comment should at least explain the meaning of the code.

Also, whitespace nit: Each subsequent line of the @todo should be indented two spaces. Reference: http://drupal.org/node/1354#todo

Crell’s picture

I'd much rather see this logic in the install system, not in the DB connection directly. As webchick said, there's plenty of opportunity for weirdness in the connection, and the @todo in the latest patch makes me worried that we're going to start adding Drupal-specific code to the driver constructor (which is a won't-fix in my book).

What we can do in the installer, perhaps, is something similar to what's done here; rather than connecting straight to the DB, connect to the server, check if the DB exists, try to create it if it doesn't, and then continue or error out accordingly.

sun’s picture

@Crell: Actually, I have a different use-case (tests... *wink* ;)) and I always wondered why DBTNG doesn't provide native methods for creating/dropping a database...? In other words, I don't think that this is a special case of the installer, but much rather functionality that should be supported by the database abstraction layer.

But anyway, I certainly don't want to hold up this wonderful patch on architectural perfection, so please ignore me. ;)

chx’s picture

Draft of somewhat alternative approach here http://privatepaste.com/3974fffabd . Also, if you want this to work for all db, then add to the connection class a (static?) method which verified whether the exception is db not found (ie for mysql it's return $e->getCode() == 1049) and another which creates a db actually.

webchick’s picture

Status: Needs work » Needs review
FileSize
4.4 KB

Ok, new patch thanks to lots of help from chx!

- Logic to create database if it doesn't exist is moved back to Tasks::connect() where it belongs.
- New function createDatabase() in the Connection class, which it calls.
- Only change to the driver class is if $connection_options['database'] isn't specified, it will not bother appending a database connection string to the dsn.

The one bit of ugliness is the business of escaping the database name. I had backticks there originally, which chx railed against because you could name a table "HELLO WORLD" which chx suspects would screw Drupal. You can't use $this->escapeTable() because that allows dots. We settled on $this->quote(), even though it's for field values and not database names, but have to do some substr() madness to clean it up (this is commented thoroughly).

To-do: move that constant for 1049 into a method call instead of a constant. Also get this working for other drivers. But hey, progress! :)

webchick’s picture

Oops. Also todo: the first half of xjm's review about commenting the try/catch blocks. Though I think we should make sure whatever's done there is in-line with the rest of the try/catch blocks in those files (and elsewhere).

chx’s picture

Why are you catching an exception in createDabase if you re-throw it immediately? You wanted a $this->fail IMO much like the original core/lib/Drupal/Core/Database/Install/Tasks.php connect does.

Crell’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -107,6 +109,24 @@ class Connection extends DatabaseConnection {
+    try {
+      // Create the database and set it as active.
+      $this->exec("CREATE DATABASE $database");
+      $this->exec("USE $database");
+    }
+    catch (\Exception $e) {
+      throw $e;
+    }

If you're not going to do anything with the exception, just let it bubble up uncaught. This extra try-catch does nothing but change the line that the exception is thrown from, which may (I'm not sure) confuse debugging.

Or, do something useful here, like wrapping the exception into another, DBTNG-specific exception.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php
@@ -8,6 +8,13 @@
+/**
+ * Error code for "Unknown database" error.
+ */
+const MYSQL_DATABASE_NOT_FOUND = 1049;

This should be a class constant, \mysql\Connection::DATABASE_NOT_FOUND. That lets it autoload as part of the class.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php
@@ -33,4 +40,40 @@ class Tasks extends InstallTasks {
+      // This doesn't actually test the connection.
+      db_set_active();
+      // Now actually do a check.
+      Database::getConnection();
+      $this->pass('Drupal can CONNECT to the database ok.');

db_set_active() shouldn't be necessary; and if it is, use the direct methods instead. Never use the wrapper functions inside OO code.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php
@@ -33,4 +40,40 @@ class Tasks extends InstallTasks {
+        throw($e);
+        return FALSE;

The return statement is extraneous. It will never be reached.

Also, throw is a language construct so it shouldn't have ().

I definitely prefer this version to the earlier approaches!

webchick’s picture

Assigned: Unassigned » webchick

Ok, let's see if I can get any further on this tonight. :) The patch somehow miraculously still applies. Yay!

webchick’s picture

Status: Needs work » Needs review
FileSize
96.32 KB
6.51 KB

Ok, here's an updated patch. I believe this addresses all feedback so far, except for:

db_set_active() shouldn't be necessary; and if it is, use the direct methods instead. Never use the wrapper functions inside OO code.

That is from a direct copy/paste of the code in Drupal\Core\Database\Install\Tasks::connect(), so I'd prefer not to change it here.

Here's a screenshot of the error you get if you specify a non-existant database with an unprivileged user:

 1044 Access denied for user 'peon'@'localhost' to database 'name'

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Patch works fine. Created database automatically.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Novice

Thanks @jibran!

I spoke to @webchick and it sounds like this patch still needs to support postgres and possibly sqlite.

Meanwhile, there's a few things to clean up:

  1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
    @@ -113,6 +121,24 @@ public function databaseType() {
    +  public function createDatabase($database) {
    

    This method appears to be missing a docblock. I think it just needs to be "Overrides \Drupal\Core\Database\Connection::createDatabase()."?

  2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
    @@ -113,6 +121,24 @@ public function databaseType() {
    +    $database = substr($this->quote($database), 1, -1);
    

    Is there a reason not to use Database::getConnection()->escapeTable($table);?

    Edit: and there is, above:

    You can't use $this->escapeTable() because that allows dots

    Can we add an inline comment add to the inline to that effect? Edit: Explicitly mentioning why escapeTable() is not used.

  3. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
    @@ -113,6 +121,24 @@ public function databaseType() {
    +    catch (\Exception $e) {
    +      throw new ConnectionDatabaseDoesNotExistException($e->getMessage());
    

    Looks like the docblock for this additionally needs an @throws.

  4. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.phpundefined
    @@ -33,4 +36,47 @@ public function name() {
    +          $this->fail(st('Database %database not found. Attempted to create it, and the server reports the following message: %error.', array('%database' => $database, '%error' => $e->getMessage())));
    

    I feel like this string could use a little work, maybe "The server reports the following message when attempting to create the database" for the second sentence?

  5. There's also a few minor coding standards violations in the patch (comment lines over 80 chars http://drupal.org/node/1354#general, missing verbs or incorrect verb forms http://drupal.org/node/1354#functions, the new standard for @file in class files http://drupal.org/coding-standards/docs#files, etc.)

xjm’s picture

Assigned: webchick » Unassigned

Unassigning to give novices a chance at this reroll. :)

cafuego’s picture

Title: Create database in installation time » Create database at installation time

Fixing the issue title.

webchick’s picture

Some other notes from IRC (not part of the novice re-roll, unless you're feeling particularly dashing!)


- SQLite won't really do anything to support createDatabase() because it uses files instead and is able to auto-create them already. Should just return TRUE;
- PostgreSQL is much harder, and DamZ recommends not attempting to do so in this patch. Instead, what he'd like to see is a new exception called UnsupportedDatabaseException (or whatever; just so it's consistent), and have pgsql's createDatabase() method throw it.
- Then Crell noted that in the docs for the main createDatabase() base method, mention that the method is only supported in certain DBs and will throw said exception if it is not.

Nevermind. ;) Still discussing.

iflista’s picture

Assigned: Unassigned » iflista
FileSize
3.09 KB
6.64 KB

Cleaned up patch due to @xjm comment.

kbasarab’s picture

Status: Needs review » Needs work

Did a manual test of patch in #30.

When installing I get the following notice:

Notice: Undefined variable: table in Drupal\Core\Database\Driver\mysql\Connection->createDatabase() (line 135 of core/lib/Drupal/Core/Database/Driver/mysql/Connection.php).

Screenshot: http://www.evernote.com/shard/s39/sh/dd8b7ee3-98c0-46b2-a62e-ba0b201e042...

Reversed patch and ran install again and error'ed out from not having DB created yet.

pdrake’s picture

This patch cleans up the PHP notice and creates the database in pgsql, but fails to recognize that it can connect to the pgsql database on the first try (successfully connects on the second try), so this is pretty close to functional with pgsql and works fine with mysql.

pdrake’s picture

Status: Needs work » Needs review
FileSize
12.46 KB

This appears to make pgsql database creation fully functional on my system. If available, it uses the PECL intl extension to get the proper locale. Otherwise, it falls back en_US.

Crell’s picture

ConnectionDatabaseNotFoundException seems an odd exception name. Shouldn't it be just DatabaseNotFoundException?

Otherwise I think this is good to go.

xjm’s picture

Thanks @iflista, @kbasarab, and @pdrake!

Few more points for cleanup in addition to @Crell's point from #34:

  1. +++ b/core/lib/Drupal/Core/Database/Connection.phpundefined
    @@ -741,6 +741,20 @@ public function schema() {
    +   * @return
    +   *   The sanitized database name string.
    

    This needs the @return type (string, right?)

  2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
    @@ -113,6 +121,25 @@ public function databaseType() {
    +   * @throws
    
    +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.phpundefined
    @@ -167,6 +174,24 @@ public function databaseType() {
    +   * @throws
    

    This needs to have the name of the exception class after the @throws.

  3. And a few style tweaks:
    +++ b/core/lib/Drupal/Core/Database/ConnectionDatabaseDoesNotExistException.phpundefined
    @@ -0,0 +1,14 @@
    + * Exception thrown if specified database is not found.
    
    +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.phpundefined
    @@ -33,4 +36,49 @@ public function name() {
    +   * Checking database connection.
    
    +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.phpundefined
    @@ -42,6 +44,61 @@ public function minimumVersion() {
    +   * Check if we can connect to the database.
    

    These three docblocks should start with verbs. Reference: http://drupal.org/node/1354#functions

    +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.phpundefined
    @@ -33,4 +36,49 @@ public function name() {
    +        // Database connection failed for some other reason than the database not
    +        // existing.
    
    +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.phpundefined
    @@ -42,6 +44,61 @@ public function minimumVersion() {
    +        // Database connection failed for some other reason than the database not
    +        // existing.
    

    The first line here is 81 chars and so needs to wrap a word earlier.

    +++ b/core/lib/Drupal/Core/Database/Connection.phpundefined
    @@ -1088,6 +1102,16 @@ public function supportsTransactionalDDL() {
    +   *   Desired name of database.
    

    I'd change this to "The name of the database to create."

    +++ b/core/lib/Drupal/Core/Database/ConnectionDatabaseDoesNotExistException.phpundefined
    @@ -0,0 +1,14 @@
    + * Definition of Drupal\Core\Database\ConnectionDatabaseDoesNotExistException
    

    This should be "Contains" now rather than "Definition of" and end in a period.

So, let's roll a new patch that renames ConnectionDatabaseNotFoundException to DatabaseNotFoundException and cleans up these points.

pdrake’s picture

Ok, did comment/name cleanup and this also implements createDatabase in the sqlite driver (this patch previously caused a method not found error when attempting to install with sqlite).

Status: Needs review » Needs work
Issue tags: -Novice, -Needs backport to D7

The last submitted patch, drupal-new-database-on-install-203955-36.patch, failed testing.

pdrake’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs backport to D7
webchick’s picture

OMG PDRAKE YOU ARE AWESOME!!! :D :D

pdrake’s picture

@webchick, thanks. The one failing test was apparently a testbot hiccup as all tests passed the second time around. So, I guess this just needs another review as it now includes createDatabase() functionality for all 3 core database drivers and passes all tests.

pdrake’s picture

Somehow I missed a use statement. Here's an updated patch and an interdiff.

cirage’s picture

FileSize
79.92 KB
93.02 KB

We tested patch #36 with LANG=en_US.UTF-8 with MySql, Postgres and SQlite. The Patch worked for us.

In MySQL the error message for insufficient DB creation rights is a litlle bit misleading. In Postgres this error message is more clear for the user (see attachements).

We'll also test patch #41.

cirage’s picture

Patch #41 works as advertised (tested with MySQL, Progres, SQlite)

Crell’s picture

cirage: So are you recommending RTBC for #41, or only after the error message gets tweaked? If the latter, what would you recommend? (Or just reroll with a better message.)

cirage’s picture

If it isn't possible to get the detailed reason for the 'create database' error I would recommend to add another item in the list of possible reasons why it could have failed:

  • Does the database user have sufficient privileges?
pdrake’s picture

The screenshot shown above occurs when a user without sufficient privileges to create a database is used, and the database does not exist. If a user with sufficient privileges to create the database is used, the actual creation error message is shown on failure. Given this, I have updated the error message to ask if the database exists or if the database user has sufficient privileges to create it. Interdiff included for ease of review.

Status: Needs review » Needs work

The last submitted patch, drupal-new-database-on-install-203955-46.patch, failed testing.

pdrake’s picture

Status: Needs work » Needs review
FileSize
89.58 KB

Here's a screenshot of the modified error message.

pdrake’s picture

Crell’s picture

Status: Needs review » Reviewed & tested by the community

The Crell approves. Thanks!

amontero’s picture

Does webchick's comment on backporting still stand for this feature as it is now?

catch’s picture

Title: Create database at installation time » Change notice and followup: Create database at installation time
Category: feature » task
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

I've been wondering whether this leaves uninstalled sites any more open to arbitrary code execution than it currently does, but since you can put any database you like in the form, I don't see a way that it can. Crell approves™ of the database layer changes and the installer changes are pretty self-contained. So committed/pushed to 8.x.

This needs a change notice and there's no diff to INSTALL.txt but I'm sure there ought to be given the new feature - we can handle that as a follow-up in this issue or a new one though - but please don't mark this issue fixed until that's started somewhere. Should also be changed in the install instructions on d.o somewhere I assume.

jibran’s picture

Status: Active » Needs review

Created change notice http://drupal.org/node/1845692.

ParisLiakos’s picture

Textfield's description

"The name of the database your Drupal data will be stored in. It must exist on your server before Drupal can be installed."

We should change/remove it?

webchick’s picture

Title: Change notice and followup: Create database at installation time » Followup: Create database at installation time
Priority: Critical » Normal
Status: Needs review » Active

IT GOT COMMITTED!! OMG!! YAY!!!!!! :D :D :D :D :D :D

Thank you thank you THANK YOU so much pdrake for taking this one home!!!

I adjusted the change notice slightly to add a bit more details. Looks good to me. The screenshot is a nice touch. :)

And yep, we need to change that description, + INSTALL.txt instructions. This should be fairly easy. This issue is already tagged Novice.

Shawn DeArmond’s picture

Status: Active » Needs review
FileSize
2.04 KB

Tonight, at our Sacramento Drupal Users Group meeting, we reviewed this issue as part of our Drupal Ladder learn and issue sprint.

We are proposing the following changes:

1. Simply remove the second sentence for the text field, since it is no longer completely accurate.

"The name of the database your Drupal data will be stored in."

2. Modified the text of INSTALL.txt to the following:

-   Because Drupal stores all site information in a database, you must create
-   this database in order to install Drupal, and grant Drupal certain database
-   privileges (such as the ability to create tables). For details, consult
-   INSTALL.mysql.txt, INSTALL.pgsql.txt, or INSTALL.sqlite.txt. You may also
-   need to consult your web hosting provider for instructions specific to your
-   web host.
+   Because Drupal stores all site information in a database, the Drupal
+   installer will attempt to create this database for you. If you create the
+   database manually, you must grant Drupal certain database privileges (such as
+   the ability to create tables).  For details, consult INSTALL.mysql.txt,
+   INSTALL.pgsql.txt, or INSTALL.sqlite.txt. You may also need to consult your
+   web hosting provider for instructions specific to your web host.
adammalone’s picture

Status: Needs review » Reviewed & tested by the community

Patch works for me!

webchick’s picture

Version: 8.x-dev » 7.x-dev
Category: task » feature
Status: Reviewed & tested by the community » Patch (to be ported)

Great, thanks! :D

Committed and pushed to 8.x.

Crossing my fingers and moving to backport. ;)

amontero’s picture

Status: Patch (to be ported) » Needs review
FileSize
8.2 KB

First working version for 7.x (MySQL only) for initial review.
It is a quite straightforward port. I had to tweak a line to make it work and would like some feedback.

In includes/database/mysql/install.inc (7.x):

    catch (\Exception $e) {
      // Attempt to create the database if it is not found.
      if ($e->getCode() == DatabaseConnection_mysql::DATABASE_NOT_FOUND) {

as opposed to 8.x:

      if ($e->getCode() == Connection::DATABASE_NOT_FOUND) {

I'm not very familiar with database layer, so I'm unsure if this was the correct fix. Please review before going for pgsql/sqlite. This works for me, now let's see what the testbot has to say. Crossing fingers, too ;)

amontero’s picture

Assigned: iflista » Unassigned

Unassigning.

dawehner’s picture

amontero’s picture

dawehner’s picture

Even this is indeed a really nice feature, but it's a new feature so should this really be backported?

amontero’s picture

I +1 webchicks comment in #203955-12: Create database at installation time and I think it's a very useful addition that will not affect already installed sites. It only breaks a translated string at:

-      '#description' => st('The name of the database your @drupal data will be stored in. It must exist on your server before @drupal can be installed.', array('@drupal' => drupal_install_profile_distribution_name())),
+      '#description' => st('The name of the database your @drupal data will be stored in.', array('@drupal' => drupal_install_profile_distribution_name())),

and since it's at install time I see it as bearable, given the drop in the Drupal Entry Barrier™ it encompasses.
Thoughts?

ParisLiakos’s picture

Title: Followup: Create database at installation time » Create database at installation time
dawehner’s picture

Just want to point out the problem that users of other DB drivers can't update drupal, once this is in, and the drivers are not adapted, this seems to be problematic.

cweagans’s picture

IMO, this should be backported. It doesn't affect any existing sites, and it's a good UX improvement for new sites. The only downside that I can see is the string change, but that seems relatively minor.

This kind of sounds like a David_Rothstein question.

adammalone’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

Firstly, I'm not so sure we're finished with this on D8.

  1. Try to install Drupal with a database by the name of test-database
  2. A database called testdatabase will be created
  3. 'database' => 'test-database' will be inserted into settings.php
  4. 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.

Pancho’s picture

Can confirm the issue reported by typhonius.
Also, here's another followup for PostgreSQL: #2010368: Installer can't create new database on PostgreSQL.

Pancho’s picture

Status: Needs work » Needs review
FileSize
4.06 KB

#68:

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.

Why should that be?
Of course we can set an error before database gets created, see patch.

Pancho’s picture

Category: feature » bug
Priority: Normal » Major
Issue tags: -Novice

What about the issue raised in #68?

The problem is that hyphens are allowed in the database field, but are silently stripped when it comes to creating the database. The hyphen is kept for settings.php though, so the installation fails.
While the admin should continue to be allowed to enter a hyphen as part of an existing database name, we need to throw an error message if the database doesn't exist, so the user keeps control and can choose another name for the database to be created.

#70 should do that, while certainly needing a test. Should I move it to a separate followup issue, so we can continue doing the D7 backport here?

pplantinga’s picture

FileSize
4.06 KB

I can confirm that this is an issue and that this patch fixes it, with one small tweak: call t() instead of st().

Here's the updated patch:

jibran’s picture

72: db_create-203955-72.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 72: db_create-203955-72.patch, failed testing.

pplantinga’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
4.11 KB

re-roll

InternetDevels’s picture

I found few issues with installation on PostgreSQL after applying patch.

  1. Fatal error: Call to undefined method Drupal\Core\Database\Driver\pgsql\Connection::exec() in /var/www/drupal8/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php on line 214

    Cause of this problem is that exec() method is part of PDO class but Drupal\Core\Database\Driver\pgsql\Connection doesn't extend it. I think it is just typo :)

  2. Fatal error: Call to undefined function Drupal\Core\Database\Driver\pgsql\Install\st() in /var/www/drupal8/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php on line 86

    I got this error when entered incorrect database name (i.e. drupal8-auto). I suppose it is also typo.

Here is corrected patch to fix this errors.

tim.plunkett’s picture

Version: 8.x-dev » 7.x-dev
Category: Bug report » Feature request
Priority: Major » Normal
Status: Needs review » Patch (to be ported)

This was committed in November 2012(!) and should have been left for back port.
Please open a new issue for the follow-up.

tstoeckler’s picture

Version: 7.x-dev » 8.x-dev
Category: Feature request » Bug report
Priority: Normal » Major
Status: Patch (to be ported) » Needs review

I don't see how we can commit this to D7 if we know it currently don't works in D8? I know that you'll disagree with me @tim.plunkett but I'll say it nevertheless: It's quite common in the issue queue to fix things in D8 first and then backport. Often things that are marked as backport are re-tagged for D8 when things get discovered during the backport.

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Well "Create database at installation time" was committed, I guess the issue needs to be retitled.
And it needs an issue summary, since the database is created just fine, and I have no idea what the major bug is here.

catch’s picture

Status: Needs work » Postponed (maintainer needs more info)
sun’s picture

Hm. Why this issue is even tagged for backport in the first place?

How can we change the installer in a stable release? — Invalidates all documentation and possibly even changes code paths, no?

What about custom and contributed database drivers?

IMHO, we should move the (whichever) necessary follow-up fix into a new issue and call this fixed.

webchick’s picture

Well, when I made this patch I explicitly did it so that it could be backported to 7.x. It simply creates the database if one doesn't exist and the user has permissions to do so. Otherwise, it doesn't change anything, so doesn't invalidate any existing documentation that says to create the DB first; that'll still work.

Code paths are different, it's true, but that'd be mostly up to David Rothstein to see if they're severe enough to prevent a backport. I can't remember anymore if it requires changes in DB drivers.

I do agree though with splitting the remaining problems out into sub-issue(s). Sounds like there's a problem with it not accepting "-" as part of the DB name, and possibly a problem with SQLite. PostreSQL already has its own issue. Most likely the D7 backport (if any) should be postponed until at least PostgreSQL/SQLite are working.

vulcaneanu71’s picture

The last submitted patch, 19: drupal-new-database-on-install.patch, failed testing.

amontero’s picture

cilefen’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Postponed (maintainer needs more info) » Postponed
Issue tags: -Needs issue summary update
Related issues: +#2525906: Illegal characters in database names break database auto-creation on install

Here is the needed follow-up to #68, which I verified is still a bug:

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

The backport is postponed on that getting done.

  • catch committed a5a2ce9 on 8.3.x
    Issue #203955 by pdrake, webchick, andrewsl, iflista: Create database at...
  • webchick committed 18902b2 on 8.3.x
    Issue #203955 follow-up by Shawn DeArmond: Fix installation docs and...

  • catch committed a5a2ce9 on 8.3.x
    Issue #203955 by pdrake, webchick, andrewsl, iflista: Create database at...
  • webchick committed 18902b2 on 8.3.x
    Issue #203955 follow-up by Shawn DeArmond: Fix installation docs and...
Pancho’s picture

  • catch committed a5a2ce9 on 8.4.x
    Issue #203955 by pdrake, webchick, andrewsl, iflista: Create database at...
  • webchick committed 18902b2 on 8.4.x
    Issue #203955 follow-up by Shawn DeArmond: Fix installation docs and...

  • catch committed a5a2ce9 on 8.4.x
    Issue #203955 by pdrake, webchick, andrewsl, iflista: Create database at...
  • webchick committed 18902b2 on 8.4.x
    Issue #203955 follow-up by Shawn DeArmond: Fix installation docs and...