Settings.php $databases example improvement

I think the generated code overcomplicates the issue, since all the parenthesis and added syntax reduces readability.
It would make better sense to let the implementation follow the example.

I aggree with #2 but the output is generated in includes/install.inc:584 drupal_rewrite_settings() using the PHP function var_export(). The output of this function can't be formated differently.

The Drupal function drupal_var_export() returns similar output.

Without writing a custom var_export function we can only rewrite the documentation according to the output of var_export() to make it consistent. But that would dramatically reduce the readability of the documentation.

/**
 * Database settings:
 *
 * The $databases array specifies the database connection or
 * connections that Drupal may use.  Drupal is able to connect
 * to multiple databases, including multiple types of databases,
 * during the same request.
 *
 * Each database connection is specified as an array of settings,
 * similar to the following:
 * @code
 * array(
 *   'driver' => 'mysql',
 *   'database' => 'databasename',
 *   'username' => 'username',
 *   'password' => 'password',
 *   'host' => 'localhost',
 *   'port' => 3306,
 *   'prefix' => 'myprefix_',
 *   'collation' => 'utf8_general_ci',
 * );
 * @endcode
 *
 * The "driver" property indicates what Drupal database driver the
 * connection should use.  This is usually the same as the name of the
 * database type, such as mysql or sqlite, but not always.  The other
 * properties will vary depending on the driver.  For SQLite, you must
 * specify a database file name in a directory that is writable by the
 * webserver.  For most other drivers, you must specify a
 * username, password, host, and database name.
 *
 * Some database engines support transactions.  In order to enable
 * transaction support for a given database, set the 'transactions' key
 * to TRUE.  To disable it, set it to FALSE.  Note that the default value
 * varies by driver.  For MySQL, the default is FALSE since MyISAM tables
 * do not support transactions.
 *
 * For each database, you may optionally specify multiple "target" databases.
 * A target database allows Drupal to try to send certain queries to a
 * different database if it can but fall back to the default connection if not.
 * That is useful for master/slave replication, as Drupal may try to connect
 * to a slave server when appropriate and if one is not available will simply
 * fall back to the single master server.
 *
 * The general format for the $databases array is as follows:
 * @code
 * $databases['default']['default'] = $info_array;
 * $databases['default']['slave'][] = $info_array;
 * $databases['default']['slave'][] = $info_array;
 * $databases['extra']['default'] = $info_array;
 * @endcode
 *
 * In the above example, $info_array is an array of settings described above.
 * The first line sets a "default" database that has one master database
 * (the second level default).  The second and third lines create an array
 * of potential slave databases.  Drupal will select one at random for a given
 * request as needed.  The fourth line creates a new database with a name of
 * "extra".
 *
 * For a single database configuration, the following is sufficient:
 * @code
 * $databases['default']['default'] = array(
 *   'driver' => 'mysql',
 *   'database' => 'databasename',
 *   'username' => 'username',
 *   'password' => 'password',
 *   'host' => 'localhost',
 *   'prefix' => 'main_',
 *   'collation' => 'utf8_general_ci',
 * );
 * @endcode
 *
 * You can optionally set prefixes for some or all database table names
 * by using the 'prefix' setting. If a prefix is specified, the table
 * name will be prepended with its value. Be sure to use valid database
 * characters only, usually alphanumeric and underscore. If no prefixes
 * are desired, leave it as an empty string ''.
 *
 * To have all database names prefixed, set 'prefix' as a string:
 * @code
 *   'prefix' => 'main_',
 * @endcode
 * To provide prefixes for specific tables, set 'prefix' as an array.
 * The array's keys are the table names and the values are the prefixes.
 * The 'default' element is mandatory and holds the prefix for any tables
 * not specified elsewhere in the array. Example:
 * @code
 *   'prefix' => array(
 *     'default'   => 'main_',
 *     'users'     => 'shared_',
 *     'sessions'  => 'shared_',
 *     'role'      => 'shared_',
 *     'authmap'   => 'shared_',
 *   ),
 * @endcode
 * You can also use a reference to a schema/database as a prefix. This maybe
 * useful if your Drupal installation exists in a schema that is not the default
 * or you want to access several databases from the same code base at the same
 * time.
 * Example:
 * @code
 *   'prefix' => array(
 *     'default'   => 'main.',
 *     'users'     => 'shared.',
 *     'sessions'  => 'shared.',
 *     'role'      => 'shared.',
 *     'authmap'   => 'shared.',
 *   );
 * @endcode
 * NOTE: MySQL and SQLite's definition of a schema is a database.
 *
 * Database configuration format:
 * @code
 *   $databases['default']['default'] = array(
 *     'driver' => 'mysql',
 *     'database' => 'databasename',
 *     'username' => 'username',
 *     'password' => 'password',
 *     'host' => 'localhost',
 *     'prefix' => '',
 *   );
 *   $databases['default']['default'] = array(
 *     'driver' => 'pgsql',
 *     'database' => 'databasename',
 *     'username' => 'username',
 *     'password' => 'password',
 *     'host' => 'localhost',
 *     'prefix' => '',
 *   );
 *   $databases['default']['default'] = array(
 *     'driver' => 'sqlite',
 *     'database' => '/path/to/databasefilename',
 *   );
 * @endcode
 */

