Spellchecking Drupal - Comments

First off, this is a very nice effort. Thanks!

As to #4 and #5: as I see it, this is a widespread update of the documentation, but I'm not sure whether it would be disruptive for a lot of other patches. Only conflicts in these specific lines would cause automatic merge fails, right? I also don't think that cutting the patch into two or more pieces would be any better, since we then would have (admittedly smaller) widespread changes over a longer period of time.
I'd vote to get it in, because the merge conflicts that could arise are really easy to resolve and the chance that we introduce a new issue is non-existent.

No matter the outcome of that discussion, I reviewed the patch because I fully agree with the issue summary and I think that we should do this sooner rather than later.

Some notes out front:
- I focused on the errors, and not whether we always have full sentences, dots, commas, etc. (although I sometimes mention it anyway)
- Comment lines should not be longer than 80 characters. While I know this isn't the main focus in this issue, I don't think we should introduce new violations against the documentation guideline. We should however take in mind that fixing these will make the patch even bigger, so we might want to do this in a follow-up issue.

1. +++ b/core/modules/block/src/Tests/BlockLanguageTest.php
   @@ -11,7 +11,7 @@
   + * Tests if a block can be configure to be only visible on a particular
   

   configured

2. +++ b/core/modules/block_content/src/Tests/BlockContentSaveTest.php
   @@ -79,7 +79,7 @@ public function testDeterminingChanges() {
        // block_content_test_block_content_update() determine changes and change the
   

   80 chars

3. +++ b/core/modules/config/src/Tests/ConfigExportImportUITest.php
   @@ -177,7 +177,7 @@ public function testExportImportCollections() {
   +    // Create the tar contains the expected content for the collections.
   

   the tar that contains

4. +++ b/core/modules/config/src/Tests/ConfigOverrideTest.php
   @@ -103,7 +103,7 @@ function testConfOverride() {
   +    // Verify the overrides are still working.
   

   Verify that

5. +++ b/core/modules/content_translation/src/Tests/ContentTranslationWorkflowsTest.php
   @@ -83,7 +83,7 @@ function testWorkflows() {
   +    // Check that translation permissions govern the associated operations.
   

   Govern? I'm not sure what that means. Allow probably?

6. +++ b/core/modules/field_ui/src/Tests/EntityDisplayTest.php
   @@ -92,7 +92,7 @@ public function testEntityDisplayCRUD() {
   +    // Check that CreateCopy() creates a new component that can be correctly
   

   createCopy() (lower camel case)

7. +++ b/core/modules/node/src/Tests/NodeRevisionPermissionsTest.php
   @@ -133,7 +133,7 @@ function testNodeRevisionAccessPerType() {
   +    // Test that the accounts have access to the corresponding page revision permissions.
   

   80 chars

8. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -288,7 +288,7 @@ protected function drupalCreateContentType(array $values = array()) {
   +   * renderable array for an entity is constructed in drupal_render(). Some tests
   

   80 chars

9. +++ b/core/modules/system/src/Tests/Cache/GenericCacheBackendUnitTestBase.php
   @@ -518,12 +518,12 @@ function testDeleteTags() {
   +    // Test that cache entry has been deleted in multiple bins.
   

   the

10. +++ b/core/modules/system/src/Tests/Cache/GenericCacheBackendUnitTestBase.php
    @@ -630,12 +630,12 @@ function testInvalidateTags() {
    +    // Test that cache entry has been invalidated in multiple bins.
    

    the

11. +++ b/core/modules/system/src/Tests/Common/MergeAttachmentsTest.php
    @@ -19,7 +19,7 @@
    +   * Tests just library asset merging.
    

    Remove "just" or replace the order to "Just tests" or "Only tests".

12. +++ b/core/modules/system/src/Tests/Common/MergeAttachmentsTest.php
    @@ -67,7 +67,7 @@ function testLibraryMerging() {
    +   * Tests just JavaScript and JavaScript setting asset merging.
    

    And again.

13. +++ b/core/modules/system/src/Tests/Entity/EntityQueryAggregateTest.php
    @@ -526,7 +526,7 @@ public function testAggregation() {
    +    // Groupby and aggregate by fieldapi field, and sort by the aggregated field.
    

    "Groupby" just seems odd like this, but I get it.
    80 chars though :)

14. +++ b/core/modules/system/src/Tests/ParamConverter/UpcastingTest.php
    @@ -22,7 +22,7 @@ class UpcastingTest extends WebTestBase {
    +   * All of these requests end up being processed by a controller with this
        * the signature: f($user, $node, $foo) returning either values or labels
    

    with this the signature (remove "this")

15. +++ b/core/modules/system/src/Tests/Theme/EnginePhpTemplateTest.php
    @@ -29,7 +29,7 @@ protected function setUp() {
    +   * Ensures a theme's template is overridable based on the 'template' filename.
    

    Very convenient fix to 80 chars ;)

16. +++ b/core/modules/system/theme.api.php
    @@ -35,7 +35,7 @@
    + * registered by modules by implementing hook_theme(), which specifies the name of
    

    80 chars

17. +++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
    @@ -648,7 +648,7 @@ public function defaultAction($info = NULL) {
    +   * How to act if validation fails
    

    I find the phrasing of this comment not that great. I think that is out of scope for this issue though.

18. +++ b/core/modules/views/src/Plugin/views/argument_validator/ArgumentValidatorPluginBase.php
    @@ -102,7 +102,7 @@ public function validateArgument($arg) { return TRUE; }
    +   * Some plugins alter the argument so it uses something else internal.
    

    internally?

19. +++ b/core/modules/views/src/Plugin/views/filter/Date.php
    @@ -108,7 +108,7 @@ public function validateValidTime(&$form, FormStateInterface $form_state, $opera
         // Special case to validate grouped date filters, this is because the
         // $group['value'] array contains the type of filter (date or offset)
    -    // and therefore the number of items the comparission has to be done
    +    // and therefore the number of items the comparison has to be done
         // against 'one' instead of 'zero'.
    

    This full inline comment doesn't feel right. Probably out of scope here.
    The typo is fixed.

20. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
    @@ -1309,7 +1309,7 @@ public function storeGroupInput($input, $status) {
    +    // false means that we got a setting that means to recurse ourselves,
    

    False (capital)

21. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
    @@ -1401,7 +1401,7 @@ public function storeExposedInput($input, $status) {
    +    // false means that we got a setting that means to recurse ourselves,
    

    And again.

22. +++ b/core/modules/views/src/Plugin/views/query/Sql.php
    @@ -603,7 +603,7 @@ protected function ensurePath($table, $relationship = NULL, $join = NULL, $trace
    +      // we looped. Broken.
    

    Capital.

23. +++ b/core/modules/views/src/Plugin/views/relationship/GroupwiseMax.php
    @@ -120,7 +120,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +    // WIP: This stuff doesn't work yet: namespacing issues.
    

    Work in progress generally means that the "stuff" doesn't fully work yet :)
    The typo got fixed though.

24. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
    @@ -613,7 +613,7 @@ public function renderGrouping($records, $groupings = array(), $group_rendered =
    +    // If this parameter isn't explicitly set modify the output to be fully
    

    set, modify (add comma)

25. +++ b/core/modules/views/src/Tests/GlossaryTest.php
    @@ -28,7 +28,7 @@ class GlossaryTest extends ViewTestBase {
    +    // create a content type and add some nodes, with a non random title.
    

    non-random?

26. +++ b/core/modules/views/src/Tests/Handler/FieldWebTest.php
    @@ -290,7 +290,7 @@ public function testAlterUrl() {
    +    // Tests the linkclass setting and see whether it actually exists in the output.
    

    80 chars

27. +++ b/core/modules/views/src/Tests/Handler/HandlerAllTest.php
    @@ -86,7 +86,7 @@ public function testHandlers() {
    +      // Go through each step individually to see whether some parts are failing.
    

    80 chars

28. +++ b/core/modules/views/tests/modules/views_test_data/src/Plugin/views/display/DisplayTest.php
    @@ -50,7 +50,7 @@ protected function defineOptions() {
    +   * Overrides Drupal\views\Plugin\views\display\DisplayPluginBase::optionsSummary().
    

    80 chars. Not sure how this should be fixed though.

29. +++ b/core/modules/views/tests/src/Unit/Plugin/pager/PagerPluginBaseTest.php
    @@ -249,7 +249,7 @@ public function testExecuteCountQueryWithOffset() {
    +// As StatementInterface extends \Traversable, which though always needs
    

    though?

30. +++ b/core/modules/views/views.theme.inc
    @@ -117,7 +117,7 @@ function template_preprocess_views_view_fields(&$variables) {
    +      // Protect ourselves somewhat for backward compatibility. This will prevent
           // old templates from producing invalid HTML when no element type is selected.
    

    Protecting against?
    80 chars (twice)

31. +++ b/core/modules/views_ui/css/views_ui.admin.theme.css
    @@ -683,11 +683,11 @@ td.group-title {
    + * Applies a overridden(italics) font style to overridden buckets.
    

    an?

The patch provided has the TransliterationTest and the Source class in it, which is probably a rebase issue. It causes the test failures.

Glad to see this green again. Thank you! The patch looks great already!

I tested my spot the difference skills again, and have some additional feedback:

1. +++ b/core/modules/language/src/Tests/LanguageUrlRewritingTest.php
   index bf62cf7..7323c14 100644
   --- a/core/modules/language/tests/src/Unit/LanguageNegotiationUrlTest.php
   
   +++ b/core/modules/search/src/Tests/SearchSimplifyTest.php
   @@ -21,7 +21,7 @@ function testSearchSimplifyUnicode() {
   diff --git a/core/modules/serialization/src/Tests/SerializationTest.php b/core/modules/serialization/src/Tests/SerializationTest.php
   
   +++ b/core/modules/system/src/Form/PerformanceForm.php
   @@ -54,18 +54,18 @@ class PerformanceForm extends ConfigFormBase {
   -  public function __construct(ConfigFactoryInterface $config_factory, CacheBackendInterface $render_cache, DateFormatter $date_formater, AssetCollectionOptimizerInterface $css_collection_optimizer, AssetCollectionOptimizerInterface $js_collection_optimizer) {
   +  public function __construct(ConfigFactoryInterface $config_factory, CacheBackendInterface $render_cache, DateFormatter $date_formatter, AssetCollectionOptimizerInterface $css_collection_optimizer, AssetCollectionOptimizerInterface $js_collection_optimizer) {
        parent::__construct($config_factory);
    
        $this->renderCache = $render_cache;
   -    $this->dateFormatter = $date_formater;
   +    $this->dateFormatter = $date_formatter;
   

   These fixes will need to be removed, since they are taken care of in #2383871: Spellchecking Drupal - PHP. They will cause a merge conflict.

2. +++ b/core/modules/system/src/Tests/Entity/EntityAccessControlHandlerTest.php
   @@ -2,7 +2,7 @@
   + * Contains \Drupal\system\Tests\Entity\EntityAccessHControlHandlerTest.
   

   "EntityAccessControlHandlerTest". There is an "H" too much there.

3. +++ b/core/modules/system/src/Tests/Theme/EnginePhpTemplateTest.php
   @@ -29,7 +29,7 @@ protected function setUp() {
   +   * Ensures a theme's template is overridable based on the 'template' filename.
   

   Not really sure about this, but shouldn't that be "themes" without the single quote?

4. +++ b/core/modules/taxonomy/src/TermInterface.php
   @@ -45,7 +45,7 @@ public function getFormat();
   +   *   The term's description text format.
   

   This should be "terms".

5. +++ b/core/modules/views/tests/modules/views_test_data/src/Plugin/views/display/DisplayTest.php
   @@ -50,7 +51,7 @@ protected function defineOptions() {
   +   * Overrides Drupal\views\Plugin\views\display\DisplayPluginBase::optionsSummary().
   

   80 chars.

Ah ok, my bad, I wasn't sure. But the change wasn't made in the last patch, so we are good here.

I think we'll be able to rtbc this with a final review, but I don't have enough time left just now.