At the bottom of default.settings.php it says:
/**
* Load local development override configuration, if available.
*
* 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 not happen on development and testing sites.
*
* Keep this code block at the end of this file to take full effect.
*/
Should it instead say:
/**
* Load local development override configuration, if available.
*
* 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 not happen on production.
*
* Keep this code block at the end of this file to take full effect.
*/
Comment | File | Size | Author |
---|---|---|---|
#42 | interdiff_3158589_32-42.txt | 1.14 KB | ankithashetty |
#42 | 3158589-42.patch | 1.93 KB | ankithashetty |
#35 | Patch 32.png | 128.66 KB | tripurari |
#35 | Patch 29.png | 127.65 KB | tripurari |
#32 | 3158589-32.patch | 1.92 KB | brittany.huntzberry |
Comments
Comment #2
cilefen CreditAttribution: cilefen as a volunteer commentedComment #3
bhushan.nagaonkar CreditAttribution: bhushan.nagaonkar as a volunteer and at Axelerant for Drupal India Association commentedComment #4
cilefen CreditAttribution: cilefen as a volunteer commentedI 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.
Comment #6
Kristen PolTagging this issue for new contributors to work on during the Friday DrupalCon Global sprint.
Comment #7
bhushan.nagaonkar CreditAttribution: bhushan.nagaonkar as a volunteer and at Axelerant for Drupal India Association commentedI am working on it, as part of Global2020
Comment #8
bhushan.nagaonkar CreditAttribution: bhushan.nagaonkar as a volunteer and at Axelerant for Drupal India Association commentedHere is updated patch.
Comment #9
cilefen CreditAttribution: cilefen as a volunteer commented@bhushan.nagaonkar If you look at the test results for the first patch, it is evident there is another file you must edit.
Comment #10
bhushan.nagaonkar CreditAttribution: bhushan.nagaonkar as a volunteer and at Axelerant for Drupal India Association commentedThanks @cilefen
Updated my patch.
Comment #11
bhushan.nagaonkar CreditAttribution: bhushan.nagaonkar as a volunteer and at Axelerant for Drupal India Association commentedComment #12
cilefen CreditAttribution: cilefen as a volunteer commentedComment #13
bhushan.nagaonkar CreditAttribution: bhushan.nagaonkar as a volunteer and at Axelerant for Drupal India Association commentedComment #14
brittany.huntzberryGoing to review this patch shortly!
Comment #15
brittany.huntzberryI 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!
Comment #16
hedrickbt CreditAttribution: hedrickbt as a volunteer commentedI 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.
Comment #18
brittany.huntzberryRe-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?
Comment #19
binnythomas CreditAttribution: binnythomas at Axelerant for Drupal India Association commentedSeems to be working for me.
Comment #20
alexpottI 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.
Comment #21
brittany.huntzberryOr
Open to other suggestions too!
Comment #22
alexpottI think making a list would be good because the problem is the work disable and what it is applying to.
So something like:
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.
Comment #23
brittany.huntzberryOh 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
Comment #24
cilefen CreditAttribution: cilefen as a volunteer commentedI like the idea! There is just one adjustment I would prefer. "Typically used to..." is not the beginning of an English sentence.
Comment #25
nijolawrence CreditAttribution: nijolawrence as a volunteer commented@cliefen How about making it
Comment #26
alexpottsettings.local.php
is a singular thing ...It is typically...
Comment #27
brittany.huntzberryThis file is often used to:
orThis file is typically used to:
Is there any issue with being specific about the subject e.g. file?
Comment #28
cilefen CreditAttribution: cilefen as a volunteer commentedThere 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: ..."?
Comment #29
brittany.huntzberryThank 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!
Comment #30
brittany.huntzberryComment #31
cilefen CreditAttribution: cilefen as a volunteer commentedIn 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.
Comment #32
brittany.huntzberryThank you, I've updated the comments in a new patch.
Comment #33
tripurari CreditAttribution: tripurari commentedAssigning to myself and will be updating very shortly.
Comment #34
tripurari CreditAttribution: tripurari commentedComment #35
tripurari CreditAttribution: tripurari commented@brittany.huntzberry changes looks good to me as per latest patch #32. I would suggest it is good to go for RTBC.
Comment #36
tripurari CreditAttribution: tripurari commentedComment #37
brittany.huntzberryThank you for reviewing tripurari! I'll set this as RTBC for now, but anyone can provide feedback or additional testing still.
Comment #38
alexpott@brittany.huntzberry I know how frustrating it is for someone to say
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!
Comment #39
brittany.huntzberry@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)?
Comment #40
davidhernandez* development, etc) installations of this site.
"etc" is an abbreviation and should have a trailing period.
"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.
Comment #41
tripurari CreditAttribution: tripurari at Srijan | A Material+ Company for Drupal India Association commentedPlease Ignore the comments by mistake comment was added.
Comment #42
ankithashettyThe 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.
Comment #43
longwaveThe new wording is clearer to me, so this looks good to go.
Comment #44
alexpottCommitted 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