Follow-up to #1392754: Comply with new documentation standards for @file for namespaced class files, some parts of the @file docblock cleanup during rc have been addressed in #2598840: Bad @file description in Test files classes & #2600304: Bad @file description in core/ classes

Problem/Motivation

In the @file docblock, some descriptions are malformed due to incorrect namespaces or class names. For example :

/**
 * @file
 * Contains \Drupal\Core\Command\DbDumpCommand.
 */

namespace Drupal\Core\Command;

...
class DbImportCommand

The class is DbImportCommand in this case, not DbDumpCommand

OR 

/**
 * @file
 * Contains \Drupal\system\Tests\PhpStorage\PhpStorageFactoryTest.
 */

namespace Drupal\Tests\system\Kernel\PhpStorage;

...
class PhpStorageFactoryTest extends KernelTestBase {

The namespace in the definition is Drupal\system\Tests\PhpStorage where the declared namespace is Drupal\Tests\system\Kernel\PhpStorage;

Proposed resolution

  • Resolve incorrect namespaces or class names in the @file docblock definitions

I don't think there is a simple grep to find these, will attempt to repurpose an old bash script from the original issue.
First attempt a script to find these, feel free to improve: https://gist.github.com/justAChris/acb1479407b7c0e5d1aa

This should be RC Eligible since this only cleans up documentation only

CommentFileSizeAuthor
#19 interdiff-2600538-6-7.txt440 bytesrakesh.gectcr
#19 2600538-7.patch17.74 KBrakesh.gectcr
#15 interdiff-2600538-4-6.txt541 bytesrakesh.gectcr
#15 interdiff-2600538-5-6.txt419 bytesrakesh.gectcr
#15 2600538-6.patch17.14 KBrakesh.gectcr
#11 interdiff-2600538-4-5.txt603 bytesrakesh.gectcr
#11 2600538-5.patch17.74 KBrakesh.gectcr
#7 interdiff-2600538-3_0-4.txt1.89 KBrakesh.gectcr
#7 2600538-4.patch17.86 KBrakesh.gectcr
#5 2600538-3.patch19.77 KBanil280988
#3 2600538-3.patch19.82 KBanil280988
#2 2600538-incorrectFiles.txt2.89 KBjustachris

Comments

justAChris created an issue. See original summary.

justachris’s picture

justachris commented
Issue summary: View changes
StatusFileSize
new 2600538-incorrectFiles.txt2.89 KB

Script to parse files to find incorrect namespace or classname in the @file docblock:
https://gist.github.com/justAChris/acb1479407b7c0e5d1aa

Produces the attached list of files. Ignoring core/lib/Drupal/Component/Assertion/Handle.php, since the script incorrectly gets the classname and core/lib/Drupal/Core/Archiver/ArchiveTar.php because that file is a bit of a mess (missing docblock, etc..)

anil280988’s picture

anil280988 commented
Status: Active » Needs review
StatusFileSize
new 2600538-3.patch19.82 KB

Created Patch

Status: Needs review » Needs work

The last submitted patch, 3: 2600538-3.patch, failed testing.

anil280988’s picture

anil280988 commented
Status: Needs work » Needs review
StatusFileSize
new 2600538-3.patch19.77 KB

Re-created the patch

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Thanks! Most of this looks fine. But there are a few problems:

  1. +++ b/core/modules/simpletest/tests/fixtures/simpletest_phpunit_run_command_test.php
@@ -1,5 +1,10 @@

@@ -1,5 +1,10 @@
 <?php
 
+/**
+ * @file
+ * Contains \Drupal\Tests\simpletest\Unit\SimpletestPhpunitRunCommandTestWillDie.
+ */
+

    In this case, I do not think that the standard @file header for PSR-4 classes is appropriate.

    Let's just take this out of the patch. This file deserves a better @file block. It should contain the stuff that is currently in the test class docs I think.

    In any case, the standard of making @file this way is only really meant for standard PSR-4 class files. 

  2. +++ b/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
@@ -2,7 +2,7 @@

@@ -2,7 +2,7 @@
 
 /**
  * @file
- * Contains \Drupal\system\Tests\Extension\ModuleHandlerTest.
+ * Contains \Drupal\Tests\system\Extension\ModuleHandlerTest.
  */
 
 namespace Drupal\Tests\system\Kernel\Extension;

    Huh, what??? This one doesn't seem right... 

