Fix method comments in tests for Drupal.Commenting.DocComment.ShortSingleLine

Back to need review.

Tested this locally by commenting out the line in the phpcs file and running on the files from the diff.

FILE: /Users/smustgrave/Sites/Drupal-Demos/drupal-10.x/web/core/modules/dblog/tests/src/Functional/DbLogTest.php
------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------
 18 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph

FILE: /Users/smustgrave/Sites/Drupal-Demos/drupal-10.x/web/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceFormatterTest.php
-----------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------------------
 58 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
-----------------------------------------------------------------------------------------------------------------------

FILE: /Users/smustgrave/Sites/Drupal-Demos/drupal-10.x/web/core/modules/field/tests/src/Kernel/FieldImportDeleteUninstallTest.php
---------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------------------------
 11 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
----------------------------------------------------------------------------------------------------------------------------

FILE: /Users/smustgrave/Sites/Drupal-Demos/drupal-10.x/web/core/modules/file/tests/src/Functional/FileManagedTestBase.php
-------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------
 12 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph

FILE: /Users/smustgrave/Sites/Drupal-Demos/drupal-10.x/web/core/modules/file/tests/src/Kernel/FileManagedUnitTestBase.php
-------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------
 13 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph

FILE: /Users/smustgrave/Sites/Drupal-Demos/drupal-10.x/web/core/modules/locale/tests/src/Functional/LocaleTranslationUiTest.php
-------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------------
 13 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph

FILE: /Users/smustgrave/Sites/Drupal-Demos/drupal-10.x/web/core/modules/migrate/tests/src/Unit/MigrateSourceTest.php
--------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------
 503 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph

FILE: /Users/smustgrave/Sites/Drupal-Demos/drupal-10.x/web/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
-------------------------------------------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
-------------------------------------------------------------------------------------------------------------------------------------------
 30 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
 89 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
 97 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph

FILE: /Users/smustgrave/Sites/Drupal-Demos/drupal-10.x/web/core/modules/system/tests/src/Functional/Form/StorageTest.php
------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------
 11 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph

#19. All the instances reported are out of scope for this issue. I know, one can get lost on which subset of the problem these issues are focused on.

They are either a class comment or a class property comment. Class comments are being done in 📌 Fix class comment doc blocks in tests for 'Drupal.Commenting.DocComment.ShortSingleLine' Fixed . Class property were done in #3268829: Fix class property comments for Drupal.Commenting.DocComment.ShortSingleLine → and it looks like a few were missed. If there are only a few of these, then we may be able to done them in the last issue of this sniff. 📌 Enable 'Drupal.Commenting.DocComment.ShortSingleLine' coding standard Fixed .

I am setting back to NR.

Thanks @quietone

there's one thread

This should also be an @covers. Maybe we could file a followup for this and the previous spot I mentioned it to keep the scope constrainted.

Does this need to be addressed?

So believe all the points have been addressed? Large MRs certainly are difficult.

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

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Rebased and tests passing, restoring RTBC

This needs reroll for 10.1.x.

Is there a way of enforcing this partial fix in phpcs.xml.dist?

I did rebase on my local, instead of force pushing now, attaching a patch with git diff 10.1.x > ~/3268833-26.patch, in case i mess up the MR.

Please don't switch these issues back to patches; they're really much harder to review as such. If you are worried about a force-push, you can try merging HEAD instead, or open a second MR temporarily.

The PHPStan error is:

 ------ --------------------------------------------------------------------- 
  Line   core/modules/system/tests/src/Kernel/System/FloodTest.php            
 ------ --------------------------------------------------------------------- 
  85     Instantiated class Drupal\Tests\system\Kernel\System\MemoryBackend   
         not found.                                                           
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
 ------ --------------------------------------------------------------------- 

I was able to rebase this through the UI without any issues, so hiding the above patch.

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

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Hi, @xjm yes, but you were rebasing against the 10.0.x branch, not 10.1,x which @longwave
asked

Repost here.

or open a second MR temporarily.

Sorry, I could not do it, because 10.1.x is not a valid reference on the current fork. Or please edit the target branch of the MR to 10.1.x.

