Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#76 | interdiff-203955-75-76.txt | 1.67 KB | InternetDevels |
#76 | db_create-203955-76.patch | 4.61 KB | InternetDevels |
#75 | db_create-203955-75.patch | 4.11 KB | pplantinga |
#72 | db_create-203955-72.patch | 4.06 KB | pplantinga |
#70 | db_create-203955-70.patch | 4.06 KB | Pancho |
Comments
Comment #1
andrewsl CreditAttribution: andrewsl commentedNew patch contais code improvements and support for postgresql database type.
Comment #2
PanchoAs 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.
Comment #3
chx CreditAttribution: chx commentedThis is not too feasible. It's unlikely you have the privileges to do it.
Comment #4
Jeff Veit CreditAttribution: Jeff Veit commentedI 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.
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #6
amonteroHi 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.
Comment #7
David_Rothstein CreditAttribution: David_Rothstein commentedSince 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.
Comment #8
amonteroHi 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.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #10
amonteroHi 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.
Comment #11
amonteroD7 contrib port now has a Drush makefile in case you want to check it out.
Comment #12
webchickThe 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
Comment #13
chx CreditAttribution: chx commentedCan'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.
Comment #14
webchickWell 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?
Comment #15
xjmMight be good to explain the logic and assumptions before the try/catches. Also, should we be using any more specific exception than Exception?
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
Comment #16
Crell CreditAttribution: Crell commentedI'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.
Comment #17
sun@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. ;)
Comment #18
chx CreditAttribution: chx commentedDraft 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.
Comment #19
webchickOk, 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! :)
Comment #20
webchickOops. 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).
Comment #21
chx CreditAttribution: chx commentedWhy 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.
Comment #22
Crell CreditAttribution: Crell commentedIf 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.
This should be a class constant, \mysql\Connection::DATABASE_NOT_FOUND. That lets it autoload as part of the class.
db_set_active() shouldn't be necessary; and if it is, use the direct methods instead. Never use the wrapper functions inside OO code.
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!
Comment #23
webchickOk, let's see if I can get any further on this tonight. :) The patch somehow miraculously still applies. Yay!
Comment #24
webchickOk, here's an updated patch. I believe this addresses all feedback so far, except for:
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:
Comment #25
jibranPatch works fine. Created database automatically.
Comment #26
xjmThanks @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:
This method appears to be missing a docblock. I think it just needs to be "Overrides \Drupal\Core\Database\Connection::createDatabase()."?
Is there a reason not to use
Database::getConnection()->escapeTable($table);
?Edit: and there is, above:
Can we
add an inline commentadd to the inline to that effect? Edit: Explicitly mentioning whyescapeTable()
is not used.Looks like the docblock for this additionally needs an
@throws
.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?
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.)
Comment #27
xjmUnassigning to give novices a chance at this reroll. :)
Comment #28
cafuego CreditAttribution: cafuego commentedFixing the issue title.
Comment #29
webchickSome 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.
Comment #30
iflista CreditAttribution: iflista commentedCleaned up patch due to @xjm comment.
Comment #31
kbasarab CreditAttribution: kbasarab commentedDid a manual test of patch in #30.
When installing I get the following notice:
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.
Comment #32
pdrake CreditAttribution: pdrake commentedThis 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.
Comment #33
pdrake CreditAttribution: pdrake commentedThis 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.
Comment #34
Crell CreditAttribution: Crell commentedConnectionDatabaseNotFoundException seems an odd exception name. Shouldn't it be just DatabaseNotFoundException?
Otherwise I think this is good to go.
Comment #35
xjmThanks @iflista, @kbasarab, and @pdrake!
Few more points for cleanup in addition to @Crell's point from #34:
This needs the
@return
type (string, right?)This needs to have the name of the exception class after the @throws.
These three docblocks should start with verbs. Reference: http://drupal.org/node/1354#functions
The first line here is 81 chars and so needs to wrap a word earlier.
I'd change this to "The name of the database to create."
This should be "Contains" now rather than "Definition of" and end in a period.
So, let's roll a new patch that renames
ConnectionDatabaseNotFoundException
toDatabaseNotFoundException
and cleans up these points.Comment #36
pdrake CreditAttribution: pdrake commentedOk, 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).
Comment #38
pdrake CreditAttribution: pdrake commented#36: drupal-new-database-on-install-203955-36.patch queued for re-testing.
Comment #39
webchickOMG PDRAKE YOU ARE AWESOME!!! :D :D
Comment #40
pdrake CreditAttribution: pdrake commented@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.
Comment #41
pdrake CreditAttribution: pdrake commentedSomehow I missed a use statement. Here's an updated patch and an interdiff.
Comment #42
cirage CreditAttribution: cirage commentedWe 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.
Comment #43
cirage CreditAttribution: cirage commentedPatch #41 works as advertised (tested with MySQL, Progres, SQlite)
Comment #44
Crell CreditAttribution: Crell commentedcirage: 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.)
Comment #45
cirage CreditAttribution: cirage commentedIf 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:
Comment #46
pdrake CreditAttribution: pdrake commentedThe 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.
Comment #48
pdrake CreditAttribution: pdrake commentedHere's a screenshot of the modified error message.
Comment #49
pdrake CreditAttribution: pdrake commented#46: drupal-new-database-on-install-203955-46.patch queued for re-testing.
Comment #50
Crell CreditAttribution: Crell commentedThe Crell approves. Thanks!
Comment #51
amonteroDoes webchick's comment on backporting still stand for this feature as it is now?
Comment #52
catchI'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.
Comment #53
jibranCreated change notice http://drupal.org/node/1845692.
Comment #54
ParisLiakos CreditAttribution: ParisLiakos commentedTextfield'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?
Comment #55
webchickIT 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.
Comment #56
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedTonight, 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.
2. Modified the text of INSTALL.txt to the following:
Comment #57
adammalonePatch works for me!
Comment #58
webchickGreat, thanks! :D
Committed and pushed to 8.x.
Crossing my fingers and moving to backport. ;)
Comment #59
amonteroFirst 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):
as opposed to 8.x:
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 ;)
Comment #60
amonteroUnassigning.
Comment #61
dawehnerOne follow up for that: #1924278: Fatal error: Class 'Drupal\Core\Database\DatabaseNotFoundException' not found
Comment #62
amontero#59: drupal--203955--create-database-at-installation-time-D7-59.patch queued for re-testing.
Comment #63
dawehnerEven this is indeed a really nice feature, but it's a new feature so should this really be backported?
Comment #64
amonteroI +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:
and since it's at install time I see it as bearable, given the drop in the Drupal Entry Barrier™ it encompasses.
Thoughts?
Comment #65
ParisLiakos CreditAttribution: ParisLiakos commentedComment #66
dawehnerJust 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.
Comment #67
cweagansIMO, 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.
Comment #68
adammaloneFirstly, I'm not so sure we're finished with this on D8.
'database' => 'test-database'
will be inserted into settings.phpBecause 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 #69
PanchoCan confirm the issue reported by typhonius.
Also, here's another followup for PostgreSQL: #2010368: Installer can't create new database on PostgreSQL.
Comment #70
Pancho#68:
Why should that be?
Of course we can set an error before database gets created, see patch.
Comment #71
PanchoWhat 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?
Comment #72
pplantinga CreditAttribution: pplantinga commentedI 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:
Comment #73
jibran72: db_create-203955-72.patch queued for re-testing.
Comment #75
pplantinga CreditAttribution: pplantinga commentedre-roll
Comment #76
InternetDevels CreditAttribution: InternetDevels commentedI found few issues with installation on PostgreSQL after applying patch.
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 :)
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.
Comment #77
tim.plunkettThis was committed in November 2012(!) and should have been left for back port.
Please open a new issue for the follow-up.
Comment #78
tstoecklerI 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.
Comment #79
tim.plunkettWell "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.
Comment #80
catchComment #81
sunHm. 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.
Comment #82
webchickWell, 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.
Comment #83
vulcaneanu71 CreditAttribution: vulcaneanu71 commented19: drupal-new-database-on-install.patch queued for re-testing.
Comment #85
amonteroComment #86
cilefen CreditAttribution: cilefen commentedHere 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.
Comment #89
Panchoupdating related MySQL issue