Fix duplicate word typos (the the, to to, etc.) for code comments

Note for #3: only change the es6.js files, and use yarn to recompile the JS ones. never edit them manually

@jungle you need to search the issue queue to ensure the duplicates of this issue are referenced.

1. +++ b/core/modules/jsonapi/src/Context/FieldResolver.php
   @@ -242,7 +242,7 @@ public static function resolveInternalIncludePath(ResourceType $resource_type, a
   -   * This method takes this external field expression and and attempts to
   +   * This method takes this external field expression and attempts to
       * resolve any aliases and/or abbreviations into a field expression that will
   

   +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
   @@ -170,7 +170,7 @@ public function getLanguageSwitchLinks(Request $request, $type, Url $url) {
   -   *   TRUE if the the content entity language negotiator has higher priority
   +   *   TRUE if the content entity language negotiator has higher priority
       *   than the url language negotiator, FALSE otherwise.
   

   The patch need to be checked for comments that need to be re-flowed now that lines are shorter... I've not checked it all.

2. Existing typo issues in the issue queue should be searched for.
3. Given the prevalence of some of these mistakes it might make sense to file an issue to add a rule to coder to detect the the and the like. Or actually looking at PHPCS - they are using cspell - maybe we should open an issue to try to configure for core. Yes it is a NPM package but at some point core is going to have build process that involves NPM so that shouldn't stop us. We have an issue for that already - see #2972224: Add .cspell.json to automate spellchecking in Drupal core - I wonder cspell detects this.

Mostly looks good, thanks!

However, in some places the duplicate word was supposed to be another similar word. E.g. "that that" needs to be "that the", not just "that". Details below:

1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -75,7 +75,7 @@ public function __construct(AccessManagerInterface $access_manager, AccountInter
   -   * subtrees to become accessible again, thus forcing us to conclude that that
   +   * subtrees to become accessible again, thus forcing us to conclude that
       * subtree is unconditionally inaccessible.
   

   For this to parse, it should be:

   ... that the subtree is unconditionally inaccessible.

   so the 2nd that here needs to become "the", not removed.

2. +++ b/core/modules/block/tests/src/Functional/Views/DisplayBlockTest.php
   @@ -195,7 +195,7 @@ public function testViewsBlockForm() {
   -    // Test that that machine name field is hidden from display and has been
   +    // Test that machine name field is hidden from display and has been
   

   "Test that the machine name..."

3. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeTest.php
   @@ -179,7 +179,7 @@ public function testRegionClass() {
   -   * separate file, so this test also ensures that that file is correctly loaded
   +   * separate file, so this test also ensures that file is correctly loaded
   

   s/that that/that the/

4. +++ b/core/modules/views/tests/src/Functional/Update/ImageStyleDependencyUpdateTest.php
   @@ -32,8 +32,8 @@ public function testUpdateImageStyleDependencies() {
   -    // We test the case the the field formatter image style doesn't exist.
   -    // Checks that 'nonexistent' image style is not a dependency of view 'foo'.
   +    // We test the case the field formatter image style doesn't exist. Checks
   +    // that 'nonexistent' image style is not a dependency of view 'foo'.
   

   "We test the case that the field ..."

5. +++ b/core/modules/views/tests/src/FunctionalJavascript/PaginationAJAXTest.php
   @@ -71,7 +71,7 @@ public function testBasicPagination() {
   -    // Make sure the the view_path is set correctly.
   +    // Make sure the view_path is set correctly.
   

   This is probably fine, although "Make sure that the view_path..." also works (and is probably what was originally intended).

6. +++ b/core/modules/views/tests/src/Kernel/Handler/FieldCounterTest.php
   @@ -116,7 +116,7 @@ public function testPager() {
   -    // Go the the second page.
   +    // Go the second page.
   

   "Go to the second page."

7. +++ b/core/modules/views/tests/src/Kernel/Handler/FieldCounterTest.php
   @@ -124,7 +124,7 @@ public function testPager() {
   -    // Go the the third page.
   +    // Go the third page.
   
   @@ -158,7 +158,7 @@ public function testPager() {
   -    // Go the the second page.
   +    // Go the second page.
   
   @@ -166,7 +166,7 @@ public function testPager() {
   -    // Go the the third page.
   +    // Go the third page.
   

   Go to the ...

8. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/FinalExceptionSubscriberTest.php
   @@ -38,7 +38,7 @@ public function testOnExceptionWithUnknownFormat() {
   -    // Also check that that text/plain content type was added.
   +    // Also check that text/plain content type was added.
   

   Also check that the text/plain...

I tested cspell and it doesn't detect duplicate words like the the I do think that maybe adding a duplicate word in comment checker to coder might be worth it. 30K is a large patch. Obviously with configuration so we can allow for duplicate had and other necessary words.... https://en.wikipedia.org/wiki/James_while_John_had_had_had_had_had_had_h...

Almost there, thanks! You just missed some of the reflow points @alexpott requested in #7.1. I'm reviewing 3121362-13-8.9.x.patch as the largest one, I assume the same points are to be found in the other patches, or aren't relevant on their branches...

1. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigDiffTest.php
   @@ -146,10 +146,10 @@ public function testCollectionDiff() {
   -   *   (optional) The original value of of the edit. If not supplied, assertion
   +   *   (optional) The original value of the edit. If not supplied, assertion
       *   is skipped.
   ...
   -   *   (optional) The closing value of of the edit. If not supplied, assertion
   +   *   (optional) The closing value of the edit. If not supplied, assertion
       *   is skipped.
   

   'is' would now fit on the previous line (in both cases).

2. +++ b/core/themes/claro/css/components/dropbutton.css
   @@ -413,7 +413,7 @@
   - * Set the the inherited button border color to transparent for high contrast
   + * Set the inherited button border color to transparent for high contrast
     * mode.
   
   +++ b/core/themes/claro/css/components/dropbutton.pcss.css
   @@ -353,7 +353,7 @@
   - * Set the the inherited button border color to transparent for high contrast
   + * Set the inherited button border color to transparent for high contrast
     * mode.
   

   'mode.' will now fit on the previous line (in both cases).

Otherwise, I think this is RTBC...

Thanks,
-Derek

p.s. Crediting everyone so far for reviews in here.

p.s. Similar to the point about es6.js vs. .js, I think you're now supposed to only edit the .pcss files for Claro, and then run something to regenerate the .css files from that. Haven't actually messed with that myself, yet, and perhaps the end result would be the same, but it might be worth trying to do that The Right Way(tm), just to make sure.

@Meenakshi.g: Thanks for fixing those.

<aside class="teaching-moment">
@jungle: Generally, if you've contributed substantially to the patches for an issue, you're not qualified to RTBC it. Technically #19 "isn't your patch" since @Meenakshi.g made those small tweaks. But the bulk of the work here is your own, so you should let someone else who hasn't written any of the patches do the RTBC'ing.

In this case, it's a trivial doc-only fix, so the stakes are low and it's basically fine. Plus I already said "Otherwise, I think this is RTBC..." in #17, and #18 indeed addressed the 2 points I raised.
</aside>

TL;DR: Confirming this is RTBC. ;)

+1 RTBC :)

Thanks for your work on this!

Just noting that since this change is altering deprecation messages, it's disruptive, so I'm not sure it's allowable during beta nor backportable to a patch release. We should consider splitting the comment-only fixes away from the changes to deprecation messages or exceptions. I'll get an opinion from other committers.

@larowlan and @catch also agreed we should split out the comment changes from the changes to deprecation or runtime error messages.

Let's use this issue to fix code comments for all active branches. Exception and deprecation messages should be fixed in a separate issue in 9.1.x only (although, skimming the D9 patch, there might be little to fix there anymore.) We'll just have to live with silly "the the" in some deprecation messages until the end of D8 support, but that's okay.

Thank you!

--- a/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
+++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
@@ -75,7 +75,7 @@ public function __construct(AccessManagerInterface $access_manager, AccountInter
* This is why inaccessible subtrees are deleted, except at the top-level
* inaccessible link: if we didn't keep the first (depth-wise) inaccessible
* link, we wouldn't be able to know which cache contexts would cause those
- * subtrees to become accessible again, thus forcing us to conclude that that
+ * subtrees to become accessible again, thus forcing us to conclude that the
* subtree is unconditionally inaccessible.
*
* @param \Drupal\Core\Menu\MenuLinkTreeElement[] $tree

Here i found very minor issue
'subtrees to become accessible again, thus forcing us to conclude that' - it should be like this.

mybe i am wrong. Please check with that issue

Sounds like duplicate

Thanks @jungle, @andypost, and @prabha1997.

#27 looks good. Tagging "Needs followup" for the 9.1.x patch (which will basically start with the 9.0.x interdiff in #27, reversed).

Agreed that #2851394: Fix grammar 'a' to 'an' when necessary is not a duplicate, although they have similarities.

For @prabha1997 regarding #29, I don't quite follow your suggestion. Sorry! I think it looks OK but it could be that I missed something too.

Oops, sorry, the followup was already filed in #28. Thanks!

Re #3122547: Fix duplicate word typos (the the, to to, etc) for test assertions and https://www.drupal.org/files/issues/2020-03-26/interdiff-9.0.x-19-27.txt

Why would we need to leave any of that broken in 9.0.x? It's only in tests. I understand the potential disruption of changing deprecation messages, but I don't understand the potential disruption of changing internal test module JS code and the assertions about its output. Tests are not API. Please explain why that's out of scope here and requires #3122547 to exist at all. Not that I'll ever understand our scoping policies, but I still try. ;)

Thanks,
-Derek

index 3645a0e5c6..01e69b51c7 100644
--- a/core/modules/aggregator/tests/src/Kernel/AggregatorPluginManagerTest.php

--- a/core/modules/aggregator/tests/src/Kernel/AggregatorPluginManagerTest.php
+++ b/core/modules/aggregator/tests/src/Kernel/AggregatorPluginManagerTest.php

+++ b/core/modules/aggregator/tests/src/Kernel/AggregatorPluginManagerTest.php
@@ -35,7 +35,7 @@ public function testParserInfoAlter() {

@@ -35,7 +35,7 @@ public function testParserInfoAlter() {
     $widget_definition = \Drupal::service('plugin.manager.aggregator.parser')->getDefinition('aggregator_test_parser');
 
     // Test if hook_aggregator_parser_info_alter is being called.
-    $this->assertTrue($widget_definition['definition_altered'], "The 'aggregator_test_parser' plugin definition was updated in `hook_aggregator_parser_info_alter()`");
+    $this->assertTrue($widget_definition['definition_altered'], "The 'aggregator_test_parser' plugin definition was updated in in `hook_aggregator_parser_info_alter()`");
   }
 
   /**
@@ -45,7 +45,7 @@ public function testProcessorInfoAlter() {

@@ -45,7 +45,7 @@ public function testProcessorInfoAlter() {
     $widget_definition = \Drupal::service('plugin.manager.aggregator.processor')->getDefinition('aggregator_test_processor');
 
     // Test if hook_aggregator_processor_info_alter is being called.
-    $this->assertTrue($widget_definition['definition_altered'], "The 'aggregator_test_processor' plugin definition was updated in `hook_aggregator_processor_info_alter()`");
+    $this->assertTrue($widget_definition['definition_altered'], "The 'aggregator_test_processor' plugin definition was updated in in `hook_aggregator_processor_info_alter()`");
   }
 
 }

Also, these should *definitely* be in scope here. They only change the assertion text, that presumably no one sees unless the test fails. ;)

@dww, it's just simpler to review only-comment code separately from anything that's not comment code, as per the guidelines in https://www.drupal.org/core/scope#context. It's not possible for pure documentation changes to break anything and it's always backportable during any release cycle phase. So I think #27 is still the correct scope. Thanks!

I agree with #38.

Setting to RTBC as the patch looks good and applies cleanly to each branch.

Hilarious. We split out #3122547: Fix duplicate word typos (the the, to to, etc) for test assertions since those were deemed harder to review/commit and it should speed this up. That issue already landed, while this one has not. ;) /shrug

Committed and pushed cc564e260e to 9.1.x and 7dcb6fa2ec to 9.0.x. Thanks!
Committed 9ac3a55 and pushed to 8.9.x. Thanks!
Committed ee34cc5 and pushed to 8.8.x. Thanks!