Thanks!

@jungle You do this by creating a new branch and new MR within the same repository. I did so just now by:

[ayrton:drupal | Fri 14:13:56] $ git checkout -b 3268833-10.1.x 10.1.x
Switched to a new branch '3268833-10.1.x'
[ayrton:drupal | Fri 14:14:04] $ curl https://www.drupal.org/files/issues/2023-02-24/3268833-27.patch | git apply --index -
[ayrton:drupal | Fri 14:14:52] $ git push --set-upstream drupal-3268833 HEAD

...Then clicking the "Open merge request" button for the new branch.

Just a note, I deliberately did not push any commits, because I noticed some bad hunks in the above patch. But the MR is functional, at least. :) Edit: Except I did just accidentally push it, which I am going to force-push away because of said bad hunks.

Updated with force push. Here's how I made this:

1. git checkout -b 3268833-10.1.x 3268833-fix-method-comments
2. Cherry-pick each commit from the 10.0.x MR. There was only one merge conflict:
   

   diff --cc core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
   index 8fa62025af,fa4ffdf6d1..0000000000
   --- a/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
   +++ b/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
   @@@ -466,20 -451,7 +465,24 @@@ class UrlGeneratorTest extends UnitTest
       }
     
       /**
   ++<<<<<<< HEAD
    +   * Tests deprecated methods.
    +   *
    +   * @group legacy
    +   */
    +  public function testDeprecatedMethods() {
    +    $this->expectDeprecation('Drupal\Core\Routing\UrlGenerator::getRouteDebugMessage() is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Use the route name instead. See https://www.drupal.org/node/3172303');
    +    $this->assertSame('test', $this->generator->getRouteDebugMessage('test'));
    +    $this->expectDeprecation('Drupal\Core\Routing\UrlGenerator::supports() is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Only string route names are supported. See https://www.drupal.org/node/3172303');
    +    $this->assertTrue($this->generator->supports('test'));
    +  }
    +
    +  /**
    +   * Tests that the 'scheme' route requirement is respected during URL
    +   * generation.
   ++=======
   +    * Tests the 'scheme' route requirement is respected during url generation.
   ++>>>>>>> b8c71c1c9b ( #2)

   I resolved this by accepting the HEAD version for most of it, but capitalizing "URL" in our new docblock.

3. git push --set-upstream drupal-3268833 HEAD --force

Note that there are several open threads on the previous merge request, so we should verify that those three threads are addressed before marking this RTBC again.

Hiding bot output files.

I reviewed a little bit further, to core/modules/user/tests/src/Functional/UserCreateTest.php.

Note: A quick scan of the lines after those I reviewed revealed that no one has taken my previous suggestion of extrapolating and applying our other standards to later hunks. In general, one-line summaries should:

• Make sense out of context.
• Completely summarize what the method does.
• Be grammatically correct (for example, no skipping small words like "the" or "that").
• Start with a third-person indicative verb (e.g. "Tests").
• Be a complete predicate for an English sentence.

Additionally, function, method, or class names should generally not be referenced in one-line summaries; rather, functions and methods should instead be added to the @see section of the docblock (with fully-qualified namespaces).

Time for another review.

let 1 comment on the thread. Not sure if it should move to NW for that.

If non issue +1 for RTBC

Since it's been a week will go ahead and move this along to the committer.

Setting back to NR because changes where needed to @covers. See the last commit.

Fix for @covers seems fine to me.

Committed and pushed to 10.1.x. Didn't apply cleanly to 10.0.x or 9.5.x, leaving open for possible backport to make other cherry picks easier.

@longwave, thanks!

Patches attached for 9.5.x and 10.0.x. For 9.5.x, there was one conflict, in \Drupal\Tests\user\Kernel\Migrate\d6\MigrateUserRoleTest::assertRoles. For 10.0.x, the patch applied cleanly but I still made a new patch.

Committed and pushed to 10.0.x and 9.5.x, thanks!

The commits and reverts are because I committed 📌 Enable 'Drupal.Commenting.DocComment.ShortSingleLine' coding standard Fixed with the wrong commit message.

quietone → closed merge request !3538

quietone → closed merge request !1962

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