  3. +++ b/core/modules/system/tests/src/Kernel/PhpStorage/PhpStorageFactoryTest.php
@@ -5,7 +5,7 @@

@@ -5,7 +5,7 @@
  * Contains \Drupal\system\Tests\PhpStorage\PhpStorageFactoryTest.
  */
 
-namespace Drupal\Tests\system\Kernel\PhpStorage;
+namespace Drupal\system\Tests\PhpStorage;

    This namespace line change seems to be out of scope for the issue. Also it doesn't look correct to me. Remove from the patch. 

  4. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/ProxyServicesPassTest.php
@@ -2,11 +2,10 @@
-namespace Drupal\Tests\Core\DependencyInjection\Compiler;
+namespace Drupal\Tests\Core\DependencyInjection\Compiler\Compiler;

    Out of scope for this issue. Also incorrect as far as I can tell. The namespace doesn't have Compiler in there twice. Remove from the patch.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
StatusFileSize
new 2600538-4.patch17.86 KB
new interdiff-2600538-3_0-4.txt1.89 KB

@jhodgdon

I have done the changes you mentioned.

@anil280988

Be careful while you changing the name space, It can effect entire system too.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
Status: Needs work » Needs review

The last submitted patch, 3: 2600538-3.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Thanks, but:

+++ b/core/modules/ban/tests/src/Unit/Plugin/migrate/source/d7/BlockedIpsTest.php
@@ -2,10 +2,10 @@
-namespace Drupal\Tests\ban\Unit\Plugin\migrate\source\d7;
+namespace Drupal\ban\Tests\Unit\Plugin\migrate\source\d7;

Do not change namespaces in this patch!

If you think they are wrong (and this one looks wrong), please file a separate issue. This issue is in the "documentation" component and should only change documentation lines.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
StatusFileSize
new 2600538-5.patch17.74 KB
new interdiff-2600538-4-5.txt603 bytes

@jhodgdon

Sorry it was mistake from my side, i did the changes you addressed. I could have been checked patch. Any way I changed that.

about the namespace i will do the home work , Then its necessary i will create the new issue.

Please find the patch and interdiff files

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
Status: Needs work » Needs review

The last submitted patch, 3: 2600538-3.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Thanks! But now:

+++ b/core/modules/ban/tests/src/Unit/Plugin/migrate/source/d7/BlockedIpsTest.php
@@ -2,7 +2,7 @@
- * Contains \Drupal\Tests\ban\Unit\Plugin\migrate\source\d7\BlockedIps.
+ * Contains \Drupal\ban\Tests\Unit\Plugin\migrate\source\d7\BlockedIps.
  */
 
 namespace Drupal\Tests\ban\Unit\Plugin\migrate\source\d7;

The change here does not agree with the namespace line below. Let's leave this file alone and file a separate issue to fix the docs and namespace together. Put that into the "ban module" component.

Thanks!

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
StatusFileSize
new 2600538-6.patch17.14 KB
new interdiff-2600538-5-6.txt419 bytes
new interdiff-2600538-4-6.txt541 bytes

@jhodgdon
I have done the changes you mentioned above

I have created a separate issue to fix the docs and namespace together, the issue link is following
https://www.drupal.org/node/2601780

Thank You!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Great, thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Whoops. I went over to #2601780: Wrong name space and @file description in /core/modules/ban/ and investigated more closely.

It looks like core/modules/ban/tests/src/Unit/Plugin/migrate/source/d7/BlockedIpsTest.php:
- Does actually have the correct namespace line
- Does not have the right Contains line at the top, because the class name is BlockedIpsTest not BlockedIps.

 * Contains \Drupal\Tests\ban\Unit\Plugin\migrate\source\d7\BlockedIps.

So can we add the change from BlockedIps to BlockedIpsTest at the end of this line, to this patch?

The last submitted patch, 3: 2600538-3.patch, failed testing.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
StatusFileSize
new 2600538-7.patch17.74 KB
new interdiff-2600538-6-7.txt440 bytes

@jhodgdon

I applied the changes you have mentioned.

Thank you!

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
Status: Needs work » Needs review

The last submitted patch, 3: 2600538-3.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 0a8d97e and pushed to 8.0.x. Thanks!

  • alexpott committed 0a8d97e on 8.0.x 
    Issue #2600538 by rakesh.gectcr, anil280988, justAChris, jhodgdon:...

The last submitted patch, 3: 2600538-3.patch, failed testing.

Status: Fixed » Closed (fixed)

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