Attached patch rewrites output for $databases setting in includes/install.inc:584 drupal_rewrite_settings().

Added a comment.

And removed a trailing space.

Valid patch that fixes the issue.

Thanks for your work on this patch. Couple questions/remarks:

+++ b/includes/install.incundefined
@@ -623,7 +623,18 @@ function drupal_rewrite_settings($settings = array(), $prefix = '') {
+                $buffer .= '$' . "databases['$db_first_key']['$db_second_key'] = " . var_export($values, TRUE) . ";" . (!empty($setting['comment']) ? ' // ' . $setting['comment'] . "\n" : "\n");
...
+            $buffer .= '$' . $variable[1] . " = " . var_export($setting['value'], TRUE) . ";" . (!empty($setting['comment']) ? ' // ' . $setting['comment'] . "\n" : "\n");

These are sort of hard to read... I'd suggest using a full if/else rather than the ternary, since the lines are long.

Also, is it possible to add an automated test for this behavior? Not sure if we have test coverage for the creation of settings.php or not.

Finally, note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades).

It would also help to have somewhat more detailed reviews (rather than simply "fixes the issue."). E.g., some before/after, what you did to test it, etc.

These are sort of hard to read... I'd suggest using a full if/else rather than the ternary, since the lines are long.

I agree, but basically the second one is just re-adding the existing code, removed before:

-          $buffer .= '$' . $variable[1] . " = " . var_export($setting['value'], TRUE) . ";" . (!empty($setting['comment']) ? ' // ' . $setting['comment'] . "\n" : "\n");

Maybe we don't even need the comments following the $databases array?

+          if ($variable[1] == 'databases') {
+            foreach($setting['value'] as $db_first_key => $values) {
+              foreach($values as $db_second_key => $values) {
+                $buffer .= '$' . "databases['$db_first_key']['$db_second_key'] = " . var_export($values, TRUE) . ";\n");
+              }
+            }
+          }

Creating tests for this is over my head, I guess. I'd need some help with that.

I'll be happy to reroll the patch, but setting this to "needs review" to get some opinions first.

I updated the patch so that it will apply against the current state of Drupal.

While I was doing that I also decided to brake out the two big long lines into full if/else statements. After doing that I realized that I had never seen the output from $setting['comment'], so I did a quick grep to find where else it is used and found that $setting['comment'] is not used in core and it has no purpose, hence I completely removed the part where it adds $setting['comment'] to $buffer.

I would also have written a test to test against this functionality, however I can't figure out how one would actually run the test if this code didn't work in the first place.

