Improve comment in default.settings.php

I suppose we could have a follow-up that both the old and proposed "Typically used to disable caching, JavaScript/CSS compression, re-routing of outgoing emails..." is not an English sentence, because it lacks a subject.

Tagging this issue for new contributors to work on during the Friday DrupalCon Global sprint.

I am working on it, as part of Global2020

Here is updated patch.

@bhushan.nagaonkar If you look at the test results for the first patch, it is evident there is another file you must edit.

git grep 'to override variables on secondary'
core/assets/scaffold/files/default.settings.php: * Use settings.local.php to override variables on secondary (staging,
sites/default/default.settings.php: * Use settings.local.php to override variables on secondary (staging,

Thanks @cilefen

Updated my patch.

Going to review this patch shortly!

I have reviewed and tested the patch on 9.1-x-dev. Patch applied cleanly and the two files showed the updated comments without issue.

I'm updating the status to "Reviewed & tested by the community" and welcome any additional feedback or comments. Thank you!

I have also reviewed and tested the patch on 9.1-x-dev. Patch applied cleanly and the two files showed the updated comments without issue.

Re-tested patch from #10 and passed: https://www.drupal.org/pift-ci-job/1762586

Not sure why the automated test failed, it doesn't seem related to this change?

Seems to be working for me.

+++ b/core/assets/scaffold/files/default.settings.php
@@ -754,7 +754,7 @@
  * development, etc) installations of this site. Typically used to disable
  * caching, JavaScript/CSS compression, re-routing of outgoing emails, and
- * other things that should not happen on development and testing sites.
+ * other things that should not happen on production.

I think this comment is confusing. I think we're in double negative territory. I.e use to disable caching... and other things that should not happen on production?!!? - Caching definitely should happen on production :)

I think the original is "correct" but I think this can be clearer. Not sure how atm.

* Use settings.local.php to override variables on secondary (staging,
* development, etc) installations of this site. Typically used to disable
* caching, JavaScript/CSS compression, re-routing of outgoing emails, and
* other things that should happen on development and testing sites.

Or

* Use settings.local.php to override variables on secondary (staging,
* development, etc) installations of this site. 
* Typically used on development on testing sites to disable caching and 
* JavaScript/CSS compression, set re-routing of outgoing emails, and 
* other things.

Open to other suggestions too!

I think making a list would be good because the problem is the work disable and what it is applying to.

So something like:

 * Use settings.local.php to override variables on secondary (staging,
 * development, etc) installations of this site. Typically used to:
 * - Disable caching.
 * - Disable JavaScript/CSS compression.
 * - Reroute outgoing emails.

Another problem with original test is that we say "Typically used to..." and the follow this up with the hand-wavy "and other things that should not happen on development and testing sites" - there's nothing typical about that part.

Oh yeah, that is easier to read -- clear, concise and to the point w/o making assumptions. I can get a patch up or someone else can if they want. :D

I like the idea! There is just one adjustment I would prefer. "Typically used to..." is not the beginning of an English sentence.

@cliefen How about making it

 * Use settings.local.php to override variables on secondary (staging,
 * development, etc) installations of this site. They are typically used to:
 * - Disable caching.
 * - Disable JavaScript/CSS compression.
 * - Reroute outgoing emails.

settings.local.php is a singular thing ... It is typically...

This file is often used to: or This file is typically used to:
Is there any issue with being specific about the subject e.g. file?

There is ambiguity about the subject of the second sentence. May I suggest "Create a settings.local.php file to override variables on secondary (staging, development, etc) installations of this site. Typical uses of settings.local.php include: ..."?

Thank you cilefen! That flows much better, I'm not great with proper sentence structure and grammar. :)

I've rolled up a new patch with your suggestion. Hopefully I did this correctly!

In order to complete the sentence we need the gerund form of each verb in the bullet points, which means "Disable caching" should be "Disabling caching", and so on.

Thank you, I've updated the comments in a new patch.

Assigning to myself and will be updating very shortly.

@brittany.huntzberry changes looks good to me as per latest patch #32. I would suggest it is good to go for RTBC.

Thank you for reviewing tripurari! I'll set this as RTBC for now, but anyone can provide feedback or additional testing still.

@brittany.huntzberry I know how frustrating it is for someone to say

I would suggest it is good to go for RTBC.

But effectively #37 is rtbc'ing your own patch. It's important for community processes that we minimise rtbc-ing of our own patches. Maybe @tripurari can follow up their comments with a real rtbc?

@tripurari please do not post screenshots of patches. Posting screenshots of your codebase or CLI does not advance the issue, since the automated testing infrastructure tells us whether the patch applies correctly. So, I've removed the issue credit for that screenshot. In the future, you can get credit for issues by reading the issue to understand its purpose, and posting your review or testing of that purpose. On a patch like this which is quite small a credit worthy comment is one that results in a change to the patch. For example see #31. Thank you!

@alexpott ++ Thank you, I appreciate your guidance since I'm still very new to contributing to Drupal and open source in general! Do you have any resources or references I could use for determining when moving to RTBC is appropriate? I know it is case-by-case basis, so it's probably difficult to determine without a lot of experience. Is there a certain number of people needed for review -- or any standards needed to be met (more specific to documentation)?

* development, etc) installations of this site.

"etc" is an abbreviation and should have a trailing period.

* development, etc) installations of this site.
 * Typical uses of settings.local.php include:

"Typical uses ..." is starting a new paragraph, so there should be a blank line before.

Otherwise, the change looks fine.

@brittany.huntzberry I'm not sure if there are specific docs that would help you in determining RTBC status, but basically, don't RTBC if you have submitted any patches. You are considered a contributor to the actual fix. The reviewer needs to be an independent person. There doesn't need to be a certain number of people reviewing. One can do it. They are just checking that the fix is correct. If they feel it is and there is no further work needed, it gets set to RTBC. For some special cases, core committers may request more than one person review an issue. Like for a large or complex change that they want help double-checking.

Please Ignore the comments by mistake comment was added.

The patch provided in #32 is great! But I agree with the changes suggested in #40, so included an updated patch with an intediff. Kindly review the same.

Thank You.

The new wording is clearer to me, so this looks good to go.

Committed and pushed 6cac5d7f21 to 9.1.x and 92da9f80d8 to 9.0.x and e43f7306ee to 8.9.x. Thanks!

Backported the docs fix to 8.9.x