Split "This maybe" into "This may be", hyphenated some adjectives, and standardized the naming of modules.

CommentFileSizeAuthor
#8 1829366-settings-grammar-3.patch974 bytesgcassie
#5 1829366-settings-grammar-2.patch1.66 KBgcassie
#1 1829366-settings-grammar.patch3.61 KBgcassie
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gcassie’s picture

gcassie CreditAttribution: gcassie commented
Status: Active » Needs review
FileSize
1829366-settings-grammar.patch3.61 KB

patch attached

Status: Needs review » Needs work

The last submitted patch, 1829366-settings-grammar.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I have objection to these two

+ * By default, Drupal configuration files are stored in a randomly-named
+ * of heavily-visited sites and may also provide other site caching,

The added hyphen is unnatural and unneeded.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

RE #3 - actually I think those two changes are correct by English grammar/punctuation rules.

The changes in the patch look fine to me. But please take the chunk out that is not default.settings.php (we try to keep issues to one topic only). Thanks!

gcassie’s picture

gcassie CreditAttribution: gcassie commented
Status: Needs work » Needs review
FileSize
1829366-settings-grammar-2.patch1.66 KB

Sorry, the SessionTest stuff snuck in. New patch for default.settings.php only attached.

I double-checked the hyphens and since the words in question here are adverbs, they are not strictly needed. The new patch omits them.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

Thanks - this patch looks fine! I'll get it committed next week (there are a lot of sprints happening this weekend for BADCamp and committing patches ties up the test bot, so postponing a few days). This patch will most likely still apply to 7.x too; if not, after commit to 8.x someone can backport.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1829366-settings-grammar-2.patch, failed testing.

gcassie’s picture

gcassie CreditAttribution: gcassie commented
Status: Needs work » Needs review
FileSize
1829366-settings-grammar-3.patch974 bytes

Looks like the Ban logic was removed from default.settings.php .

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks! Sorry for the delay (there were sprints going on and I was avoiding commits for a while and then got busy) -- committed to 8.x and 7.x.

gcassie: is that your first core patch? Congrats!

Automatically closed -- issue fixed for 2 weeks with no activity.