I just want to see how the automated test site handles this. Don't use this on a real install.

Ok, it appears that qa.drupal.org already tests to make sure that Drupal can install. Therefore, writing a test against this will duplicate existing functionality(I think?).

Thanks @hosef (and thanks also for fixing that ternary). The patch looks clean to me codewise.

@hosef and I discussed this in IRC (regarding what the bot actually tests, etc.) Since we can't test the installer at present, let's do some manual testing for this patch (maybe with each database type?). Include some before/after code snippets of the settings.php from your testing in your review.

Leaving this tagged as "novice" for that task.

In the course of trying to figure out what i was supposed to assert in a test, I installed D8 ~30 times, so I already have the code snippets. But, I still need someone else to test it before it should be marked RTBC.

Here is the $databases variable before applying the patch on each of my VM's and after applying the patch on each of my VM's.

MySQL before patch:

$databases = array (
  'default' => 
  array (
    'default' => 
    array (
      'database' => 'testdatabase',
      'username' => 'testuser',
      'password' => 'T3st1ngP4ssw0rd',
      'host' => 'localhost',
      'port' => '',
      'driver' => 'mysql',
      'prefix' => '',
    ),
  ),
);

MySQL after patch:

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

SQLite before patch:

$databases = array (
  'default' => 
  array (
    'default' => 
    array (
      'database' => 'sites/default/files/.ht4.sqlite',
      'driver' => 'sqlite',
      'prefix' => '',
    ),
  ),
);

SQLite after patch:

$databases['default']['default'] = array (
  'database' => 'sites/default/files/.ht5.sqlite',
  'driver' => 'sqlite',
  'prefix' => '',
);

PostgreSQL before patch:

$databases = array (
  'default' => 
  array (
    'default' => 
    array (
      'database' => 'testdatabase',
      'username' => 'testuser',
      'password' => 'T3st1ngP4ssw0rd',
      'host' => 'localhost',
      'port' => '',
      'driver' => 'pgsql',
      'prefix' => '',
    ),
  ),
);

PostgreSQL after patch:

$databases['default']['default'] = array (
  'database' => 'testdatabase',
  'username' => 'testuser',
  'password' => 'T3st1ngP4ssw0rd',
  'host' => 'localhost',
  'port' => '',
  'driver' => 'pgsql',
  'prefix' => '',
);

Excellent, thanks @hosef! That will make it much easier for reviewers to know what to check for.

I'll test this in a bit

Confirmed, the patch does as hosef said. I installed a fresh version of D8 with the patch in, and the settings.php file did include all of the changes.

Thanks for the positive review Zgear. There are a few things I would like to point out though.

First, it appears from your comment that you only tested my patch on one of the three supported databases. When there is a patch that could potentially modify how database stuff works, one is supposed to test it on at least MySQL, PostgreSQL, and SQLite. If you have access to another database such as Microsoft SQL server or Oracle server, then it would also be nice for the community to know how well something works with them and where we could make improvements.

Second, the 'Assigned:' drop down box is supposed to be the person who is fixing the problem not the person reviewing the fix. So, in this case even though you were helping by reviewing the patch it was still assigned to me. Assigning an issue to yourself is kind of like saying 'I have an interest in the issue and want to write a patch that fixes it, and/or re-roll existing patches until the patch is committed to core or I un-assign it from myself.'

Assigning to self for manual review

If I'm reading the comment thread correctly, this issue requires before/after patch testing against the three supported database schemata. I had taken on this task thinking it was simply a documentation review; regrettably I only have MySQL available right now, so I can't actually test the patch in the remaining databases.

That said, being new to Drupal but familiar with PHP, great job on getting documentation and implementation to match.

De-assigning; kindly forgive if this comment is superfluous!

reroll

MySQL

