Problem/Motivation

Discovered at #2972224: Add .cspell.json to automate spellchecking in Drupal core, and pointed by @xjm in https://www.drupal.org/project/drupal/issues/3122088#comment-13628724

+++ b/core/.cspell.json
@@ -0,0 +1,1288 @@
+      "DONT",
...
+      "Dont",
...
+      "don't",
@xjm:

Unless this is French, it should probably be DoNot/DO_NOT in code and Don't or Do not in text. There may be a whole category in here of contractions without their apostrophes.

Proposed resolution

As title and see the change record https://www.drupal.org/node/3122084 for how to work with cspell.

Remaining tasks

Pick out all applicable words from #2972224: Add .cspell.json to automate spellchecking in Drupal core and fix them.

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jungle created an issue. See original summary.

jungle’s picture

mohrerao’s picture

Assigned: Unassigned » mohrerao
mohrerao’s picture

Status: Active » Needs review
FileSize
1023 bytes
mohrerao’s picture

Issue tags: +DIACWMay2020
mohrerao’s picture

Assigned: mohrerao » Unassigned
longwave’s picture

Status: Needs review » Needs work

There are more instances of "dont", they appear in code and we need to change these to "do_not" etc

mohrerao’s picture

@longwave
This may impact some of the test methods which may have been used in other modules/projects/tests.
Also found one instance of url: https://www.drupal.org/docs/8/upgrade/preparing-an-upgrade#dont_create_c....
Shall i go ahead and update?

longwave’s picture

See https://www.drupal.org/core/d8-bc-policy#tests for the policy regarding tests. Basically we can change individual tests as necessary except for base classes and helpers.

The URL will probably have to be left alone for now.

mohrerao’s picture

@longwave. Thanks for the info. Attaching updated patch.

mohrerao’s picture

Status: Needs work » Needs review
sja112’s picture

Assigned: Unassigned » sja112
longwave’s picture

Status: Needs review » Reviewed & tested by the community

