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

I'm on it.

Unfortunately neither patch applies.

Check, please, re-rolled patch.

1. +++ b/core/lib/Drupal/Core/Database/StatementInterface.php
   @@ -36,8 +36,11 @@
   +   *
   +   * @todo Move out of interface in https://www.drupal.org/node/2168241
   +   *
   +   * protected function __construct(Connection $dbh);
       */
   -  //protected function __construct(Connection $dbh);
   

   This looks wrong. Let's add the @todo but remove the code.

2. +++ b/core/lib/Drupal/Core/FileTransfer/FTPExtension.php
   @@ -108,7 +108,7 @@ function chmodJailed($path, $mode, $recursive) {
   -        //empty directory - returns false
   +        // Empty directory - returns FALSE.
            return;
   

   Well it doesn't return FALSE. I think we should just make the comment // Empty directory.

3. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
   @@ -242,8 +242,8 @@ public function stream_eof() {
   -    // fseek returns 0 on success and -1 on a failure.
   -    // stream_seek   1 on success and  0 on a failure.
   +    // The fseek() returns 0 on success and -1 on a failure.
   +    // The stream_seek()   1 on success and  0 on a failure.
   

   This looks wrong. The fseek() is odd. The PHPCS rule should be clever enough to allow comments to begin with function names imo.

4. +++ b/core/modules/color/color.module
   @@ -681,7 +681,7 @@ function _color_shift($given, $ref1, $ref2, $target) {
   -  // delta = 1 - |ref2 - ref1| / |white - ref1|
   +  // Calculates delta = 1 - |ref2 - ref1| / |white - ref1|.
   

   Hmmm this all looks odd.

5. +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldFormatterSettingsTest.php
   @@ -132,7 +132,7 @@ protected function setUp() {
   -                  // settings is always expected to be an array. Making it NULL
   +                  // Settings is always expected to be an array. Making it NULL
                      // causes a fatal.
   

   Maybe The 'settings' value is... since Settings is not in the array.

6. +++ b/core/modules/filter/filter.module
   @@ -496,8 +496,8 @@ function _filter_url($text, $filter) {
   -  //full path
   -  //and allow @ in a url, but only in the middle. Catch things like http://example.com/@user/
   +  // Allow @ in a URL but only in the middle.
   +  // Catch things like http://example.com/@user/.
   

   This comment is not correctly wrapped.

7. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -113,7 +113,7 @@ public function fields() {
   +    // The format can be 0 when the body field is hidden. Set the format to 1
        // to avoid migration map issues (since the body field isn't used anyway).
   

   Comment wrapping. to can now go on the previous line.

8. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
   @@ -119,7 +119,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -      $form['remove_groups'][$id] = array(); // to prevent a notice
   +      $form['remove_groups'][$id] = array(); // To prevent a notice.
   

   Bit of a useless comment.

This is blocked on #2168241: Type hints for optional methods in StatementInterface (D8) / DatabaseStatementInterface (D7).

This was postponed on an issue that is now closed as outdated, so work can begin again.

#11.3 Here alexpott thinks the sniff should be smarter so that lower case function names can start a sentence. Should this be postponed on improvements to the sniff or add ignore lines before comments like that?

Rerolled the patch in #9. Still need to address #11.

And now respond to #11.

1. No longer relevant, constructor removed.
2. Fixed.
3. Surrounded comment with phpcs:disable/enable
4. Looks odd to me to. Not sure what to do to improve it.
5. No longer relevant
6. Wrapping fixed.
7. Wrapping fixed.
8. No longer relevant.

The drupalci.yml file is changed to just run commit-code-check. I did that in the patch in #21 but made a mistake.

Add periods to end of comments.

I also noticed that the sniff finds // prefix but not // prefix.

A do-over is easier than a re-roll.

Special call-out to core/modules/views/tests/src/Functional/Plugin/ArgumentDefaultTest.php, where a test function was commended out with a todo, and now I've upgraded it to a test marked incomplete. This makes me wonder if #2905007: Allow run-tests.sh to report skipped/incomplete PHPUnit tests is still needed, but that's for later. My wager is that a reviewer will want to just delete the test function.

The rest of the changes are pretty simple, and it looks like all the concerns from #11 have been refactored away in some form or another over time.

Must declare visibility....

We're going to need a patch for 10.1.x / 10.0.x too.

Also I think we need to have a think about some of the comments that are being changed here.

1. +++ b/core/modules/file/tests/src/Kernel/CopyTest.php
   @@ -165,7 +165,7 @@ public function testExistingError() {
   -      // expected exception.
   +      // Expected exception.
   
   +++ b/core/modules/file/tests/src/Kernel/FileRepositoryTest.php
   @@ -151,7 +151,7 @@ public function testExistingError() {
   -      // expected exception.
   +      // Expected exception.
   
   +++ b/core/modules/file/tests/src/Kernel/MoveTest.php
   @@ -154,7 +154,7 @@ public function testExistingReplaceSelf() {
   -      // expected exception.
   +      // Expected exception.
   
   @@ -186,7 +186,7 @@ public function testExistingError() {
   -      // expected exception.
   +      // Expected exception.
   

   This comment is not worthwhile.

2. +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
   @@ -373,7 +373,7 @@ public function providerTestRenderAsLinkWithPathAndOptions() {
        // entity_type flag.
        $entity_type_id = 'node';
        $data[] = ['test-path', ['entity_type' => $entity_type_id], '<a href="/test-path">value</a>'];
   -    // prefix
   +    // Prefix
        $data[] = ['test-path', ['prefix' => 'test_prefix'], '<a href="/test-path">value</a>', 'test_prefix<a href="/test-path">value</a>'];
        // suffix.
        $data[] = ['test-path', ['suffix' => 'test_suffix'], '<a href="/test-path">value</a>', '<a href="/test-path">value</a>test_suffix'];
   

   Why not // suffix. too - also this would be better to change all this to keys for $data.

10.1.x Reroll for #27

Fixed CCF in #31

I will review the patch

#31 #32 patch failed to applied

Can we not "just" get a reroll but can we also, at the same time, address the points in #29?

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 test failure was a known random failure, so retesting and setting back RTBC.

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

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

Edit: Similarly an inline comment line length rule.

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".

   • 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".

   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.
    

3. // 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.

4.     // 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.

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

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

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

7. 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.
    

8. 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.)
   

9.     // 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.

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 #3395900: Correct spelling of words in module core/tests
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.