Problem/Motivation

Suggested by @jhodgdon here: #1816782-9: Add missing type hinting to Field_ui module docblocks

Various @see annotations are structurally incorrect, with trailing punctuation and extra comments.

Drupal coding standards for @see: https://www.drupal.org/coding-standards/docs#see

Proposed resolution

Fix all @see annotations so they are correct. Emphasize coding standards correctness over changing content.

Use a phpcs command similar to this:

phpcs --standard="Drupal" --extensions="module/php,inc/php,php" --report-csv --ignore=vendor/ PATH/TO/REVIEW | grep -F @see

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#21 2662208-21.patch21.61 KBsuresh prabhu parkala
#20 2662208-20.patch21.61 KBsuresh prabhu parkala
#16 Screenshot 2020-05-14 at 3.46.14 PM.png76.75 KBhimanshu_sindhwani
#2 2662208_2.patch46.94 KBmile23

Comments

Mile23 created an issue. See original summary.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review
StatusFileSize
new 2662208_2.patch46.94 KB

Here's a patch of (mostly) everything but core/modules/. Only skipped because it's close to dinner time. :-)

Some stuff:

There are a few places where a @see was used as a substitute for describing @params, like this:

   * @param mixed $time
   *   @see __construct()
   * @param mixed $timezone
   *   @see __construct()

In these sorts of cases, there are now no @param descriptions (because there really weren't to begin with), and the @see is moved to its proper place in the docblock.

Also, places where I was already working on the file, and I saw that @see only referred to a class-local method, I expanded the reference to include the class, like this:

-   * @see parseArithmetic()
+   * @see \Drupal\Component\Gettext::parseArithmetic()

It's a coin toss as to whether @see will come after or before @defgroup or other such directives. I concentrated on having adequate spacing around these (blank lines), rather than order, since there's no rule for it in phpcs.

The main problem in reviewing this patch will be how much memory phpcs has available on your machine. :-)

I used a phpcs script like this:

phpcs --standard="Drupal" --extensions="module/php,inc/php,php" --report-csv --ignore=vendor/ PATH/TO/REVIEW | grep -F @see
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
jhodgdon’s picture

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

Thanks for filing this issue and starting on a patch!

As a note, there is actually a standard for the order of @tags in documentation blocks:
https://www.drupal.org/node/1354#order
I do not know if it has been added to the Drupal sniffer, but we do have a documented standard.

I also took a quick look through the patch, and there are some things in there I think are not really optimal:

  1. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -177,11 +178,8 @@ public static function createFromTimestamp($timestamp, $timezone = NULL, $settin
    *   date will be created using the createFromFormat() method.
-   *   @see http://us3.php.net/manual/en/datetime.createfromformat.php

    Here, the correct fix should really be that @see is changed to the word "See" and the reference/link is left where it is. Moving the @see to the bottom of the doc block loses the context that this was intended for. Whoever created this doc block should not have put @see there but should have just incorporated the information into the docs directly. 

  2. +++ b/core/lib/Drupal/Component/Gettext/PoHeader.php
@@ -478,6 +478,7 @@ private function tokenizeFormula($formula) {
    * @see parseArithmetic()
+   *
    * @throws Exception

    This kind of fix seems to be to be out of scope for the issue. 

  3. +++ b/core/lib/Drupal/Component/Plugin/Exception/InvalidPluginDefinitionException.php
@@ -25,7 +25,7 @@ class InvalidPluginDefinitionException extends PluginException {
-   * @see \Exception for the remaining parameters.
+   * @see \Exception

    Again, here I think this was not a very good fix for the docs. We lost information. Better fix is to change @see to "See".

    I realize a script cannot make this kind of judgement call probably, but if these kinds of fixes are in the patch I do not think we should commit it.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

I think we might define scope here as anything you find when you run the phpcs command: phpcs --standard="Drupal" --extensions="module/php,inc/php,php" --report-csv --ignore=vendor/ PATH/TO/REVIEW | grep -F @see

That way the patch is easier to generate and is also easier to review.

So therefore #4.2 is in scope, and tag ordering isn't (since we don't have sniffer rules about it for @see).

1 and 3 are a bit ambiguous, yah, so thanks for the ruling. :-)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Note that the review in #4 -- I didn't look through the whole patch for this type of problem mentioned in items 1/3. There are probably more. People use @see quite a bit when they shouldn't be doing it. Probably any time it's scrunched into a paragraph, it should be replaced with the word "See", and if it's kind of off by itself, it should be moved to the bottom and fixed up. Maybe a script could figure that out, or at least make decisions closer to items 1/3?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

David_Rothstein’s picture

David_Rothstein commented
Related issues: +#2940420: @see should not be used inside a @param description

Here is an issue that focuses specifically on the @see inside @param problem and that might have some overlap with this one.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Version: 8.6.x-dev » 8.9.x-dev

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

himanshu_sindhwani’s picture

himanshu_sindhwani commented
StatusFileSize
new Screenshot 2020-05-14 at 3.46.14 PM.png76.75 KB

This issue seems to be fixed in Drupal version 9.1.x-dev . I ran the above command mention in issue summary and it displays no results. Then I manually checked into some files mention in patch #2 and couldn't find the issue in this version.

output

himanshu_sindhwani’s picture

himanshu_sindhwani commented
Status: Needs work » Needs review

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

sulfikar_s’s picture

sulfikar_s commented
Status: Needs review » Needs work

The patch in #2 needs reroll in 9.2.x-dev. As said by @himanshu_sindhwani some of the areas are already covered in 9.2.x-dev, but some files still needs work!

So changing the status to Needs work!

suresh prabhu parkala’s picture

suresh prabhu parkala commented
Status: Needs work » Needs review
StatusFileSize
new 2662208-20.patch21.61 KB

Patch re-rolled against 9.2.x. Please review!

suresh prabhu parkala’s picture

suresh prabhu parkala commented
StatusFileSize
new 2662208-21.patch21.61 KB

Sorry for the noise. Applied new patch please review.

suresh prabhu parkala’s picture

suresh prabhu parkala commented

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Issue tags: +Coding standards

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Status: Needs review » Closed (duplicate)

I'm closing this as a duplicate of a later issue, #3102478: [PP-1] @see tags don't use fully qualified class/interface names, which has more discussion. Transferring credit over there.

Thanks for working on this.