Fix 'Drupal.Commenting.InlineComment.NotCapital' coding standard

Patch in #39 no longer applies. Using git apply --rej shows us that all changes apply except the one to core/phpunit.xml.dist, and then passes composer phpcs. So it should be easy to reroll.

MR in #35 seems broken.

In an ideal world, the MR would reflect the current changes, but it might be less work just to stick with patches. Dunno.

Hello @Mile23! Could you please try against the latest core 10.1.x? The #39 patch still applies successfully.
Also, there is no change made to file core/phpunit.xml.dist in the patch.🤔

Restoring back to the previous state. Thanks!

I created a patch following the steps described in the IS and compared it with the patch from #39, I saw some differences, but they were discussed in the comments. So it looks good for me.

The last submitted patch, 39: 2719657-39.patch, failed testing. View results →

The test failure was a known random failure, so retesting and setting back RTBC.

Closing the broken MR for clarity and removing credit for it.

xjm → closed merge request !3087

Is there a separate rule we can enable for comments that don't end in a period? Because a lot of these qualify.

1. Etag must match.

   I think ETag is actually the canonical capitalization. The method is only named getEtag() because of our camel-casing spelling rules.

2. I think we need to rethink how we're addressing the comments in
   core/modules/system/tests/src/Kernel/Migrate/d6/MigrateSystemConfigurationTest.php.

   For example

   • Uuid is not handled by the migration.

     This should be "UUID" when used in a sentence.

Autorun is not handled by the migration.

If this is used as a word in a sentence, it should be "Auto-run".

3. Langcode and default_langcode are not handled by the migration.

   This is inconsistent. Either both langcode and default_langcode are machine names, or they should be treated as words in a sentence, i.e. "Language code" and "Default language code".

4. For all of the above -- and other similar comments in the same file about gzip and such I think the sentence should be rephrased as:

   The 'foo' property is not handled by the migration.

   I'm not sure if it's a "property", maybe it's a "configuration" or a "settting" or something else. Use the appropriate noun in context.

5. // suggestionnotimplemented() is not an implemented theme hook so

   I could not find suggestionnotimplemented() anywhere in our API. It appears to be test data used in ThemeTest and RendererTest. The CSpell issues will need to fix this eventually, but just adding parentheses is fine for now.

6.     // Allow + for or, , for and
   

   Uh, what? Reading the code, I think this means:

   // Allow '+' for "or". Allow ',' for "and".
   

   (Though who would think + meant "or"?) Anyway. Followup for this.

7.     // Init the table queue with our primary table.
   

   This needs to use the full word, "Initialize".

8. There is an out-of-scope hunk in core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php adding string typehints to a method.

9. That same class rewrites a data provider to use meaningful array keys rather than inline comments on the cases, which is a good change. That could be handled in a separate, blocking issue to make things easier.

10. The hunks in core/modules/views/views.api.php are weird; we have the sentence fragments:
    // Although the table aliases will be different.

    I looked at this in context and the inline comment before the code snippet is:

      // If you've decided an automatic join is a good idea, here's how to do it;   
      // the resulting SQL query will look something like this: 
    

    So, I think we should changes these two to:

    // (The table aliases will be different.)
    

11.     // Todo mock a request with a route.
    

    This should actually be changed to:

        // @todo Mock a request with a route.
    

Changes described in comment number 49 have been applied.

Here is the patch and inter-diff for the same -

Very little of #49 is actually addressed, at least based on the interdiff posted.

quietone → reopened merge request !3087

Converted to an MR, ran phpbcf and then started work on #49.

#49.
1. Fixed
2. The first two are changed as asked for. The third corrects 'Langcode' but does not rework the sentences to use quotes around the variable names. I agree that it is correct and we are touching that line but I stopped because of the comment suggests it be done in the rest of the file. That would be out of scope. Even with inconsistencies across files I prefer to keep the style of comments in a file the same for readability.
3. Fixed in 📌 Correct spelling of words in module core/tests Fixed
4. Still needs a followup.
5. Fixed.
6, 7 No longer relvant
8. No change. The suggested change doesn't make sense to me at this late hour.
9. Fixed

#49.
4. Fixed.
8. Fixed

Of the points in #49 only 49.2 was one I had a question on. Left the question in the MR but will leave in review for others.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

My one comment/concern has been answered. Believe this is good

Committed/pushed to 11.x. I also committed/cherry-picked the same diff minus the actual phpcs.xml.dist change, to 11.0.x, 10.4.x, and 10.3.x to keep codebase in sync.

catch → closed merge request !3087

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