default.settings.php database instructions need to be user friendly

Good idea! If you'd like to make a patch that improves the documentation, that would be welcome. Thanks!

Also, please read the issue tag guidelines below the Issue tags field.

I created first dirty patch, not sure that's done in right direction. Btw just to do up in this discussion.
I thought about instructions and not sure how it should look. Do we need very detailed instructions (for mysql, for pgsql etc.) or surface with links to documentations). What level of user who may want to configure database access manually? Etc...

That's a decent start... do you think people will find it though, given that all the other database settings are above? Also it will need some wording/puncutation attention.

According to me, i am unable to apply this patch from the drupal root directory.
Here is the output of the git apply command.

davinder@davinder-VirtualBox:/var/www/html/d8-core$ git apply -v 2536682-4.patch
Checking patch default/default.settings.php...
error: default/default.settings.php: No such file or directory
davinder@davinder-VirtualBox:/var/www/html/d8-core$

Please review attached one.

1. +++ b/sites/default/default.settings.php
   @@ -219,6 +219,23 @@
   + * You can uncomment the following lines to configure the mysql database connection
   + * for a single database configuration
   

   This exceeds 80 columns and there is no period at the end of the sentence.

2. +++ b/sites/default/default.settings.php
   @@ -219,6 +219,23 @@
   + * $databases['default']['default'] = array (
   + *   'database' => '',
   + *   'username' => '',
   + *   'password' => '',
   + *  'prefix' => '',
   + *  'host' => 'localhost',
   

   There is an indentation problem here. It should be two spaces after the mandatory space that is after the "*". Also array ( should not have a space between "array" and "(".

+++ b/sites/default/default.settings.php
@@ -219,6 +219,23 @@
+/** Default database settings
+ *
+ * You can uncomment the following lines to configure the mysql database connection
+ * for a single database configuration

"Default database settings" is on the wrong line and it should be followed by a colon.

@cilefen,

I have done changes , according to the comment #9.

forgot to add the interdiff between #8 and #11. Please find the attachment.

+++ b/sites/default/default.settings.php
@@ -220,6 +220,24 @@
+ * You can uncomment the following lines to configure the mysql database

When you refer to the product by name it should be "MySQL".

Perhaps try: "You can uncomment the following lines to configure a single MySQL database connection."

updated

@chegor Please see the API documentation and comment standards.

Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over...

@cilefen,

I have done the changes.

Thanks for the patches! Sorry for the delay in reviewing -- I've been on vacation and no one else, unfortunately, ever reviews docs patches. :(

I'm sorry, but adding a new separate doc block below the existing database instructions doc block, which only gives some suggested defaults, does not satisfy the goals of this issue. Please read the issue summary again and make a patch that addresses the concerns there. And make the changes in the *existing* doc block for the database settings, not in a separate doc block. There is actually already an example in there anyway...

Here is my attempt at an update. I took the tack of breaking it up into two sections "Database settings" and "Customizing database settings" so that the "what I have to know to get started" is condensed to a single, easily digestible chunk and the myriad of options are described as a separate section.

The primary reason for this is the $databases = array(); declaration which shows at the end of the docblock. You must add the connection code after that declaration, so uncommenting the section of code in the example (which I have seen done) rather than redeclaring it, is overwritten by the array declaration. To have it "within sight" of the example makes it much easier to see where it should be placed.

Changing username, password to sqlusername and sqlpassword.

Thanks for the patches!

In the future, when you create a new patch in an issue that already had a patch, make an interdiff file so we can see what was changed from the previous patch.

Anyway... reviewing the patch in #24... I think the reorganization from #23 is excellent, and the change in #24 to the user name and password is fine.

Actually, the whole thing looks very good. A few small suggestions:

1. +++ b/sites/default/default.settings.php
   @@ -64,20 +64,28 @@
   + * One example of the simplest connection array is shown below. To use the
   + * sample settings, copy the code below between the @code and @endcode lines
   + * and paste it after the $databases declaration. Remember to replace the
   + * values with the credentials for your database system. The documentation
   + * below describes how to customize the array for more specific needs.
   

   This is very clear, thanks!

   I'm wondering if we should add something about "be sure to remove the * characters from the beginning of the lines" though? Because if you just copy/paste you'll get invalid PHP syntax.

2. +++ b/sites/default/default.settings.php
   @@ -64,20 +64,28 @@
   - *   'collation' => 'utf8mb4_general_ci',
   

   Hm. The collation line was removed and not replace. Maybe we should put that back in?

3. +++ b/sites/default/default.settings.php
   @@ -64,20 +64,28 @@
   + *   'host' => 'sqlhost',
   + *   'port' => '',
   + *   'driver' => 'mysql',
   

   I think we should leave this with host being 'localhost' and port being 3306, which is the normal configuration?

4. Please review the original issue summary again. I'm not sure if all the points have been addressed.

Thanks for the feedback, Jennifer! I had originally made an interdiff, but didn't attach it because my patch had nothing to do with the original patch since it was deemed to be completely the wrong tack. Now I know to attach it anyway.

1. I changed "copy the code below" to "copy and uncomment the code below" which hopefully can help with that confusion as well.
2. My thought with the collation and port was due to the fact that the UI and drush site-install process write out the following $databases if there is none present and the settings.php file is writable:
   

   $databases = array (
     'default' =>
     array (
       'default' =>
       array (
         'database' => 'drupal7',
         'username' => 'root',
         'password' => '<redact>',
         'host' => 'localhost',
         'port' => '',
         'driver' => 'mysql',
         'prefix' => '',
       ),
     ),
   );
   

   which is probably a separate issue that if we really do want to encourage and "force" the collation that the installer should include it when writing the array into settings.php, but I'll file that as a separate issue.

3. I added the text:
   

   The sample provides the most common host "localhost" and port "3306" those should be changed if your provider supplies you with different values.

   and added the values back in.

4. Turns out that DatabaseConnection_mysql() is no longer a function. We removed it moving to PSR-0 in #1321540: Convert DBTNG to namespaces; separate Drupal bits. So I am not sure what the current proper syntax is to refer to the method, but took a stab with mysql::Connection::_construct. I also cleaned up a bit of the text because the pieces that were mentioned in the original summary were actually dealing with the section above it that wasn't mentioned and it wasn't clear to me either that the "defaults" were in reference to the sample above it. I added a @see link to the issue that originally introduced this option and the warnings.

Thanks for the new patch!

1. +++ b/sites/default/default.settings.php
   @@ -64,20 +64,31 @@
   + * sample settings, copy and uncomment the code below between the @code and
   

   OK, this is better.

   Do you think we need to explain how to "uncomment" the code (remove the * characters) or is this sufficient? Hm... probably OK as it is.

   However.. I do not think we can put @code and @endcode into text like this... the docs parser will see them as actual @code and @endcode tags! Oops.

   Well actually the docs parser isn't going to really work on this file anyway... so I guess we can ignore that.

2. +++ b/sites/default/default.settings.php
   @@ -64,20 +64,31 @@
   + * needs. The sample provides the most common host "localhost" and port "3306"
   + * those should be changed if your provider supplies you with different values.
   

   We need a ; before "those" in this sentence.

3. +++ b/sites/default/default.settings.php
   @@ -186,28 +184,24 @@
   + * @see mysql::Connection::__construct
   + * @see pgsql::Connection::__construct
   + * @see sqlite::Connection::__construct
   

   Hm, not sure what this :: notation is... You need to provide the full namespace of the classes. Something like:

   @see \Drupal\Something\Something\Something\Connection::__construct()

   Note also that method names in docs should end in ()

   Also all the @see lines should be together, and should be at the end of the doc block. If they don't logically belong at the end of the doc block, then use the word "See" in a paragraph (or a list) instead of the "@see" tag (@see makes a separate See Also section in docs).

I think going to the extent of describing how to uncomment would be as Moshe put it "pure nanny talk". ;-)

For the @code/@endcode, there are quite a number of other instances of those in this file, so if we do want the file to be parsed, we should probably separate that out into a separate issue.

The problem with the @see being not at the end of the block could possibly be that we have about 4 "subsections" combined into one now. We split the "basic config" to its own block, but have a massive block of "Customizing database settings" which contains information on at least the following:

• target databases
• prefixes
• initial commands
• samples for postgres and sqlite

Do you think we should break each of those out to make them more contained and consumable as individual items? They all deal with the database settings, but they can all be used independently. I could see it being beneficial either way.

I did undo the @see references there since they are not currently at the end of the docblock and wouldn't make sense to be at the end of this massive block, but only if we had a section dedicated to "initial commands".

**Edits I have made to this comment are just grammatical annoyances. Nothing substantial**

And just a quick update because I realized that while __construct() methods have two underscores, my patch only had one.

Thanks! This is looking very good! Just a few minor things to address...

1. +++ b/sites/default/default.settings.php
   @@ -64,20 +64,31 @@
   + * Customizing database settings:
   

   I don't think we should start this with a line ending in a : -- normally end lines in . unless they are just before a bullet list or a code sample or something like that. See below...

2. +++ b/sites/default/default.settings.php
   @@ -64,20 +64,31 @@
     * The "driver" property indicates what Drupal database driver the
     * connection should use.  This is usually the same as the name of the
   @@ -118,19 +129,6 @@
   

   So... Given that we've moved the sample database code into a completely different doc block, the sections here about the "driver" property etc. are kind of missing context.

   Maybe we should change the first line to say something like

   Customizations of the simple database configuration above.

   which would clue people in that things like "driver" and "prefix" shown in this doc block come from the block above.

   Thoughts?

3. +++ b/sites/default/default.settings.php
   @@ -186,28 +184,25 @@
   + * https://www.drupal.org/node/1309278 for more information on these defaults
   

   This appears to be a URL of an issue.

   Normally we would not want to link to an issue. We would want to link to documentation -- either:
   - a Book page on drupal.org
   - A change record page on drupal.org
   - A class, function, topic, etc. on api.drupal.org

   Issues are not generally easy to read as documentation.

4. +++ b/sites/default/default.settings.php
   @@ -186,28 +184,25 @@
   + * More details can be found in the constructor methods for each driver:
   + *     Drupal\Core\Database\Driver\mysql::__construct()
   + *     Drupal\Core\Database\Driver\pgsql::__construct()
   + *     Drupal\Core\Database\Driver\sqlite::__construct()
   

   Formatting: This should probably be formatted as a list -- see
   https://www.drupal.org/node/1354#lists

   If not a list, then put commas after each one and no indent

   And in any case the namespaces need to start with \

   Actually though... I do not see that these classes exist in Drupal 8. I think that you've omitted the class names for the methods?

5. +++ b/sites/default/default.settings.php
   @@ -186,28 +184,25 @@
   + * SQLite (sqlite):
     *
   - * Database configuration format:
     * @code
   

   Get rid of the empty line between the : and the @code

   But... for sqlite I don't think this will work. I think you would need to tell Drupal where the SQLite data file would be located?

Okay, so I think we're getting there!

1. Done.
2. Added a small sentence to the front end of the Customizing section to give context.
3. I changed the URL to the documentation that seems to lead to the same conclusions.
4. Fixed up the list and added the Connection class back in.
5. I moved the pgsql and sqlite examples to separate @code blocks as they really are meant to be either/or.

I wasn't sure of what the issue was with the sqlite example. There is a path which for the sqlite driver is the "database" array element. Do we need to supply a legitimate looking path since we use "localhost" in other places? It seems okay to me, but as someone who understands what it is, I may not be the best to imagine how it would best be helpful to a newcomer.

#32 was failed, Again a new Patch applied. Please review.

Thanks, @r_sharma08, sorry for the noise, I rebased 8.0.x, but not in my branch before making the diff.

@eporama we all do that(i'm sure i'll do it again). Could you check @r_sharma08's patch to ensure all your changes from #32 and any extras items are correct?

If anybody on this issue isn't using dreditor, please consider adding it to your browser, it makes contribution much nicer and is how @jhodgdon does the snippets in #31

Thanks everyone -- I think this is all looking great! Just one very very minor thing to fix:

+++ b/sites/default/default.settings.php
@@ -186,38 +187,36 @@
+ * More details can be found in the constructor methods for each driver:
+ *   - \ Drupal\Core\Database\Driver\mysql::__construct()
+ *   - \ Drupal\Core\Database\Driver\pgsql::__construct()
+ *   - \ Drupal\Core\Database\Driver\sqlite::__construct()

When making a list, line up the - bullets with the text above. These are indented 2 spaces extra.

Thanks jhodgdon , the changes applied. Please review.

1. +++ b/sites/default/default.settings.php
   @@ -64,20 +64,34 @@
   + * of the values can be customized for the particular databse system.
   

   "database" is misspelled.

2. +++ b/sites/default/default.settings.php
   @@ -186,38 +187,36 @@
   + * - \ Drupal\Core\Database\Driver\mysql::__construct()
   + * - \ Drupal\Core\Database\Driver\pgsql::__construct()
   + * - \ Drupal\Core\Database\Driver\sqlite::__construct()
   

   There's an extra space between first space and Drupal. And the Connection class is missing.

There's also a blank line before the "@code" at line 175, that seems incongruent with the other samples of code, so I have removed that.

The full sentence for "And Sample Database configuration format for SQLite (sqlite)" is a good addition, but shouldn't have "And" and needs to have the colon as per the pgsql code.

+++ b/sites/default/default.settings.php
@@ -91,7 +91,7 @@
+ * of the values can be customized for you particular database system.

Nice review thanks for this.

+++ b/sites/default/default.settings.php
@@ -64,20 +64,34 @@
+ * of the values can be customized for you particular database system.

and "you particular" should be "your particular".

New patch attached and quick interdiff from 39.

Thanks for the patches and reviews! This is definitely looking better, but as this file is fairly prominent, I think we should strive for it to be a little bit better in a couple of spots:

1. +++ b/sites/default/default.settings.php
   @@ -64,20 +64,34 @@
   + * @endcode lines and paste it after the $databases declaration. Remember to
   + * replace the values with the credentials for your database system. The
   

   I don't think we should say "Remember to..." here.

   Maybe something like:

   You will need to replace the database username and password with the credentials for your database system, and you may also need to change the port and host.

   Thoughts?

2. +++ b/sites/default/default.settings.php
   @@ -64,20 +64,34 @@
   + * those should be changed if your provider supplies you with different values.
   

   Also... are we using "provider" elsewhere in this file to refer to web hosting? It seems a bit vague. Maybe if we change the text to something like what I suggested above this problem would vanish?

3. +++ b/sites/default/default.settings.php
   @@ -64,20 +64,34 @@
   + * Given the simple database configuration example in the section above, many
   + * of the values can be customized for your particular database system.
   

   This wording seems a bit strange. "Given ... ... can be customized" ? Kind of doesn't make sense to me.

   How about something like:

   You can customize many database settings; refer to the sample in the section above as a starting point.

Here's an attempt with those changes. At least the spirit of them. I agree that #1 and #2 can be handled by one change. And actually expanded #3 to be two separate sentences.

Having no profit for assigning to me, everyone is doing patching without assigning issue to himself.
This has been recorded many times.

@eporama By assigning issues to yourself will prevent from duplication of effort. Please check Assigning ownership

Yes, well, sorry if that caused any confusion. However, if you do read the "Assigning ownership" page it specifically says that for Drupal core, you are discouraged from assigning the issue yourself:

Contributors to Drupal Core are discouraged from using the "assigned to" field unless they have a very good reason to use it. In most cases it is more appropriate to create a comment indicating that you are working on the issue and specifying which part of the issue you are working on.

I had thought that as the person making most of the changes and patches at the time that I was "indicating" that I wished to continue working on it. I assumed that cleaning up minor changes is typically the province of the person who put forth that patch unless someone else chimes in or a significant amount of time has passed. Certainly didn't mean to be snatching it away from anyone. All patches are welcome, I'm sure.

@eporama: Where is this "assigning ownership" page? I think it is wrong and would like to fix it (after discussing with other core maintianers). We actually *do* want people to assign issues to themselves if they are working on them, as far as I know. Thanks!

@jhodgdon, https://www.drupal.org/node/2172049 is what I was pointed to.

For now, I'll leave it unassigned. If it comes back as needing work, I'd be glad to grab it. If anyone else has ideas on different phrasing or a different way, please feel free to take it.

Thanks for the link! I'll discuss this with other maintainers and get that page fixed up.

Anyway, the latest patch looks good to me. Thanks everyone!

1. +++ b/sites/default/default.settings.php
   @@ -186,38 +188,36 @@
   + * https://www.drupal.org/node/310071 for more information on these defaults
   

   Let's prettify https://www.drupal.org/node/310071 to something like https://www.drupal.org/developing/api/database/configuration

2. +++ b/sites/default/default.settings.php
   @@ -186,38 +188,36 @@
   - * @see DatabaseConnection_mysql::__construct
   - * @see DatabaseConnection_pgsql::__construct
   - * @see DatabaseConnection_sqlite::__construct
   + * More details can be found in the constructor methods for each driver:
   + * - \Drupal\Core\Database\Driver\mysql\Connection::__construct()
   + * - \Drupal\Core\Database\Driver\pgsql\Connection::__construct()
   + * - \Drupal\Core\Database\Driver\sqlite\Connection::__construct()
   

   Don't we need the @see for api.drupal.org to link correctly - the fully qualified names are a good fix.

@alexpott:
1. https://www.drupal.org/developing/api/database/configuration is not a correct link. Can we write here as "see database configuration at https://www.drupal.org/developing/api/database"?
2. What do you mean be fully qualified names?

Regarding #51, item 2 -- no. api.drupal.org doesn't really handle this file well anyway, since it has multiple /** */ doc blocks that don't document "things" it recognizes (files, classes, functions, etc.). Fully qualified names are actually better I think.

Regarding item 1, I have made a page alias. So https://www.drupal.org/developing/api/database/configuration now works. Please update the patch to use this line in place of https://www.drupal.org/node/310071

And I think that is the only change needed in this patch. Thanks!

Thanks jhodgdon, the changes applied to the patch.
Please review.

The interdiff in #54 is making more than the changes that I said in #53 needed to be done. PLEASE read reviews and follow instructions. Thanks!

Hows it ?

I have reviewed changes it seems fine.

Thank you. Yes, this is I think what we want.

+++ b/sites/default/default.settings.php
@@ -186,38 +188,36 @@
- * @see DatabaseConnection_mysql::__construct
- * @see DatabaseConnection_pgsql::__construct
- * @see DatabaseConnection_sqlite::__construct
+ * More details can be found in the constructor methods for each driver:
+ * - \Drupal\Core\Database\Driver\mysql\Connection::__construct()
+ * - \Drupal\Core\Database\Driver\pgsql\Connection::__construct()
+ * - \Drupal\Core\Database\Driver\sqlite\Connection::__construct()

See note above in #53 as to why I think this list is actually much better than the original @see lines here. Besides which, @see needs to be at the end of doc blocks, not in a list in the middle like this... if you want to have things in context in the middle of a doc block, you cannot use @see (which is for making a separate See Also section).

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!