Improve code documentation for \Drupal\update\ProjectSecurityData

The parent issue has been fixed on the 8.9.x and 9.0.x branches, so we can un-postpone this issue.

And there's a patch, so I guess it needs review.

Thanks for taking the trouble to improve these comments!

The one useful thing the testbot would tell us about this patch is whether it applies cleanly. It does.

The changes are generally an improvement, but I suggest a few changes:

1. @@ -165,7 +169,31 @@ private function getSecurityCoverageUntilVersion() {
   ...
   +   * - The target minor release where security coverage for the current release
   +   *   should expire. This target release is determined by
   +   *   static::getSecurityCoverageUntilVersion().
       

   I do not feel strongly about it, but I think you can leave off the static::, especially since there is an @see below. (See next comment.)

2. @@ -173,6 +201,8 @@ private function getSecurityCoverageUntilVersion() {
   ...
   +   * @see static::getSecurityCoverageUntilVersion()
       

   I find some existing examples of @see references like this, so maybe this is a new (to me) standard. But the coding standards show fully qualified class names, starting with \Drupal, not static::. Looking at the documentation pages that get generated, such as MenuTreeStorage::loadTreeData, I see that the links are generated correctly, but the link text includes static::, which is less helpful.

3. @@ -181,17 +211,16 @@ private function getAdditionalSecurityCoveredMinors($security_covered_version) {
   ...
   -        // Therefore if we have found an official, published release with the
   -        // same major version as $security_covered_version then this release
   +        // Therefore, if we have found a published official release with the
   +        // same major version as $security_covered_version, then this release
       

   I agree with adding a comma before "then". Also after "Therefore", although I am less sure of myself there. I disagree with removing the comma between the two adjectives "official" and "published".

   Now that I am looking closely at the choice of words, I am not sure that I like "official" anywhere in this documentation. Is it redundant? Does it add anything? (Are my questions redundant?)

4. @@ -181,17 +211,16 @@ private function getAdditionalSecurityCoveredMinors($security_covered_version) {
   ...
   -    // If $latest_minor is set, we know that $latest_minor and
   -    // $security_covered_version_minor have the same major version. Therefore we
   -    // can simply subtract to determine the number of additional minor security
   -    // covered releases.
   +    // If $latest_minor is set, we know that $security_covered_version_minor and
   +    // $latest_minor have the same major version. Therefore, we can subtract to
   +    // determine the number of additional minor security covered releases.
       

   This is an improvement, but I would make the same replacement that you made in the one-line description: "releases with security coverage" instead of "security covered releases".

1. Thanks
2. I second the /shrug response. Thanks for following the written standards.
3. Thanks for clarifying.
4. Thanks.

Yes, #2158497: [policy, no patch] Use "$this" and "static" in documentation for @return types is limited to @return comments. (I noticed that policy in the docs, but had not previously seen the issue.) Maybe we should have a follow-up for @see comments, especially since the links are generated correctly and there are already some examples in core. I searched for @see on that issue: it was discussed and declared out of scope, but there was some support for including it.

The additional comments are a good step forward after #2991207: Drupal core should inform the user of the security coverage for the site's installed minor version including final 8.x LTS releases (already committed on the 8.9.x and 9.0.x branches), so I hope this patch will be accepted. Given the limited lifetime of the 8.8.x branch, I do not think we need to back-port this patch, but it should also be applied to the 9.0.x branch. I just checked, and the patch applies cleanly to that branch.

1. Several docs lines in the patch are 81 characters long, and some are not indented correctly (e.g. the @todo at the top).

2. +++ b/core/modules/update/src/ProjectSecurityData.php
   @@ -148,6 +148,10 @@ public function getCoverageInfo() {
   +   *    on a hard-coded constant (self::CORE_MINORS_WITH_SECURITY_COVERAGE).
   

   Our standard is to use static instead of self for reasons I don't remember.

3. +++ b/core/modules/update/src/ProjectSecurityData.php
   @@ -165,7 +169,31 @@ private function getSecurityCoverageUntilVersion() {
   +   * The Drupal Security Team policy is currently to provide security coverage
   +   * for 2 minor release cycles. Some examples of how this function behaves:
   

   I don't think this documentation belongs here. If anything it belongs on the constant, but this is exactly the kind of thing that we'll miss updating if we change it. If anything, we should link the release cycle overview section on security coverage instead of writing this in the codebase.

1. I checked that the patch in #14 fixes the long lines and indentation.
2. I reviewed the coding standards, and I could not find one that applies, either. We are not using @see here, but #3112830: [policy, no patch] Allow static::methodName() and/or self::methodName() in @see comments when referring to the same class does seem close enough to be relevant.
3. Maybe we should consider rewriting this comment.

What do you think of moving the new @todo comment to the declaration of CORE_MINORS_WITH_SECURITY_COVERAGE? Then we would not need a reference.

I think that part of the difficulty is that getAdditionalSecurityCoveredMinors() does not really describe what the function does. Changing it is out of scope for this issue. Besides, additionalMinorReleasesDuringWhichThisOneReceivesSecurityUpdates() is ridiculous.

What do you think of something like this?

For the sake of example, assume that the currently installed version of Drupal is 8.7.11 and that static::CORE_MINORS_WITH_SECURITY_COVERAGE = 2.

When Drupal 8.9.0 is released, the supported minor versions will be 8.8 and 8.9. At that point, Drupal 8.7 will no longer have security coverage and this function will return 0.

After the release of Drupal 8.8.0 and before the release of 8.9.0, this function will return 1.

Before the release of Drupal 8.8.0, this function will return 2.

Note: callers should not test this function's return value with empty() since 0 is a valid return value that has different meaning than NULL.

Choosing 8.7.11 for an example is a little odd, since we know that this patch (and the parent issue) will not be back-ported to the 8.7.x branch. But it has the advantage that we know the policy in effect when 8.7.11 was released. I do not care strongly, but it might make more sense to use 9.0, 9.1, and 9.2 in the examples.

I would also be happy to move the examples to the doc block of getSecurityCoverageUntilVersion(). If we do that, then the comment might read

For the sake of example, assume that the currently installed version of Drupal is 8.7.11 and that static::CORE_MINORS_WITH_SECURITY_COVERAGE = 2. When Drupal 8.9.0 is released, the supported minor versions will be 8.8 and 8.9. At that point, Drupal 8.7 will no longer have security coverage. Therefore this function returns "8.9.0".

I am updating the issue summary because #2991207: Drupal core should inform the user of the security coverage for the site's installed minor version including final 8.x LTS releases has already been committed (on the 8.9.x branch).

Thanks, I think the version in #17 is an improvement. The only thing I do not like is repeating the text

... the supported minor versions will be 8.8 and 8.9. At that point, Drupal 8.7 will no longer have security coverage ...

I am willing to go along if you think it really helps, but my suggestion is to remove it from the second doc block.

By the way, one of the comments that is tweaked in this issue now reads

        // The releases are ordered with the most recent releases first.
        // Therefore, if we have found a published, official release with the
        // same major version as $security_covered_version, then this release
        // can be used to determine the latest minor.

I looked at the docs for the Update status XML feed, and it says nothing about "most recent releases first". Since we are relying on that, I could just edit that page and hope that it is considered a requirement, but maybe I will post something to the #drupalorg Slack channel instead.

Thanks for the requested changes in #19 and #22. As far as I am concerned, this patch is good to go!

+++ b/core/modules/update/src/ProjectSecurityData.php
@@ -144,6 +150,12 @@ public function getCoverageInfo() {
+   * coverage. Therefore this function would return "8.9.0"

Ubernit: Missing comma after "therefore" and missing final period.

That's trivial enough that I'll fix it on commit:

diff --git a/core/modules/update/src/ProjectSecurityData.php b/core/modules/update/src/ProjectSecurityData.php
index 59b36e1cee..45068978fa 100644
--- a/core/modules/update/src/ProjectSecurityData.php
+++ b/core/modules/update/src/ProjectSecurityData.php
@@ -154,7 +154,7 @@ public function getCoverageInfo() {
    * Drupal is 8.7.11 and that static::CORE_MINORS_WITH_SECURITY_COVERAGE is 2.
    * When Drupal 8.9.0 is released, the supported minor versions will be 8.8
    * and 8.9. At that point, Drupal 8.7 will no longer have security
-   * coverage. Therefore this function would return "8.9.0"
+   * coverage. Therefore, this function would return "8.9.0".
    *
    * @todo In https://www.drupal.org/node/2998285 determine how we will know
    *    what the final minor release of a particular major version will be. This

Committed to 9.1.x, 9.0.x, 8.9.x, and 8.8.x as a documentation improvement. Thanks!

OK, sure. Thanks!