Follow-up to #2676816: Consolidate miscellaneous docs from class @file docblocks into the class docblocks

Problem/Motivation

In #2665992: @file is not required for classes, interfaces and traits, we are implementing a new coding standard to remove redundant @file docblocks from classes. (See #2304909: Relax requirement for @file when using OO Class or Interface per file and #2665744: @file is not required for classes, interfaces and traits.) However, a handful of classes in core have actual documentation of some sort added to the @file docblock.

Proposed resolution

These docs should be merged into the class docblock instead. Move the docs into the class docblock and consolidate them appropriately (e.g. removing redundant information and ordering the sections according to our standards).

Remaining tasks

CommentFileSizeAuthor
#11 2700367-11.patch7.89 KBalexpott
#11 7-11-interdiff.txt1.45 KBalexpott
#7 2700367-7.patch8 KBalexpott
#7 6-7-interdiff.txt1.64 KBalexpott
#6 2700367-6.patch6.36 KBalexpott
#6 2-6-interdiff.txt1.33 KBalexpott
#2 2700367-2.patch5.03 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2700367-2.patch5.03 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Parent issue: #2676816: Consolidate miscellaneous docs from class @file docblocks into the class docblocks » #2665992: @file is not required for classes, interfaces and traits
Related issues: +#2676816: Consolidate miscellaneous docs from class @file docblocks into the class docblocks
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: +rc target

Tagging since this is part of #2665992: @file is not required for classes, interfaces and traits

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2-6-interdiff.txt1.33 KB
new 2700367-6.patch6.36 KB

I improved the way I found them and found 2 more.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 6-7-interdiff.txt1.64 KB
new 2700367-7.patch8 KB

And one final iteration and I've found 2 more.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Interdiffs look good, still RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I've tested this patch by doing the following:

  1. Create a new branch from 8.2.x
  2. Apply latest patch here
  3. Apply latest patch on #2676836: Update proxy class generator for new @file standard
  4. Commit the changes
  5. Run using the latest coder rules (8.x-2.x) run phpcbf ./ --sniffs=Drupal.Commenting.FileComment
  6. Do git diff | grep "^-.*[A-Za-z]" | grep -v "^--- a/core" | grep -v '^- \* Contains \\' | grep -v "^- \* @file\$"

The grep looks for any removed line with a letter on it. Then remove lines like:

  • --- a/core/tests/Drupal/Tests/Core/Plugin/Discovery/TestDerivativeDiscovery.php
  • - * Contains \Drupal\Tests\Core\Plugin\Context\ContextTypedDataTest.
  • - * @file

The resulting output is:

- * contains \Drupal\image\Plugin\migrate\process\d6\ImageCacheActions.
- * Contains Drupal\simpletest\ContentTypeCreationTrait.
- * Contains Drupal\simpletest\NodeCreationTrait
-   * @file
-   * Contains \Drupal\user\Tests\Views\HandlerArgumentUserUidTest.
- * Contains Drupal\Tests\user\Kernel\Migrate\d7\UserMigrationClassTest.
- * @file \Drupal\KernelTests\Core\Cache\CacheCollectorTest.
- * Contains Drupal\Tests\Core\Enhancer\EntityRevisionRouteEnhancerTest.

Which are all acceptable to remove.

jhodgdon’s picture

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

Sorry, but I think this needs a bit of work:

  1. +++ b/core/lib/Drupal/Core/Utility/ProjectInfo.php
@@ -12,6 +10,8 @@
 /**
+ * API for building lists of installed projects.
+ *
  * Performs operations on drupal.org project data.
  */
 class ProjectInfo {

    I'm not sure if the added line belongs at the top of this doc block, or if we need it at all?

    It seems like the ProjectInfo class is not in itself an "API for building lists of installed projects", but instead is part of that API, right?

    I think we should just drop the line. 

  2. +++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkSeparateFormatter.php
@@ -25,6 +20,10 @@
  * )
+ *
+ * @todo https://www.drupal.org/node/1829202 Merge into 'link' formatter once
+ *   there is a #type like 'item' that can render a compound label and content
+ *   outside of a form context.
  */
 class LinkSeparateFormatter extends LinkFormatter {

    This is placed after what looks like some annotation. It should be before the annotation. See

    https://www.drupal.org/node/1354#order

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 7-11-interdiff.txt1.45 KB
new 2700367-11.patch7.89 KB

Thanks for the review @jhodgdon

jhodgdon’s picture

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

Thanks! Looks good to me now.

  • catch committed 7358f7a on 8.2.x 
    Issue #2700367 by alexpott: Consolidate miscellaneous docs from class @...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Status: Fixed » Closed (fixed)

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