Fix parameter and return type documentation in ComposerIntegrationTest

Here's an attempt.

Thanks for the patch.

1) Looked at a few @return examples in the codebase and found no consistency. Some start with "An array..." or similar mention of "array" while others don't mention array at all. Since it's obvious it's an array, IMO it doesn't need to mention "array" explicitly. Using "list" seems fine as an alternative, or anything that is clear. That said, here's some feedback on the wording:

1. +++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
   @@ -66,6 +66,7 @@ public function testComposerTilde($path) {
       * @return array
   +   *   Paths of all composer.json files present in the Drupal core.
       */
      public function providerTestComposerJson() {
   

   Consider simplifying and using "List of" like the others, e.g.

   List of all composer.json file paths in Drupal core.

2. +++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
   @@ -126,6 +127,7 @@ public function testAllModulesReplaced() {
       * @return array
   +   *   List of scaffold files along with their source path and destination.
       */
      public function providerTestExpectedScaffoldFiles() {
   

   Consider being more explicit about the data in the array, e.g.

   List of scaffold files with destination path, source path, and named destination location.

   Or, an alternative without explicit parameter ordering that is just one line and has more generic wording:

   List of scaffold files with destination and source locations.

3. +++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
   @@ -267,6 +269,8 @@ public function testVendorCleanup($class, $property) {
       * @return array[]
   +   *   List of class properties which contain the packages whose vendor files
   +   *   are to be cleaned up.
       */
      public function providerTestVendorCleanup() {
   

   Consider simplifying, e.g.

   List of class properties that contain vendor package files to clean up.

2) Noticed a couple other things that could be updated in this file while adding these:

a) Missing periods.

   * @param string $destRelPath
   *   Path to scaffold file destination location
   * @param string $sourceRelPath
   *   Path to scaffold file source location
   * @param string $expectedDestination
   *   Named location to the destination path of the scaffold file

b) Missing @params.

  /**
   * Tests the vendor cleanup utilities do not have obsolete packages listed.
   *
   * @dataProvider providerTestVendorCleanup
   */
  public function testVendorCleanup($class, $property) {

3) Marking back to "Needs work" to consider 1 & 2.

Update patch with the changes mentioned in #3.

Thanks for the update. Not sure what happened to the arrays though. Maybe I'm missing something... tired and need sleep :)

1. +++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
   @@ -65,8 +65,8 @@
   +   * @return list
   

   list => array

2. +++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
   @@ -126,8 +126,8 @@
   +   * @return list
   

   list => array

3. +++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
   @@ -268,9 +272,8 @@
   +   * @return list
   

   list => array[]

Hi,
Updated patch as per the changes mentioned in #6.

Thanks. I'll try to review tomorrow.

Thanks for the update.

1) Reviewed the interdiff in #7 and it addresses items in #6.

2) Patch applies cleanly to 9.0 and 9.1 but not 8.9.

[mac:kristen:drupal-8.9.x-dev]$ patch -p1 < 3133901-7.patch
patching file core/tests/Drupal/Tests/ComposerIntegrationTest.php
Hunk #1 succeeded at 52 (offset -14 lines).
Hunk #2 succeeded at 104 (offset -23 lines).
Hunk #3 succeeded at 149 (offset -23 lines).
Hunk #4 FAILED at 226.
Hunk #5 FAILED at 250.
2 out of 5 hunks FAILED -- saving rejects to file core/tests/Drupal/Tests/ComposerIntegrationTest.php.rej
[mac:kristen:drupal-8.9.x-dev]$ cd90
[mac:kristen:drupal-9.0.x-dev]$ patch -p1 < 3133901-7.patch
patching file core/tests/Drupal/Tests/ComposerIntegrationTest.php
[mac:kristen:drupal-9.0.x-dev]$ cd91
[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < 3133901-7.patch
patching file core/tests/Drupal/Tests/ComposerIntegrationTest.php

3) Reviewed the code again and have one question.

+   * @param mixed $class
+   *   Name of the class where the property is defined.

Not sure why "mixed" is use. I do see it used in a few places but it's mostly string:

[mac:kristen:drupal-9.1.x-dev]$ grep -r "@param mixed \$class" . | wc
       4      20     279
[mac:kristen:drupal-9.1.x-dev]$ grep -r "@param string \$class" . | wc
      52     260    4466

e.g.

[mac:kristen:drupal-9.1.x-dev]$ grep -r "@param mixed \$class" .
./core/tests/Drupal/Tests/ComposerIntegrationTest.php:   * @param mixed $class
./core/lib/Drupal/Core/Entity/EntityTypeManagerInterface.php:   * @param mixed $class
./core/lib/Drupal/Component/Utility/Html.php:   * @param mixed $class

Hi @Kristen Pol,
Thanks for the review. I have used 'mixed' for $class as I got the reference from the php.net official website, which is using the 'mixed' datatype for $class in ReflectionProperty class here.

Thanks for the explanation. Looks RTBC to me and patch applied to 9.0 and 9.1. Not sure if this would be backported.

Thanks for working on this -- always good to see our docs improved.

Usually, we like to see coding standards cleanups across all of core for a single rule, but for issues that add new parameter and return documentation, it makes sense to work just on one API (since one has to read and understand the code in order to document it). Therefore, I think the scope of this issue is correct.

1. +++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
   @@ -66,6 +66,7 @@ public function testComposerTilde($path) {
       * @return array
   
   @@ -126,6 +127,7 @@ public function testAllModulesReplaced() {
       * @return array
   

   If we're correcting parameter and return type docs for this class, we should also make the return doc type more specific than array. (Is it a string? Etc.)

2. +++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
   @@ -247,6 +249,10 @@ protected static function getContentHash($composerFileContents)
       * @dataProvider providerTestVendorCleanup
   +   * @param mixed $class
   +   *   Name of the class where the property is defined.
   

   There should be a blank line before the start of the @param section.

   Also, "Name of the class" sounds like a string. Is it the name of the class, or is it the class? (Looking at the code, I think it's the class).

   mixed is indeed very rarely used, but in this case seems accurate. Thanks @mrinalini9 for providing the reference there!

Moving to 8.8.x since documentation improvements are eligible for backport to the production branch. Thanks again!

+++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
@@ -267,6 +273,7 @@ public function testVendorCleanup($class, $property) {
    * @return array[]
+   *   List of class properties that contain vendor package files to clean up.

Oh also, I think this isn't just a list. (If it were, it'd be string[][] or such. It's an array of arrays where the first element is the class and the second element is the property name. (I think). Also, it's not a list, it's an array of test cases.

So let's consider a little more what the docs should say here.

Patch for 8.8.x, please review.

For the above query:
1.

If we're correcting parameter and return type docs for this class, we should also make the return doc type more specific than an array. (Is it a string? Etc.)

For example:
providerTestComposerJson(): This function returns $data which is an array and it will contain the list of all composer.json file paths which I do have mentioned in the code doc. So, I think this should be fine.

Regarding #14, I was thinking of a list in a general sense not a "type" sense. Here are some examples already in use so these might help us reword the comments:

core/modules/user/src/PermissionHandler.php

   * @return array[]
   *   Each return permission is an array with the following keys:
   *   - title: The title of the permission.
   *   - description: The description of the permission, defaults to NULL.
   *   - provider: The provider of the permission.

core/tests/Drupal/Tests/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumperTest.php

     * @return array[]
     *   Returns data-set elements with:
     *     - aliases as returned by ContainerBuilder.
     *     - aliases as expected in the container definition.

core/lib/Drupal/Core/Test/JUnitConverter.php

   * @return array[]
   *   The results as array of rows in a format that can be inserted into the
   *   {simpletest} table of the results database.

Replace @return array to @return array[] as per suggestion in #14 and #19

To be clear, I didn't suggest changing the type in 19, I was simply providing some sample comments so we can figure out better documentation. Maybe the types should be changed but I haven't thought about that yet.

Note that patch still applies cleanly to 9.1.x.

I have tested , this document seems to be fine from my end and have added test for the 9.1.x .

RTBC + 1 from my end as well on 9.1

If we are fine with @return array[] then this looks good.

Actually, #14 and #19 have not been addressed. Reiterating: the current documentation is inaccurate -- it is not just a "list" -- and it is in scope for this issue to fix it.

#13.2 (second part beginning with "Also...") has not been addressed either.

This has been fixed in #2721309: Fix Drupal.Commenting.FunctionComment.ParamCommentFullStop and #3504774: Fix Drupal.Commenting.FunctionComment.MissingReturnComment in core/tests,

The issue credits should be transferred over then :)

Also, the correct issue status for that is closed dupe. :)

@xjm closed issues get credit now so if they’re checked here they’ll get something.

If we move over then we should uncheck here too else we are giving double

Uploading a screenshot to show.

Whoa! Did not know that. Thanks @smustgrave!