<?php
$databases['default']['default'] = array (
  'database' => 'drupal8',
  'username' => 'drupal',
  'password' => 'drupal',
  'host' => 'localhost',
  'port' => '',
  'driver' => 'mysql',
  'prefix' => '',
);
?>

SQLite

<?php
$databases['default']['default'] = array (
  'database' => 'sites/default/files/.ht.sqlite',
  'driver' => 'sqlite',
  'prefix' => '',
);
?>

Note on SQLite, the settings.php file was written correctly, however; I did run into some weird errors about watchdog table not existing. I think this is unrelated though, because it gave the same errors when I tried installing pre-patch.

And finally postgresql:

<?php
$databases['default']['default'] = array (
  'database' => 'drupal',
  'username' => 'drupal',
  'password' => 'drupal',
  'host' => 'localhost',
  'port' => '',
  'driver' => 'pgsql',
  'prefix' => '',
);
?>

Alright, I just tested the rerolled patch, and everything worked. Marking RTBC. I can backport this if we decide that would be good idea.

+          foreach($setting['value'] as $db_first_key => $values) {
+            foreach($values as $db_second_key => $values) {

Can we find better variable names than $db_first_key and $db_second_key? There's also code style issues here i.e. missing a space after the foreach. The array formatting does look friendlier though.

reroll and rename db_first_key to db_name and db_second_key to db_role.

Fixing foreach code style issues.

Patch changed file permissions from 644 to 755. Patch rerolled and resubmitted in following commit.

Adjust patch for correct file permissions

#31: drupal-698236-improve_database_settings_documentation-31.patch queued for re-testing.

This has always confused me, happy to find this issue!

+++ b/core/includes/install.incundefined
@@ -214,7 +214,18 @@ function drupal_rewrite_settings($settings = array()) {
+        // Add special handling for the $databases setting to improve readability and to
+        // match the format of the database settings documentation in settings.php.

This inline comment should wrap at 80 characters per http://drupal.org/node/1354#drupal.

#31: drupal-698236-improve_database_settings_documentation-31.patch queued for re-testing.

#12: improve-databases-settings-documentation-698236-5.patch queued for re-testing.

This is a cute patch :)

We also need to kill about 90% of the unnecessary text of default.settings.php, but that's another issue.

+          foreach ($setting['value'] as $db_name => $values) {
+            foreach ($values as $db_role => $values) {
+              $buffer .= '$' . "databases['$db_name']['$db_role'] = " . var_export($values, TRUE) . ";";
+            }
+          }

There are a couple of issues in there:

• We should use drupal_var_export() instead of var_export() here (and in the other branch below) to match our coding standards (especially the spacing after array);
• The variable needs to be cleared first ($databases = array();), before outputting the sub-keys;
• The keys ($db_name and $db_roles) should be output using drupal_var_export() too, as in:
  

  $buffer .= '$databases[' . drupal_var_export($db_name) . '][' . drupal_var_export($db_role) . '] = ' . drupal_var_export($values, TRUE) . ';';
  

I applied patch at #31 and ran install first with mysql. The relevant part of settings.php is as follows:

$databases['default']['default'] = array (
  'database' => 'drupal8',
  'username' => 'drupal8',
  'password' => 'drupal8',
  'host' => 'localhost',
  'port' => '',
  'namespace' => 'Drupal\\Core\\Database\\Driver\\mysql',
  'driver' => 'mysql',
  'prefix' => '',
);

Also ran the install with sqlite which gave the following:

$databases['default']['default'] = array (
  'database' => 'sites/default/files/.ht.sqlite',
  'namespace' => 'Drupal\\Core\\Database\\Driver\\sqlite',
  'driver' => 'sqlite',
  'prefix' => '',
);

It seems we now have a value 'namespace' showing up in the settings that isn't in the example.

Added code to patch #31 so that the 'namespace' values exist in the example code as they do in the settings. Works for mysql, sqlite and postgres. Postgres settings output below:

$databases['default']['default'] = array (
  'database' => 'postgres',
  'username' => 'postgres',
  'password' => 'postgres',
  'host' => 'localhost',
  'port' => '',
  'namespace' => 'Drupal\\Core\\Database\\Driver\\pgsql',
  'driver' => 'pgsql',
  'prefix' => '',
);

#38: improve_database_settings_documentation-698236-38.patch queued for re-testing.

rerolled patch from #38 against 711dd53

#41: improve_database_settings_documentation-698236-41.patch queued for re-testing.

Thanks @sanguis! I diffed the two patches, I don't think the reroll was quite right. Were there many conflicts while rebasing?

there was one but I thought I sorted it. will retry

drupal_rewrite_settings() has been completely rewritten.

In the corresponding issue #1921818: Modify drupal_rewrite_settings() to allow writing $settings values heyrocker also suggests rewriting the dumps of whole arrays in favor of a simplified output as proposed here.

Here's an updated patch.

drupal_var_export() as suggested in #37 is not available during install.

remove whitespace.

#49: 698236-49.patch queued for re-testing.

#49: 698236-49.patch queued for re-testing.

Applied patch #49 and tested with MySQL. It worked.

$databases['default']['default'] = array (
  'database' => 'database_name',
  'username' => 'user',
  'password' => 'pass',
  'host' => '127.0.0.1',
  'port' => '',
  'namespace' => 'Drupal\\Core\\Database\\Driver\\mysql',
  'driver' => 'mysql',
  'prefix' => '',
);

without apply any patch the file all the changes are reflected in setting.php file in drupal-8.x

I tested this issue on both drupal-8.x and drupal-7.x. Here are my findings:
drupal-7.x

$databases = array (
  'default' => 
  array (
    'default' => 
    array (
      'database' => 'database_name',
      'username' => 'user',
      'password' => 'pass',
      'host' => 'localhost',
      'port' => '3306',
      'driver' => 'mysql',
      'prefix' => 'drupal_',
    ),
  ),
);

drupal-8.x

$databases['default']['default'] = array (
  'database' => 'database_name',
  'username' => 'user',
  'password' => 'pass',
  'host' => 'localhost',
  'port' => '3306',
  'namespace' => 'Drupal\\Core\\Database\\Driver\\mysql',
  'driver' => 'mysql',
  'prefix' => 'drupal_',
);

drupal-8x branch is working as expected but this will need to be resolved on the drupal-7x branch

I'm currently working on a back port

The patch has been added to make the database string in an easily read format.

I have tested this locally and it works as expected.

57: 698236-57.diff queued for re-testing.

Unassigning..

Hello,

I'm working with a drupal site on mysql database but i would like to use my other database oracle to display data on my site. But when i configure a new database in settings.php, i have an error : "PDOException: could not find driver in eval() (line 4 of E:\drupal\modules\php\php.module(80) : eval()'d code)."

I have to declare my driver in "Database configuration format" of settings.php too or juste add a new database?

If you have some example how to use a second database (oracle) could you say me?

Thanks

There seem to be a few things going on at once here.

Fixes need to be done in 8.x first and then backported to 7.x.
This issue was moved to 8.x in comment #1,
and then moved back to 7.x in #55....
BUT it was never committed to 8.x.

Let's get this ready for 8.x and *then* do the backport.

----
Note on #57, the patch there is not following the coding standards.
https://drupal.org/coding-standards

For example,

+++ b/includes/install.inc
@@ -629,8 +629,25 @@ function drupal_rewrite_settings($settings = array(), $prefix = '') {
+          if ($variable[1] == 'databases')
+          {
+            foreach ($setting['value'] as $db_name => $db_roles)
+            {

The { should not be on it's own line.

@steeloctopus what editor are you using? there maybe be a drupal coding standards file you can import, or you might have to configure your editor settings manually.

-----

Now, back to 8.x.

#54 was a manual test of the last 8.x patch from #49.

A review of #49:

1.

+++ b/sites/default/default.settings.php
@@ -64,12 +64,12 @@
  * array(
- *   'driver' => 'mysql',
  *   'database' => 'databasename',
  *   'username' => 'username',
  *   'password' => 'password',
  *   'host' => 'localhost',
  *   'port' => 3306,
+ *   'driver' => 'mysql',

@@ -116,11 +116,11 @@
  * $databases['default']['default'] = array(
- *   'driver' => 'mysql',
  *   'database' => 'databasename',
  *   'username' => 'username',
  *   'password' => 'password',
  *   'host' => 'localhost',
+ *   'driver' => 'mysql',
  *   'prefix' => 'main_',

there are several reorderings in arrays in this patch, why?

Is this because when the settings are saved, that is the place the driver is? And so this is making the order in the comments match what might be saved?

--
2.
@catch's concern in #27 was addressed. good.
--
3.
in #36, @Damien Tournoud had concerns:

There are a couple of issues in there:

• We should use drupal_var_export() instead of var_export() here (and in the other branch below) to match our coding standards (especially the spacing after array);
• The variable needs to be cleared first ($databases = array();), before outputting the sub-keys;
• The keys ($db_name and $db_roles) should be output using drupal_var_export() too, as in:
  

  $buffer .= '$databases[' . drupal_var_export($db_name) . '][' . drupal_var_export($db_role) . '] = ' . drupal_var_export($values, TRUE) . ';';
  

I think this is still needs work to address that.

#63 is a good point. but I think that would be a separate issue. I searched and didn't find an existing issue to use yml for settings. So someone could create that issue and link it back to here.

adding #2232301: D8: settings.php to settings.yml as related.

@#64
I rearranged the database settings example to match what Drupal actually output, because they were quite different from each other. I thought that would make it less confusing for a novice coder who needed to change the settings manually.

I spent a while looking at this and I'm not sure what need to be changed anymore.

The majority of the code in #49 is not actually run at this time because the $prefix value when _drupal_rewrite_settings_dump_one() is run is $databases, not $databases = . Also, even if I change it to $databases, that code is not run because _drupal_rewrite_settings_dump_one() is only run on objects, I believe, and that is an array.

Without any patches, though, the database structure in 8.x core is what the documentation shows:

$databases['default']['default'] = array(
  'database' => 'd8test',
  'username' => 'root',
  'password' => 'root',
  'prefix' => '',
  'host' => 'localhost',
  'port' => '3306',
  'namespace' => 'Drupal\Core\Database\Driver\mysql',
  'driver' => 'mysql',
);

I'm not sure what the issue is, except for maybe a separate issue that this is written at the bottom of the settings.php file and not replacing $databases = array(); towards the top of the file. Also, I think the point of settings $databases = array(); right above this is a good idea if it is at the bottom because then you don't have things less clearly overwritten.

And finally, the API Docs for drupal_var_export() says that this function is deprecated and should be changed to Variable::export(). I think that means if we do use that that the statement use Drupal\Component\Utility\Variable; should be added to the top of the file to make sure the dependency of the Variable class is loaded.

1. 'driver' is legacy information, still actively used, duplicates 'namespace'.
2. 'driver' + 'namespace' should always come first; they declare which database driver is used.
3. The remaining options have a natural order, too: (3) host, (4) port, (5) database, (6) username, (7) password.

doing https://www.drupal.org/contributor-tasks/update-allowed-beta
to evaluate https://www.drupal.org/contributor-tasks/update-allowed-beta

I'm not sure that @sun's feedback is actionable here.
This is just about making the comment match what the setting get's saved as.

If the suggestion is to change the order of the saved things, I think that is a different issue.

--
but does need work to reroll/remake to apply to head since it does not apply anymore.

This has changed quite a bit since the earlier patches.

Here's an update based on the difference between default.settings.php and what's been written in my settings.php by the installer.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.