This covers all cases of "dont" or "DONT" in core except one in a d.o URL anchor that we can't change. There are no BC issues because everything is either in test code or CSS comments. We are changing core/tests/Drupal/Tests/Component/Annotation/Doctrine/DocParserTest.php which we inherited from Doctrine but I think a minor spelling fix is acceptable here. Therefore, this is RTBC.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup
  1. +++ b/core/modules/jsonapi/tests/src/Kernel/Query/FilterTest.php
    @@ -67,10 +67,10 @@ public function setUp(): void {
    -      ['colors' => ['orange'], 'shapes' => ['triangle'], 'title' => 'DONT_FIND'],
    +      ['colors' => ['orange'], 'shapes' => ['triangle'], 'title' => 'DO_NOT_FIND'],
           ['colors' => ['yellow'], 'shapes' => ['square'], 'title' => 'FIND'],
    -      ['colors' => ['yellow'], 'shapes' => ['triangle'], 'title' => 'DONT_FIND'],
    -      ['colors' => ['orange'], 'shapes' => ['square'], 'title' => 'DONT_FIND'],
    +      ['colors' => ['yellow'], 'shapes' => ['triangle'], 'title' => 'DO_NOT_FIND'],
    +      ['colors' => ['orange'], 'shapes' => ['square'], 'title' => 'DO_NOT_FIND'],
    

    For these, I verified that the title data is not ever actually used in the test; it's just meant to distinguish which scenarios meet the logic requirements vs. not. So that's all good.
     

  2. I also read over MigrateSkipRowTest and SessionTest and their fixtures rather carefully to make sure that the changes to the fixtures didn't alter the test coverage in any way.
     

  3. Meanwhile, I updated this URL fragment in the handbook:
    https://www.drupal.org/docs/upgrading-drupal/preparing-a-site-for-upgrad...

    I left an <a> tag with the old name/ID in place also, so that anything else that had the old link would not be broken. Confirmed the link works as expected.

    Updating that URL is also technically backportable since it's one of the placeholder values of the string, rather than part of the first argument of
    t(), so it's not a string break. That said, for good issue scoping, we should still probably separate the test-only changes here from that change.

  4. +++ b/core/tests/Drupal/Tests/Component/Annotation/Doctrine/DocParserTest.php
    @@ -1013,7 +1013,7 @@ public function testNotAnAnnotationClassIsIgnoredWithoutWarning()
    -    public function testAnnotationDontAcceptSingleQuotes()
    +    public function testAnnotationDoNotAcceptSingleQuotes()
    

    While @longwave is correct that this would be harmless to change, this file is actually a coding-standards-ignored file, so it should also be a cspell-ignored file IMO. In fact, the entire component should be as well as its test namespace. Can we remove this change from the patch, and incorporate that back into the parent issue?

I grepped the rest of our codebase and... the remaining instances are in fact the French word "dont". La chose exacte dont je parlais! 🇫🇷

NW for that last bullet and also to file the followup for the string URL correction. Thanks!

sja112’s picture

On this.

sja112’s picture

Status: Needs work » Needs review
FileSize
9.85 KB
793 bytes

Addressing #14.4

sja112’s picture

Assigned: sja112 » Unassigned
Issue tags: -Needs followup
mohrerao’s picture

Status: Needs review » Reviewed & tested by the community

Ran grep -ri "Dont" * and could find occurance of dont in translations and in /vendor/ckeditor/ckeditor.js which should be OK

modules/migrate_drupal/tests/fixtures/drupal6.php:  'translation' => 'Configurez ici la manière dont les champs et étiquettes de champs de ce type de contenu doivent être affichées, lorsque le contenu est vu en mode résumé ou en pleine page.',
modules/migrate_drupal/tests/fixtures/drupal6.php:  'translation' => "Configurez ici la façon dont les champs de ce type de contenu doivent être affichés lorsqu'il est rendu dans les contextes suivants.",
modules/migrate_drupal/tests/fixtures/drupal6.php:  'translation' => "Configurer la manière dont l'étiquette est affichée.",
modules/migrate_drupal_ui/src/Form/OverviewForm.php:      ':url' => 'https://www.drupal.org/docs/8/upgrade/preparing-an-upgrade#dont_create_content',
tests/Drupal/Tests/Component/Annotation/Doctrine/DocParserTest.php:    public function testAnnotationDontAcceptSingleQuotes()

RTBC for me

xjm’s picture

Status: Reviewed & tested by the community » Needs work

CSpell landed today, so we can make sure this spelling error never happens again by removing the entry from Drupal's dictionrary, so let's update the patch to include that. I think the French content should already be ignored, but if not, we should ignore the translations where it appears.

We should also add all of core/tests/Drupal/Tests/Component/Annotation/Doctrine to the ignored files. Thanks!

nikitagupta’s picture

Assigned: Unassigned » nikitagupta
mohrerao’s picture

Removed 'dont' from core/misc/cspell/dictionary.txt and added core/tests/Drupal/Tests/Component/Annotation/Doctrine to the ignored files.

mohrerao’s picture

Status: Needs work » Needs review
nikitagupta’s picture

Assigned: nikitagupta » Unassigned
longwave’s picture

Status: Needs review » Needs work
+++ b/core/.cspell.json
@@ -21,7 +21,8 @@
+      "core/tests/Drupal/Tests/Component/Annotation/Doctrine"

These paths are relative to the "core" directory so we don't need "core/" at the start of the path.

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
10.58 KB
410 bytes

Here I have addressed comment #24.

longwave’s picture

We also need to add /** to ignore all the files below that directory, and I took the liberty of keeping the (mostly) alphabetical order of the exclusions as well.

longwave’s picture

So we should also regenerate the dictionary, as there are a number of misspelled words that only appear in the Doctrine component. The attached patch removes those words from the dictionary as well as "dont".

jungle’s picture

Status: Needs review » Needs work
Issue tags: +Global2020, +Needs reroll
  1. Checked with cd core && yarn && yarn build:css and confirmed the css file touched is generated.
  2. Needs reroll
  3. Tagging "Global2020"
jameszhang023’s picture

Assigned: Unassigned » jameszhang023

Working on this.

jameszhang023’s picture

Assigned: jameszhang023 » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
10.93 KB
1.18 KB

Rolling done, please review, thanks.

jungle’s picture

Status: Needs review » Reviewed & tested by the community
  1. $ yarn spellcheck:core
    yarn run v1.22.4
    $ cspell "**/*" "../composer/**/*" "../composer.json"
    CSpell: Files checked: 14600, Issues found: 0 in 0 files
    ✨  Done in 255.94s.
    
    

    Spelling check passed.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigFileContentTest.php
    @@ -111,10 +111,10 @@ public function testReadWriteConfig() {
    diff --git a/core/themes/claro/css/components/tabledrag.css b/core/themes/claro/css/components/tabledrag.css
    
    +++ b/core/themes/claro/css/components/tabledrag.css
    @@ -92,7 +92,7 @@ body.drag {
    diff --git a/core/themes/claro/css/components/tabledrag.pcss.css b/core/themes/claro/css/components/tabledrag.pcss.css
    

    There are CSS-related changes, checked with cd core && yarn build:css, confirmed that changes are all good.

  3. +++ b/core/misc/cspell/dictionary.txt
    @@ -48,8 +48,6 @@ and's
    -annotationwithconstants
    -annots
    
    @@ -292,7 +290,6 @@ classmap
    -classwithconstants
    
    @@ -404,7 +401,6 @@ daycounter
    -dcom
    
    @@ -476,7 +472,6 @@ docroot
    -dont
    

    5 words removed in total.

  4. Changes are made to tests mainly, no BC concerns etc.

Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: 3138766-30.patch, failed testing. View results

jungle’s picture

Status: Needs work » Reviewed & tested by the community

Random failure, re-queued.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll.

Lal_’s picture

FileSize
2.83 KB

rerolled

Lal_’s picture

Status: Needs work » Needs review
FileSize
10.93 KB

oops the patch

longwave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Thanks for rerolling, back to RTBC.

alexpott’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 5412462 and pushed to 9.1.x. Thanks!
Committed and pushed 3b668ff177 to 9.0.x and 3a730ce872 to 8.9.x. Thanks!

Backported to 8.9.x without the cspell changes as this is test only.

  • alexpott committed 5412462 on 9.1.x
    Issue #3138766 by mohrerao, longwave, sja112, jameszhang023, Lal_, ravi....

  • alexpott committed 3b668ff on 9.0.x
    Issue #3138766 by mohrerao, longwave, sja112, jameszhang023, Lal_, ravi....

  • alexpott committed 3a730ce on 8.9.x
    Issue #3138766 by mohrerao, longwave, sja112, jameszhang023, Lal_, ravi....

Status: Fixed » Closed (fixed)

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