Drupal core should inform the user of the security coverage for the site's installed minor version including final 8.x LTS releases

Where does the release schedule live? It seems to me like a bad idea to hard-code future release dates. Are the future dates in (or can they be added to) updates.drupal.org? Or the server that supplies composer package information?

Unless the answer is that the future dates are already available from some such source, I think for this issue we should settle for links to one or both of

• https://www.drupal.org/docs/8/understanding-drupal-version-numbers/when-...
• https://www.drupal.org/core/release-cycle-overview

or other documentation pages.

Here is a screenshot of (part of) the status report:

There is room in the "General System Information" section for more information. Some suggestions for what to put there, probably no more than two items:

• How long this version will be fully supported (fixes for bugs and security issues)
• How long this version will be supported (fixes for security issues only)
• Link to release notes for the latest stable version
• Link to a documentation page, such as one of those listed above

For this issue, "how long" may be expressed as "until version 8.N is released".

We already have the update status provided by the update module (if it is enabled). I guess we do not want to add to that: it is already complicated (wow!) and the update module may not be enabled. So we should be thinking of adding an additional status message, probably from the system module.

To address Question 5.1, I suggest thinking in terms of the severity levels defined by hook_requirements:

• REQUIREMENT_ERROR
• REQUIREMENT_WARNING
• REQUIREMENT_OK
• REQUIREMENT_INFO

Error: the installed version is insecure or does not receive security coverage.

Warning: the installed version is secure, but only receives security fixes

Info: the installed version is secure, and it receives bug fixes and security fixes

I guess what I am saying is that we do not have to convey the difference between "receives security updates" and "is secure". Instead, we should convey the recommended action and the urgency of that recommendation. The urgency is communicated by the severity and by whatever schedule we can communicate in the status message.

The API updates.drupal.org exposes for #2766491: Update status should indicate whether installed contributed projects receive security coverage already has per-release data. Since alpha/beta/etc of an otherwise-covered project are not covered, and rules can always change in the long run.

It looks like that API needs some work to support core:

<release>
  <name>drupal 8.1.10</name>
  <version>8.1.10</version>
  …
  <security covered="1">Covered by Drupal's security advisory policy</security>
</release>

(Those changes would be implemented in a non-core-specific way, so the same rules apply to semantically-versioned contrib.)

See also #2687957: Improve Update Status messaging for End of Life in Drupal 7(and Drupal 8 and later)

Since this issue is about displaying if and for how long a site's minor version of Drupal will be supported not individual releases I think we will need something outside the <release> tag in the update XML.

Right now I don't think we have any meta information about minors just majors, recommend and supported, and individual releases, supported , covered, etc.

It seems like from the "Proposed resolution" that we would need either.

1. Have information in the drupal.org XML that tells how long the minor will be supported.

   Something like

   <recommended_major>8</recommended_major>
   <supported_majors>8</supported_majors>
   <default_major>8</default_major>
   <minors>
     <minor>
       <name>8.7</name>
       <major_version>8</major_version>
       <minor_version>7</minor_version>
       <security covered='1' end_date='223456'>This minor version is covered</security>
     </minor>
     <minor>
       <name>8.6</name>
       <major_version>8</major_version>
       <minor_version>6</minor_version>
      <security covered='1' end_date='123456'>This minor version is covered</security>
     </minor>
     <minor>
       <name>8.5</name>
       <major_version>8</major_version>
       <minor_version>6</minor_version>
      <security covered='0' end_date='023456'>This minor version is not covered</security>
     </minor>
   </minors>
   

   This seems like it be very hard to do in generic because I don't think the drupal security team covers specific minors of contrib projects.

2. Or hard code estimate end dates for core minors in to Drupal core directly.

   I don't know what the timeframe for #2909665: [plan] Extend security support to cover the previous minor version of Drupal is but hard coding be the only way if the goal is sooner than we could get the changes to drupal.org done

Here is an attempt to add "Security coverage" message for Drupal core.

The current message if a site was on 8.5.6 would be

The current minor version of Drupal core, 8.5, will be supported until the release of 8.6.0. 8.6.0 is currently in the pre-release stage. The latest pre-release is 8.6.0-rc1.

I added the bit about the pre-release to alert the user that version they will have to update by is coming soon. If no pre-release is available the the message would just be the first sentence.

If the site currently needs a security update I don't put any message about coverage because it seemed to confusing to have both messages.

1. Fix the errors in #11
2. Add an error class if in pre-release for the release when security coverage will drop for existing version
3. Added coverage_message to test cases for core

This is looking good.

I note however that we already have https://github.com/composer/semver in our dependencies, so we might be able to use a lot of its utilities for parsing version numbers etc, instead of creating our own.

1. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,249 @@
   +      throw new \Exception("@todo New version of Drupal, handle it!");
   

   This still to be done?

2. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,249 @@
   +    elseif ((int) $latest_full_release['version_major'] === $security_supported_release_info['version_major']) {
   

   no need for elseif if previous hunk throws an exception, if will do

3. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,249 @@
   +    $support_until_release['version'] = $support_until_release['version_major'] . '.' . $support_until_release['version_minor'] . '.' . $support_until_release['version_patch'];
   

   could use implode('.', $support_until_release)?

4. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,249 @@
   +    $pre_release_types = ['alpha', 'rc', 'beta'];
   

   should these strings be constants?

Here's a straight re-roll of #16.

I'm not sure how to address #17 - with the way this is written, any project could provide a security coverage message, so to me showing these on the site status report could be quite verbose. If we want this, I'm not sure where it should go - do we implement hook_requirements for that?

I'm going to move forward assuming this is meant for core only, and address the other review as well.

Addressed #17 and #18 (except #18.1, I don't know the answer to that), need more time on #19.

FYI - There are a few formatting/markup bugs here that will be addressed in the next patch.

Addressed #19, still need test coverage for unsupported minors. Also, here are screenshots of what the info/error message looks like now.

Re-roll.

I removed all the pre-release code per #19, but I'm wondering if we want to check if "version_extra" is not empty and not "rc", and if so do not show users the message about their minor being supported (the info message). Another way to say this is that alpha and beta users should not be led to believe that they have security coverage.

Also changing test coverage to use a fixture that's easy to use, since these new status messages only show up if there are no security updates.

1. +++ b/core/modules/update/tests/src/Functional/UpdateTestBase.php
   @@ -165,6 +167,11 @@ protected function assertSecurityUpdates($project_path_part, array $expected_sec
   +    if ($coverage_message) {
   +      $this->drupalGet('admin/reports/status');
   +      $this->assertSession()->pageTextContains($coverage_message);
   +    }
   

   Should we always be checking admin/reports/status even if $coverage_message is empty. If we explicitly don't want to to show a message about coverage if core is not currently secure could check to make sure "Drupal core security coverage" doesn't appear.

2. +++ b/core/modules/update/update.compare.inc
   @@ -425,6 +427,17 @@ function update_calculate_project_update_status(&$project_data, $available) {
   +  if (!empty($project_data['security updates'])) {
   +    // If we found security updates, that always trumps any other status.
   +    $project_data['status'] = UPDATE_NOT_SECURE;
   +  }
   

   Was this added back intentionally in #28?

   This was removed in #2804155: If the next minor version of core has a security release, status still says "Security update required!" even if the site is on an equivalent, secure release already because it was determined we could rely on Drupal insecure tag.

   Since #28 was labeled as re-rolled I don't think it was intentional

#32.2 was not intentional, no.

Also wanted to note that the messages added in this patch only shows up if there _isn't_ a security update, that was ported over from the previous logic of the patch.

1. I talked with @xjm who mentioned the assertion for coverage on /admins/reports/status should be in it's own test. I am not doing that in this patch to minimize changes while I address another issue.

2. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -311,8 +326,23 @@ public function securityUpdateAvailabilityProvider() {
   +      '1.0, supported' => [
   +        'site_patch_version' => '1.0',
   +        'expected_security_releases' => [],
   +        'expected_update_message_type' => static::UPDATE_NONE,
   +        'coverage_message' => 'The installed minor version of Drupal, 8.1, will receive security fixes until the release of 8.2.0.',
   +        'fixture' => 'sec.2.0',
   +      ],
   

   This is should actually have unsupported coverage message because in the fixture 8.2.0 is already released so 8.1.0 is out of coverage(I assume we are not accounting for the small window of overlap?)

3. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,190 @@
   +      'version_minor' => ((int) $existing_release['version_minor']) + static::CORE_MINORS_SUPPORTED + 1,
   

   We don't need the +1 here. I think this is why test case that should have been "unsupported" was passing as supported.

4. re #36

   If you are on 8.3.0, you get the error telling you to update to continue receiving security fixes. I'm not sure of the value of the second paragraph about the release dates there.

   Are you talking about the

   Visit the release cycle overview for more information on supported releases.

   paragraph. I still think this is good even if you are out of security coverage. Because it explains why your version is not going to receive updates. So you can keep up to date next time.

5. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -167,15 +167,17 @@ public function testMajorUpdateAvailable() {
   +   * @param string $coverage_message
   +   *   The expected coverage message.
   

   Changing this to an array of message so we can check for the "Visit the release cycle overview for ...." text. I don't think \Behat\Mink\WebAssert::pageTextContains() works for text split into paragraphs.

1. +++ b/core/modules/update/update.install
   @@ -105,6 +107,22 @@ function _update_requirement_check($project, $type) {
   +  // Return early if there isn't a core update and there is a coverage message.
   +  if ($type == 'core' && $status !== UpdateManagerInterface::NOT_SECURE && !empty($project['security_coverage_info'])) {
   +    if ($security_coverage_message = UpdateHelper::getSecurityCoverageMessage($project)) {
   +      $requirement['description'] = $security_coverage_message;
   

   One of the test failures was coming from _update_cron_notify because it calls update_requirements() directly and it was looking for a reason.

   I don't think we should place our new requirement in this function because it is made to return only 1 requirement so would have to change existing functionality. Because there is so little testing for this I think we could break something we missed.

   For instance _update_cron_notify sends out emails based on update_core requirement and we would have been not been changing the requirement in some cases.

   It is very likely that site admins have automated processes that handle these emails about needing updates.

   So I think we should leave _update_requirement_check the way it is and add \Drupal\update\UpdateHelper::getSecurityCoverageRequirement() and have a new requirement index.

2. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,190 @@
   +  const CORE_MINORS_SUPPORTED = 1;
   

   I know this issue is part of #2909665: [plan] Extend security support to cover the previous minor version of Drupal but I am assuming there is other work to be done before this is fully implemented. If we change this to 2 now committing this patch would effectively be implementing the new policy.

   Currently we are on stage 5 of 6 in #2909665.

   If we commit this patch with CORE_MINORS_SUPPORTED = 1 then we everything else is complete. we can update it to 2.

   Should we make a follow up to set it to 2 or do that now.

This patch does

1. Moves the testing coverage for message to its own test method.
2. Uses an existing fixture file for the test and removes the new one that was added for this issue
3. Checks to make sure there is no message if we are on pre-release or dev version

Re @samuel.mortenson on #30

Also changing test coverage to use a fixture that's easy to use, since these new status messages only show up if there are no security updates.

This is not longer true since I changed it to be its own requirement. See #39.1

We could still not show the message but I think it makes sense to show it in either case.
For instance if there are there is security release for you current minor and the next one. If we don't give you the message that your current minor will still be security updates you may think this is either the last 1 or an exception(which we have done right?). That would influence about whether you would pick the current minor security release or the next minor's.

1. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,222 @@
   +          'The installed minor version of %project, %version, is no longer supported and will not receive security updates. <a href=":update_status_report">Update to the next minor or higher soon</a> to continue receiving security fixes.',
   

   Changing the second sentence to remove "next minor to

   Update to a supported minor soon to continue receiving security fixes.

   which will have a link to the update status page.

   The site could be multiple minors behind so "next minor" may still result in an unsupported minor.

2. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -344,6 +344,69 @@ public function securityUpdateAvailabilityProvider() {
   +    if ($coverage_messages) {
   +      foreach ($coverage_messages as $coverage_message) {
   

   This if is left over from when this was in \Drupal\Tests\update\Functional\UpdateTestBase::assertSecurityUpdates we don't need it anymore. it will always be an array, though it may be empty.

3. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -344,6 +344,69 @@ public function securityUpdateAvailabilityProvider() {
   +    else {
   +      $assert_session->pageTextNotContains('Drupal core security coverage');
   +    }
   

   Since we have conditional logic to display the sentence about updating to the next minor I think we need to check that the sentence is not there when it should not be.

   Adding $not_contains_messages parameter to the test method to check for these. Which means we can remove this else and just check that no $not_contains_messages show.

4. re #42.1 changed CORE_MINORS_SUPPORTED to 2

1. +++ b/core/modules/update/tests/modules/update_test/drupal.sec.2.0.xml
   @@ -0,0 +1,133 @@
   +            <name>Drupal 8.3.0</name>
   +            <version>8.3.0</version>
   +            <tag>DRUPAL-8-3-0</tag>
   +            <version_major>8</version_major>
   +            <version_minor>2</version_minor>
   +            <version_patch>0</version_patch>
   

   This release was added by mistake we don't need 8.3.0 in the fixture. The only reason the tests weren't failing is because the actual version_minor value was 2 not 3.

1. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,237 @@
   +      $info['supported_until'] = $support_until_release;
   

   After we get the supported release we should be checking if the next major version is available and the supported release has not been. If so we can't provide a good coverage message so return empty.

2. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,237 @@
   +      if (static::isRecommendableRelease($release)) {
   

   We should not be checking isRecomendableRelease() which check if it is unsupported or insecure.

   We are trying to find the full release to determine if the "supported till" release is already released. It may have been release but is not insecure. that would still mean the installed version is not covered.

1. Update \Drupal\update\UpdateHelper::getSecurityCoverageMessage() to always split the message into paragraphs. This required test changes so it good because our previous tests would fail.
2. Addressed the problem of 8.4.x showing it was not under security coverage. This was because \Drupal\update\UpdateHelper::getMostRecentFullRelease() was not checking for version_extra being empty so it was picking up the 8.6.0-rc1.
   So with this patch
   
   When I ran this 8.6.0 hadn't come out yet.

   I updated the test feature to include rc release to emulate this.

1. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,267 @@
   +  const CORE_MINORS_SUPPORTED = 2;
   

   I think this is supposed to have a docblock

2. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,267 @@
   +                '%next_minor' => $support_until_release['version_major'] . '.' . $support_until_release['version_minor'],
   
   

   this will always return the future release ie 8.7. Since the future release won't be available, we want to show the current release that the user should update to. This is the bug identified in #45

Here is a screenshot of the results after the patch. I am running 8.5.5 and it is notifying me to update to 8.6 rather than 8.7.

I added four follow-up issues. See the updated issue summary.

helps if you update the patch :/. interdiff is the same from above

Here are some screenshots using the patch from #57.

The patch did not apply cleanly to Drupal 8.4.8 nor 8.5.7. I removed the diffs to one of the test files, and I had to apply one hunk manually. For the record, this one:

diff --git a/core/modules/update/update.compare.inc b/core/modules/update/update.compare.inc
index dd658120ef..380d00533a 100644
--- a/core/modules/update/update.compare.inc
+++ b/core/modules/update/update.compare.inc
@@ -425,6 +427,8 @@ function update_calculate_project_update_status(&$project_data, $available) {
     $project_data['recommended'] = $project_data['latest_version'];
   }
 
+  $project_data['security_coverage_info'] = UpdateHelper::getSecurityCoverageInfo($project_data, $available['releases']);
+
   if (isset($project_data['status'])) {
     // If we already know the status, we're done.
     return;

I made these screenshots a few days after the release of 8.6.0.

Good news: Drupal is no longer telling me to upgrade to 8.7.0. Thanks, @robpowell!

My screenshots include the "Drupal Core update status" message, also from the Update Manager module. (One screenshot also includes the Trusted Host Settings error. Sorry about that.) Looking at these together, I think there is some duplicated information.

1. In the warning message (8.5.7), "Update to 8.6 or higher soon" links to /admin/reports/updates/update, same as the link "available updates" in the next message. I do not like having two links to the same page, with different link text. (Although same text, different destinations would be even worse.) Maybe make "Update to 8.6 or higher soon" plain text, not a link. Or we can just delete this line.
2. In the error message (8.4.8), I see "security updates" and "security fixes". The warning message uses "security fixes". I have a slight preference for "security updates", but I care more that we use the same phrase consistently.
3. In the error message (8.4.8), "Update to a supported minor soon" is again a link to /admin/reports/updates/update, so again it duplicates the "available updates" link in the next message. Same suggestions as before.
4. In all three messages, we use "release cycle overview" as the link text for https://www.drupal.org/core/release-cycle-overview. It is a little confusing that the title of that page does not match the URL and link text here. Maybe we should change the title of that page to "Release Cycle Overview", with the current title as a subtitle.

I am setting the status to "needs work" because of the issues I raised in #58.

I am open to other suggestions, and maybe we should discuss this again at the weekly UX meeting, but I think the following would be acceptable:

1. Use either "security fixes" or "security updates" consistently.
2. Make "Update to 8.6 or higher soon" (in the warning message) and "Update to a supported minor soon" (In the error message) plain text, and add a second sentence: "See the available updates page for more information." Make "available updates" a link as in the "Drupal Core Update Status" message.
3. In the error message, change "soon" to "as soon as possible". The text should make clear that this is more urgent than the warning case. (The fact that this is an error should reinforce the urgency.)

I am not really happy with (2), since there is even more duplication. My goals here are to make a clear recommendation, to convey the level of urgency, and to avoid confusion (two links with the same target but different link text).

I forgot to mention (3) in my previous comment.

I forgot to remove the "Needs followup" tag when I added the followup issues (Comment #56). Done.

When I wrote Comment #8, I did not notice that the Component is set to update.module. What if the Update Manager module is not enabled? Do we want any sort of message? We could have a static message with a link to a handbook page, or we could recommend enabling the Update Manager module to get information about EOL.

I mentioned in #8 that the existing code in the Update Manager module is already complex. That is why we are adding a new message instead of modifying the existing one. The results, as I said in #58 and #59, are not really satisfactory. Maybe it is time to do the hard work of refactoring that complex code. Yet another follow-up?

1. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -12,6 +12,12 @@
   +   * How many minor versions of Drupal are supported.
   +   *
   +   * This constant is used to determine if the installed version is still
   +   * supported with security updates.
   

   I don't think we need the second part here.

2. @robpowell thanks for catching that it was prompting to update to 8.7

3. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -163,11 +169,12 @@ public static function getSecurityCoverageMessage(array $project_data) {
   +          $suggested_update_version = current($project_data['releases']);
   

   We should avoid using array pointers.

   This gets the first release here but that will only work if we because right now we don't have RC or other prelease for 8.7.x. if we did that would be the first release. Though also I don't think we can guarantee that for instance if there was security release only for 8.5.x that would probably put it to the top.

   Since we know that the installed version is only supported for 1 more minor what we really want is
   $current_minor + 1

   Fixing

4. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -344,6 +344,140 @@ public function securityUpdateAvailabilityProvider() {
   +      '1.0, supported' => [
   +        'site_patch_version' => '1.0',
   +        'requirements_section' => 'Warnings found',
   +        'fixture' => 'sec.2.0',
   +        'coverage_messages' => [
   +          'The installed minor version of Drupal, 8.1, will receive security fixes until the release of 8.3.0.',
   +          'Update to 8.3 or higher soon to continue receiving security fixes.',
   +          $release_coverage_message,
   +        ],
   

   Our test expectation was actually wrong here.

   In the fixture 8.3.0 has not come out yet so we should be prompting to update to 8.2 or higher.

   The tests didn't fail in #57 because there was 8.3.0-rc1 release so this passed.

   Updating the message and also adding a new fixture which doesn't have the rc1 release so we can test both cases.

@tedbow:

Thanks for fixing those problems.

Please address the points I raised in #58 and #59. I am setting this issue back to NW for that. If you disagree with my suggestions, please explain.

In #59.2, I recommended making "Update to 8.6 or higher soon" (in the warning message) and "Update to a supported minor soon" (In the error message) plain text. If you disagree with that, then please at least move "soon" out of the links.

So far, I have concentrated on UX review, not code review. Looking through the code, I have a few suggestions:

1. The getSecurityCoverageMessage() has a bunch of conditionals. Please add some code comments to help orient anyone reading the code.
2. Most of that method is inside a big if block. The only code outside that block is $message = ''; before and return Markup::create($message); after. Consider changing the return type to string so that we can invert the conditional and exit early, then have the main part of the method indented one fewer level. Of course, make adjustments when calling the method.

Again, I do not like duplicating so much information, but improving that may be outside the scope of this issue. I plan to bring this up at the next UX meeting.

While we made the Extended Security Support policy change last year, we still haven’t completed this issue, which is critical to to ensure users know about security coverage for their minor and make the right choice in response to security releases. After talking to @xjm, escalating to critical.

re #62 @benjifisher thanks for the reviews. Sorry it has taken soooo long to get back to this issue.

re #58

1. [UPDATE] I don't think fully understood this see my response to #59.2

   I agree that having the 2 texts that lead to the same path is not ideal but I think because they are in 2 different message rows it is not that bad because

   • Some user might just read one of the message rows and not the other. Because of that it is good to have the link to the updates page in each message row.
   • Without a much larger refactoring than this patch is currently doing I don't think we can assume that if one message row that other one will also be there. Even if that the case now we could easily update the logic and that not be the case and then we would lose the link to the update page.

   Maybe make "Update to 8.6 or higher soon" plain text, not a link. Or we can just delete this line.

   For the above reasons I think we should keep this a link.

2. Nice catch! Changed to "security updates" everywhere
3. I think it works that way it is because of reasons in 1)
4. Seems like good idea. made the suggestion here https://www.drupal.org/core/release-cycle-overview#comment-13245570

   Seems like we won't have to wait till this issue is committed though to make this change.

re #59

1. fixed
2. Fixed. I didn't fully understand this suggestion in #58. Added the new sentence "See the available updates page for more information."
3. fixed

re #62

1. Added comments
2. I did the check in the beginning and if
   !isset($project_data['security_coverage_info']['additional_minors_coverage'])
   I returned an empty string. I didn't change the return type so if we add others to this method we don't have to remember to call Markup:create()

I also had to make some changes \Drupal\Tests\update\Functional\UpdateCoreTest::testSecurityCoverageMessage() and it's dataprovider.

+++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
@@ -344,6 +344,163 @@ public function securityUpdateAvailabilityProvider() {
+    foreach ($coverage_messages as $coverage_message) {
+      $assert_session->pageTextContains($coverage_message);
+    }
+    foreach ($not_contains_messages as $not_contains_message) {
+      $assert_session->pageTextNotContains($not_contains_message);
+    }

Here we were checking that messages are/aren't on the entire page. But now that we are using the same message that appears on the page elsewhere as per #59.2 we should only be checking the security coverage section.

We probably should have been checking this section anyways since that is what the test is suppose to cover.

I did some review since it has been awhile

1. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,275 @@
   +    if ((int) $latest_full_release['version_major'] > $security_supported_release_info['version_major']) {
   

   there are a number of these comparisons where we use (int) on 1 side and not on the other. Since we are getting this from update_get_available() which is well document and gets its in data from outside XML I think it is not a bad idea to be paranoid about this data and also cast $security_supported_release_info array values to int.

2. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,275 @@
   +  public static function getSecurityCoverageMessage(array $project_data) {
   

   This does not need to be public

3. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -345,6 +345,161 @@ public function securityUpdateAvailabilityProvider() {
   +   * @param string $site_patch_version
   +   *   The patch version to set the site to for testing.
   +   * @param array $coverage_messages
   +   *   The expected coverage messages.
   +   * @param array $not_contains_messages
   +   *   Messages that should not appear in the coverage section.
   +   * @param string $fixture
   +   *   The test fixture that contains the test XML.
   +   *
   +   * @dataProvider securityCoverageMessageProvider
   +   */
   +  public function testSecurityCoverageMessage($site_patch_version, $requirements_section, $fixture,  array $coverage_messages = [], array $not_contains_messages = []) {
   

   requirements_section was not in the function @param's. Also they were out of order.

   Changing the name and also just making it always a string and not sometimes NULL.

Talked with @xjm about and we agreed that we should merge #2998287: Provide accurate information on the security coverage of the 8.x final minor and LTS, and recommend updating to the next major version when appropriate into this issue.

This current issue we had hoped would be in by 8.6.x(🤷‍♂️) but since this issue is targeted for 8.8.x the first sites that will use this logic will be affected by the need LTS logic. There is no point in committing this issue without 8.8.x/LTS specific logic. Because currently it would tell users that 8.8.0 will be supported until 8.10 which we know is not true.

So here is the patch from #2998287-6: Provide accurate information on the security coverage of the 8.x final minor and LTS, and recommend updating to the next major version when appropriate which was built on #66

Added the commented I had for the patch on #2998287-6: Provide accurate information on the security coverage of the 8.x final minor and LTS, and recommend updating to the next major version when appropriate

In this patch I provided the the 8.9 LTS as a hard-coded. But it will only use it if a LTS for Drupal 8 is not available in update XML. If one is in the update XML will use that.

This could allow #2998285: Add information on later releases to updates.drupal.org to be delayed or even not done for LTS purposes if the 8.9 plan stays the same. If the 8.9 support end changes or if another release will be the LTS then drupal.org would have to provide LTS release information.

I just realized I didn't actually implement what #2998285 was suggesting exactly. It was suggesting putting future release in the releases array that is provided in the XML. In this patch I added support for lts_releases as a top-level item in the update XML. This would have less information than regular releases but would provide a support_end value.

I didn't implement this differently on purpose from the suggestion in #2998285 but this way may be simplier for drupal.org to implement. I am not sure.

The current patch allows:

1. 1 LTS per major
2. The LTS does not have to be in the releases array provided by drupal.org xml.
3. If no LTS for a major is provided in the XML 8.9.0 with support end of 11/01/2021 is used as a fallback. Additional future major version LTS versions can be added to \Drupal\update\UpdateHelper::setFallBackLts()
4. If a release is 1 minor version before the LTS then it will be assumed that it will be supported until the next major is released. For example 8.8 will be assumed to be supported until 9.0.0(unless a different LTS is provided)
5. Since \Drupal\update\UpdateFetcher::buildFetchUrl() includes \Drupal::CORE_COMPATIBILITY in URL only releases for the current major are returned. Therefore we can't rely on 9.0.0 release information being included to determine if it has been released and 8.8 no longer being supported. Therefor this patch looks at supported_majors in the XML. I assumed that if a major is in this list then that MAJOR.0.0 has been released. Is that true? I assume this would not be added before this release. Is that true?

I have added some test coverage which includes lts_releases provided in the XML to prove this works and that we can override the Fallback LTS of 8.9. But I would like some feedback on this idea in general before I add more test coverage.

Would be great to have test presence status as addition or follow up
Having "green scheeld" does not mean tests works and pass - it could lead to some d-org page about how to find and contribute to issue to help resolve it

For Drupal.org, we’ll first need to store the new data somewhere. I think this would be a new field on tagged releases for the support end date, if it is empty, it is not an LTS release. (We could make this available to contrib, too.)

I’d like to get away from

+        <version_major>8</version_major>
+        <version_minor>10</version_minor>
+        <version_patch>0</version_patch>

It is redundant with the version number. And for contrib, version_minor & version_patch are swapping as we introduce semver. IIRC, last time we looked, we found that core actually didn’t use the existing version_* elements. I don’t recommend starting to use them.

Since \Drupal\update\UpdateFetcher::buildFetchUrl() includes \Drupal::CORE_COMPATIBILITY in URL only releases for the current major are returned.

That needs to stop using \Drupal::CORE_COMPATIBILITY ASAP. Use all like https://updates.drupal.org/release-history/token/all. See #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates

Are these really LTS “releases” or releases on LTS branches? This data could also be something like versions falling in certain version constraints are LTS.

@drumm

Are these really LTS “releases” or releases on LTS branches?

Yes they are really LTS branches

I think this would be a new field on tagged releases for the support end date, if it is empty, it is not an LTS release. (We could make this available to contrib, too.)

So because it is branches it probably can't be on an individual release.

This is also true for core because it would be super helpful to be able to designate LTS branch before there were actually any releases for that branch.

For instance because this issue is trying display a message about security coverage for 2 core minors, ie "you are on drupal 9.1 which is supported until 9.3.0 is released" this will be affected by LTS branches.

if we don't factor in LTS branch logic then the 8.8.x branch would be supported until 8.10.0.
But

1. we know 8.9.x will be the last branch for 8.x.
2. As soon as 8.8.0 is released there needs to be something in the update xml(or hard-coded in core) that tells us that 8.9.x will be the LTS branch. This allows us to say "You are on 8.8 which will be supported until 9.0.0" because with our 2 minors support policy + 8.9 being the last minor or 8.x we know this.
3. But at the time 8.8.0 is released the 8.9.0 release will not be out.

So @drumm, is there someone we can store this on the project level?

When I look at one of my projects I see

Which has "Recommended major version" for each branch
Is this stored on the project itself? maybe we could have a multi-value field "LTS-Minors". The you could have 1 minor and 1 end for each major branch that has any releases.

So right now for core it could have a "8 lts branch" and "8 lts end date". Then as soon as 9.0.0 is released you could add a "9 lts branch" and "9 lts end data", though you won't have too.

Just some random observations while reading though.

1. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,397 @@
   +          throw new \LogicException('::getAdditionalSecuritySupportedMinors() should never bee called before checking ::isNextMajorReleasedWithoutSupportedReleased().');
   

   never bee called

2. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,397 @@
   +      'version_major' => (int) $existing_release['version_major'],
   

   There's SO much casting to int throughout. Is it at all possible to ensure they are ints before returning them?

3. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,397 @@
   +   * @param string|null $major
   

   int|null?

4. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,397 @@
   +          'support_end' => '11/01/21',
   

   YYYY MM DD please (see https://xkcd.com/1179)

5. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,397 @@
   +   * @return array
   

   array|null?

@tim.plunkett thanks for the review.

1. Fixed. I will 🐝 careful of this in the future
2. Sorry about that. I casted much of these away and this patch goes from 22 (int) instances to 7.
3. Fixed
4. Fixed. I feel like real programmer now! (see https://xkcd.com/378)
5. fixed

This patch also does a bunch of refactoring. I talked with @xjm about this and the changes from that discussion are

1. Took out all logic that reads LTS information from the xml. we can add this in follow once it is determine what drupa.org should provide. We will probably want to get that in ASAP after this issue because if doesn't get in before 8.8.0 is released then any site that is on 8.8.0 and doesn't update to patch releases will not get any updated information about 8.8.0 overage window. though maybe this is unlikely to change. If it is likely to change but be extended at least we won't be telling them they are covered when they are not.
2. limited the LTS logic here to hard-coding the dates for 8.9 and 8.8.
3. I also did not now that 8.8 is really security covered for a year and the coverage window is not strictly connected to other releases.
4. I added a extra warning when 8.8 is six months away from being unsupported like there is for other release in this patch that covered for 2 minors. In those cases a warning if given if it is 1 minor(six months roughly) away from losing coverage.
5. Per discussion with @xjm I did not add warning based on 8.9, the LTS, being six months away from coverage expiring.

Another possible change that didn't get into this patch is that the current update XML from Drupal.org does have supported_majors which could be helpful
Presumably if is 8 is not in that list then the LTS(and all other drupal 8 releases are unsupported). So this maybe a way now we could put logic in cover the unlikely scenario that the 8.9 LTS window is shortened.

If supported_majors does not contain 8 we could give an unsupported message regardless of whether we are in the hard coded LTS dates.

I am including an interdiff from #66 because most the changes in #67aren'trelevantt anymore.

+++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
@@ -345,6 +345,259 @@ public function securityUpdateAvailabilityProvider() {
+    file_put_contents('/Users/ted.bowman/Sites/www/test.html', $this->getSession()->getPage()->getOuterHtml());

🤦‍♂️😞 sorry

Interdiff from 66 again to see all in 1 file

On the positive side the patch just got a little smaller!!! 🎉🎊🥳

1. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,387 @@
   +            '%project' => $project_data['title'],
   

   This another instance should be $project_data['name'] not 'title'. fixed which caused test fail

2. +++ b/core/modules/update/src/UpdateHelper.php
   @@ -0,0 +1,387 @@
   +    if ($project_data['project_type'] == 'core') {
   

   Should be ===

Which has "Recommended major version" for each branch
Is this stored on the project itself? maybe we could have a multi-value field "LTS-Minors". The you could have 1 minor and 1 end for each major branch that has any releases.

So right now for core it could have a "8 lts branch" and "8 lts end date". Then as soon as 9.0.0 is released you could add a "9 lts branch" and "9 lts end data", though you won't have too.

This is stored in a D5-era efficient custom table. Drupal.org doesn’t exactly have any sort of real entity for a “branch” that’s extended with fields. Release nodes are sometimes dev releases, and those do have fields. And/or we can extend that old table.

I’m thinking we’ll just do a one-off settings.php variable to store this on Drupal.org:

$conf['drupalorg_core_lts'] = [
  '8.8.x' => ['end' => 'future date', 'message' => 'maybe a message too?'],
];

With whatever data is needed to support this.

1. I would like to see the class of helper functions replaced with something more object-oriented:

       +  public static function getSecurityCoverageInfo(array $project_data, array $releases) {
       +  private static function getSupportUntilReleaseInfo(array $project_data, array $releases) {
       +  private static function getSecurityCoverageMessage(array $project_data) {
       +  public static function getSecurityCoverageRequirement(array $project_data) {
       +  private static function getLtsRequirement(array $project_data) {
       +  private static function createRequirementForSupportEndDate(array $project_data, $end_date_string, $warn_at = '') {
       

   That is a lot of functions passing around $project_data. If we make that a class property, then it will simplify things a little. It would also mean you only have to write one doc block for the $projectData variable. That would give us an opportunity to give some helpful documentation about the expected structure of this variable instead of the minimal "The project data" description.

   In fact, as I try to review this patch, I am frustrated by not knowing what sort of information to expect in that variable. I think that adding such documentation is more important than simplifying the code by not passing around the variable all the time. The documentation would also help when the time comes to update the XML feed that (I think) ends up populating that array.

   I would create a ProjectData helper class, with a $projectData property that is set in the constructor. This class would have the responsibility of "knowing" the update data about a project, and answering questions via its methods.

   All of the methods I listed above would be part of this class. There are a few other methods in the current UpdateHelper class that do not seem to use $project_data. I am not sure what to do with them: leave them in the class or move them somewhere else.

   If there is existing code that uses the same project data, then we should consider moving it into the new class in a follow-up issue.

2. +  private static function getAdditionalSecuritySupportedMinors(array $security_supported_release_info, array $releases) {
   +  private static function getSupportUntilReleaseInfo(array $project_data, array $releases) {
   +  private static function getMostRecentFullRelease(array $releases, $major = NULL) {
   +  private static function getSecurityCoverageMessage(array $project_data) {
   +  private static function isNextMajorReleasedWithoutSupportedReleased(array $releases, array $security_supported_release_info) {
   +  private static function getLtsRequirement(array $project_data) {
   +  private static function getVersionNotSupportedMessage($project, $minor_version) {
   +  private static function getAvailableUpdatesMessage() {
   +  private static function createRequirementForSupportEndDate(array $project_data, $end_date_string, $warn_at = '') {
       

   Why all the private functions? Normally we make everything public or protected, in case someone needs to extend our class and override some of them.

@benjifisher thanks for the review!

I am working a refactor that is along the lines of what you asked for.

re #80

@benjifisher thanks again for the review!

1. I would like to see the class of helper functions replaced with something more object-oriented:

   Yep, I agree. the helper class started out as just way not mess to the functions in update.compare.inc but they have gotten more complicated.

2. +++ b/core/modules/update/update.compare.inc
   @@ -425,6 +428,8 @@ function update_calculate_project_update_status(&$project_data, $available) {
   +  $project_data['security_coverage_info'] = UpdateHelper::getSecurityCoverageInfo($project_data, $available['releases']);
   +
   
   +++ b/core/modules/update/update.install
   @@ -36,6 +37,10 @@ function update_requirements($phase) {
   +      if ($core_coverage_requirement = UpdateHelper::getSecurityCoverageRequirement($data['drupal'])) {
   +        $requirements['coverage_core'] = $core_coverage_requirement;
   +      }
   

   I realized that this helper class was actually handling 2 different tasks

   1) calculating the security coverage information and 2) constructing the security coverage message for hook_requirments based on that and other project data.

   I thought these 2 tasks called some common functions but I was wrong.

   So split UpdateHelper into 2 new classes.

   1) ProjectSecurityCoverageCalculator which needs the $project_data and $releases and exposes getSecurityCoverageInfo().

   2) ProjectSecurityRequirement which using the coverage and other project information to create the info that hoo_requirments needs for the security coverage requirement.

3. Why all the private functions? Normally we make everything public or protected

   I think this is case where we really don't want this to extensible. This is logic that is tied to the Drupal projects security coverage policy. I think we should lock this down as much as possible and not encourage developer to extend or alter this.

   If a developer really wants to alter this they can implement hook_update_status to alter the security coverage info, really not a good idea but possible

   These are not services as of now and I don't think they should be. This is not something that should be swapped out. So having the methods as private I don't think is that big a deal.

   We do use private methods in some cases in core.

   As far as changing the security coverage requirement from update_requirments() this is not possible but this the same for all results of hook_requirments. Again I don't think a developer should do this but there is an existing issue to allow it #309040: Add hook_requirements_alter()

+++ b/core/modules/update/src/ProjectSecurityRequirement.php
@@ -0,0 +1,235 @@
+  private function getLtsRequirement() {
+    if (!($this->projectData['project_type'] === 'core' && $this->projectData['name'] === 'drupal' && (int) $this->projectData['existing_major'] === 8)) {
+      return [];
+    }
+    $minor_version = explode('.', $this->projectData['existing_version'])[1];
+    $requirement = [];
+    if ($minor_version === '8') {
+      $requirement = $this->createRequirementForSupportEndDate('2020-12-02', '6 months');
+    }
+    elseif ($minor_version === '9') {
+      $requirement = $this->createRequirementForSupportEndDate('2021-11-01');
+    }
+    return $requirement;
+  }

I think this logic should probably be moved to a new \Drupal\update\ProjectSecurityCoverageCalculator::getSupportUntilDateInfo()

That way the all the determine about when support should end whether it is based on a date(hard-code for now) or would be in ProjectSecurityCoverageCalculator

and then \Drupal\update\ProjectSecurityRequirement::getSecurityCoverageRequirement() could just evaluate that and make the requirement.

working on that now.

1. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -0,0 +1,235 @@
   + private function getAvailableUpdatesMessage() {
   

   This can be a static function

2. Also fixing #83

whoops

+++ b/core/modules/update/src/ProjectSecurityRequirement.php
@@ -0,0 +1,220 @@
+    $time = \Drupal::service('datetime.time');
...
+      $date_formatter = \Drupal::service('date.formatter');

Implemented ContainerInjectionInterface to get rid of these

Thanks for these updates! It is looking a lot easier to review now, although right now I only have time for a few comments.

In the long run (follow-up issues) I would like to see the documentation moved from UpdateManagerInterface::getProjects() and update_process_project_info() to the new class.

I am looking at ProjectSecurityCoverageCalculator.

  /**
   * Constructs a ProjectUpdateData object.
   *
   * @param array $project_data
   *   Project data form Drupal\update\UpdateManagerInterface::getProjects().
   * @param array $releases
   *   Project releases as returned by update_get_available().
   */
  public function __construct(array $project_data, array $releases) {

I suggested a class named ProjectData, and apparently you considered ProjectUpdateData before settling on ProjectSecurityCoverageCalculator. As a general rule, shorter is better. According to Object-oriented code,

Classes and interfaces should have names that stand alone to tell what they do without having to refer to the namespace, read well, and are as short as possible without losing functionality information or leading to ambiguity.
...
If necessary for clarity or to prevent ambiguity, include the last component of the namespace in the name.

So I guess ProjectUpdateData might be better than my original suggestion.

The one public method in the class (other than the constructor) is getSecurityCoverageInfo(), so I think it is redundant to include "security coverage" in the class name. That also makes it hard to add other public methods to the class. Please consider reverting to a name based on the data the class contains rather than the information we get out of it.

There is a typo in the dock block ("form" instead of "from").

Please mention in the @param comment that $project_data is also modified by update_process_project_info(). I got as far as seeing that is where the existing_version key is added.

Please be consistent in @var and @see comments about including the leading backslash. We usually leave it off, don't we?

I will continue my review later.

1. Please be consistent in @var and @see comments about including the leading backslash. We usually leave it off, don't we?

   It appears we usually have it if it is referring to a namespaced item but for no namespaced items we don't have it.
   so it should be removed here
   * @see \update_get_available()
   fixed

2. Please mention in the @param comment that $project_data is also modified by update_process_project_info(). I got as far as seeing that is where the existing_version key is added.

   fixed

3. There is a typo in the dock block ("form" instead of "from").

   fixed

4. Please consider reverting to a name based on the data the class contains rather than the information we get out of it.

   Right now I am changing the class name to ProjectSecurityData and the method name to getCoverageInfo().

   I think this removes the redundant info from the method name.

   I don't think we should name it ProjectUpdateData because the class is already pretty complicated with security only logic. I addition this only covers the current policy for security coverage for minor and the LTS logic for core. When have to extract the LTS dates from the XML it will be even more complicated. If contrib adds security coverage there will probably some minor changes between core and contrib which make it more complicated.

   I don't think we want to have this class take over extracting all information about project updates.

   I have uploaded an xml document that is just for Drupal core that the update module receives.

   If the goal of this class was represent all update data would eventually have to add all of these public methods

   getTitle()
   getReleases()
   getSupportedMajors()
   isCurrentMajorSupported()
   getDefaultMajor()
   getExistingRelease()
   getTerms()
   getApiVersion()
   getDevelopmentStatus()
   isActivelyMaintained()
   getMaintenanceStatus()
   isActivelyMaintained()
   getProjectType()
   

   Above is all the methods just from reading the existing xml if we want to move all the logic that is done elsewhere into this class like getCoverageInfo() we would also have to add methods like

   getInstallType()
   getStatusReason()
   getProjectStatus()
   getLatestVersion()
   getRecommendVersion()
   getLatestDevVersion()
   

   These just the things I saw in update.compare.inc. This probably stuff elsewhere. I would prefer to keep this class focused just on security.

Taking another look at ProjectSecurityRequirement too.

1. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -0,0 +1,248 @@
   +   * Constructs ProjectUpdateData object.
   

   Class name needs updating

2. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -0,0 +1,248 @@
   +   *   The 'security_coverage_info' key should be set calling this method.
   

   Adding referece to setting this key from ProjectSecurityData::getCoverageInfo()

3. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -0,0 +1,248 @@
   +  public function getSecurityCoverageRequirement(array $project_data) {
   

   Changing to getRequirment() to remove redundant info from class name.

4. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -0,0 +1,248 @@
   +   *   An array if there is security coverage requirement, otherwise NULL.
   

   Changing to

   Requirements array as specified by hook_requirements().

   This will never be null.

1. +++ b/core/modules/update/src/ProjectSecurityData.php
   @@ -0,0 +1,217 @@
   +    if (empty($this->releases[$this->projectData['existing_version']])) {
   +      return [];
   +    }
   +    $existing_release = $this->releases[$this->projectData['existing_version']];
   +
   +    if (!empty($existing_release['version_extra'])) {
   +      return [];
   +    }
   +    // Minors 8.8 and 8.9 have special logic for security support.
   

   Super nit, but the blank lines are weird here. Usually I see a blank line after any guard condition (one that returns early).

2. +++ b/core/modules/update/src/ProjectSecurityData.php
   @@ -0,0 +1,217 @@
   +   * @param array $security_supported_release_info
   +   *   Release information as return by update_get_available().
   

   It would have been very nice if update_get_available() returned data objects instead of ArrayPI, but that's not this patch's fault. However, is it worth *not* typehinting array in cases where it's not a iterable set?

3. +++ b/core/modules/update/src/ProjectSecurityData.php
   @@ -0,0 +1,217 @@
   +   * @throws \Exception
   ...
   +          throw new \LogicException('::getAdditionalSecuritySupportedMinors() should never be called before checking ::isNextMajorReleasedWithoutSupportedReleased().');
   

   This should say when it is thrown on the next line, and also have the more specific LogicException

4. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -0,0 +1,250 @@
   +  public function getRequirement(array $project_data) {
   +    $this->projectData = $project_data;
   

   This is a problem for me. Unlike the other class, which takes $project_data in the constructor, this one mutates the state via a public method.

   While the class continues to only have the one public method, that is probably fine. But the minute another method is added, that statefulness can start to cause very odd bugs.

   I understand the desire to use ContainerInjectionInterface, so I'm not sure what to suggest instead...

5. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -0,0 +1,250 @@
   +        $requirement['title'] = t('Drupal core security coverage');
   

   $this->t(), here and throughout

6. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -0,0 +1,250 @@
   +      $message = '<p>' . t(
   +          'The installed minor version of %project, %version, will receive security updates until the release of %coverage_version.',
   +          [
   +            '%project' => $this->projectData['title'],
   +            '%version' => "$major.$minor",
   +            '%coverage_version' => $security_info['support_end_version'],
   +          ]
   +        ) . '</p>';
   

   The line breaks and indents here are weird and hard to read. Perhaps creating local variable of the array of % args, and then the rest can be a oneliner?

7. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -345,6 +345,258 @@ public function securityUpdateAvailabilityProvider() {
   +  public function testSecurityCoverageMessage($site_patch_version, $requirements_section_message, $fixture, array $coverage_messages = [], array $not_contains_messages = [], $mock_date = '') {
   

   I understand why there's a functional test, but is it also possible to have a unit test for the more straightforward parts of the logic?

@tim.plunkett thanks for the review!

1. fixed
2. I don't think we should make the distinct. I get a phcs error "missing parameter type" if I leave this out.
3. Fixed but I wondering if we even need isNextMajorReleasedWithoutSupportedReleased() now. I think this was stop gap when this patch didn't include logic for the drupal 8 LTS release. Will update the issue after I chat with @xjm
4. Yeah didn't like this change especially. Changing it back not use ContainerInjectionInterface
5. fix.
6. Changed to onliner to fix. Also created $next_minor variable so the next use of $this->t() can be a 2 liner.
7. will work on the test. Uploading the rest of the changes for now.

1. +++ b/core/modules/update/tests/modules/update_test/drupal.sec.2.0_9.xml
   @@ -0,0 +1,157 @@
   +            <name>Drupal 8.3.0</name>
   +            <version>8.3.0</version>
   +            <tag>DRUPAL-8-3-0</tag>
   +            <version_major>8</version_major>
   +            <version_minor>2</version_minor>
   

   version_minor is wrong here

2. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -345,6 +345,258 @@ public function securityUpdateAvailabilityProvider() {
   +    if ($requirements_section_message) {
   ...
   +    else {
   +      $this->assertEmpty($coverage_messages, 'If messages are expected a requirements section most be provided');
   +      $this->assertEmpty($not_contains_messages, 'If messages to confirm do not exist are provided a requirements section most be provided');
   +    }
   

   This else here basically just tests if the provided arguments aren't wrong not the test output. If $requirements_section_message is not provided there is no section on the status report for core security coverage so we can't test if something is or isn't there.

   but we should check to make sure "Drupal core security coverage" doesn't appear on the page at all. Since we don't expect there to a section.

3. I have another patch that addresses my question in #92.3 but either way these 2 points need to be fixed.

re #92.3

ProjectSecurityData currently has logic to check if the next major version has been released and the version the installed version is suppose to be covered until has not been released. In that case we don't show any message because this wasn't suppose to happen so we we don't know that coverage.

but now that we have explicit coverage dates for 8.8.x and 8.9.x we probably can get rid of that. This could only effect 8.6.x and 8.7.x because any other minor release has already had the version they are supported to released. For 8.6.x 8.8.0 should be released in December so there is no way it won't be released before 9.0.0. So that only leaves 8.7.x which should be supported until 9.0.

If the logic I mentioned is removed then 9.0.0 is released and 8.9.0 has not been released 8.7.x installs would still get supported until 8.9.0 message. Of course this assumes that this patch is backported to 8.7.x

Here is a patch with that logic removed. You can see the changes in the test cases. It makes ProjectSecurityData much simpler.

Updated the proposed solution

I will next update the screenshots in the summary

Added update screenshots to summary

1. +++ b/core/modules/update/src/ProjectSecurityData.php
   @@ -0,0 +1,148 @@
   +    if ($minor_version === '8') {
   +      $info['support_end_date'] = '2020-12-02';
   +      $info['support_ending_warn_date'] = '2020-6-02';
   +    }
   +    elseif ($minor_version === '9') {
   

   This logic if we didn't update it in the Drupal 9 cycle would have cause these dates to be used for 9.9 and 9.8 messages.

   also we could have not found out about it until 9.8 because there was test coverage.

2. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -345,6 +345,266 @@ public function securityUpdateAvailabilityProvider() {
   +      '0.0, unsupported' => [
   +        'site_patch_version' => '0.0',
   

   All of the test case assumed 8.x.x

   changing to send complete version number so we can test 9.8 and 9.9

3. Adding test for 9.9 and 9.8 to prove they follow the 2 minors pattern.

+++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
@@ -345,6 +345,296 @@ public function securityUpdateAvailabilityProvider() {
+    $test_cases['8.9, lts 6 month'] = $test_cases['8.9, lts'];

I forgot to update the key for the test case here so got an undefined index error.

+++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
@@ -345,6 +345,296 @@ public function securityUpdateAvailabilityProvider() {
+      file_put_contents('/Users/ted.bowman/Sites/www/test.html', $this->getSession()->getPage()->getOuterHtml());

also whoops

@tedbowman, thanks for your continued work on this. I am sorry my promised review is so long delayed.

I also apologize for the mix of nit-picks among the other comments. It is the result of how I review a patch.

1. Fixing all the technical debt in the Update module is way outside the scope of this issue. The problem is that this debt is making it hard for me to see what is going on and do an honest review. I think the changes you have already made will help when we decide to reduce that debt in future issues. For now, what would really help is more documentation on the projectData and releases properties. Even if you just document the keys that the new classes expect and add, that would make it much easier to review this issue. You can add a disclaimer, "Other keys are ignored" or something. The @see references are helpful, but the functions they reference do not document these keys. Adding documentation there seems out of scope for this issue.
   
2. Sorry for nit picking. You can leave it as is if you disagree.

   @@ -36,6 +37,10 @@ function update_requirements($phase) {
          $data = update_calculate_project_data($available);
          // First, populate the requirements for core:
          $requirements['update_core'] = _update_requirement_check($data['drupal'], 'core');
   +      if ($core_coverage_requirement = (new ProjectSecurityRequirement($data['drupal']))->getRequirement()) {
   +        $requirements['coverage_core'] = $core_coverage_requirement;
   +      }
       

   Generally, we try to keep code lines under 80 characters. We can do that by making the assignment outside the conditional:

         $requirement = (new ProjectSecurityRequirement($data['drupal']))
           ->getRequirement();
         if ($requirement) {
           $requirements['coverage_core'] = $requirement;
         }
       

   Since the variable is never referenced again, I think it is OK to give it a short name. Alternatively,

         $project = new ProjectSecurityRequirement($data['drupal']);
         if ($requirement = $project->getRequirement()) {
           $requirements['coverage_core'] = $requirement;
         }
       

3. +++ b/core/modules/update/update.compare.inc
   @@ -296,6 +298,7 @@ function update_calculate_project_update_status(&$project_data, $available) {
      foreach ($available['releases'] as $version => $release) {
        // First, if this is the existing release, check a few conditions.
        if ($project_data['existing_version'] === $version) {
   +      $project_data['existing_release'] = $release;
          if (isset($release['terms']['Release type']) &&
              in_array('Insecure', $release['terms']['Release type'])) {
            $project_data['status'] = UPDATE_NOT_SECURE;
   @@ -425,6 +428,8 @@ function update_calculate_project_update_status(&$project_data, $available) {
        $project_data['recommended'] = $project_data['latest_version'];
      }
    
   +  $project_data['security_coverage_info'] = (new ProjectSecurityData($project_data, $available['releases']))->getCoverageInfo();
       

   I hate this function. The doc block is 61 lines long, and the body (before this patch) is 311 lines long. Fixing it is outside the scope of this issue, but I hope that the classes we add here will make it easier to refactor this function.

   This function adds several keys to the $project array (passed by reference). If the doc block described the existing keys, then adding the two new ones would be in scope for this issue. Maybe it is in scope for this issue to describe all the keys.

   Same nit as in #2 (but leave it as is if you disagree): I would like to see the long line broken up.

4. +++ b/core/modules/update/src/ProjectSecurityData.php
   @@ -0,0 +1,148 @@
   +<?php
   +
   +namespace Drupal\update;
   +
   +/**
   + * Calculates a projects security coverage information.
       

   Another nit: it should be "project's" (possessive).

5. +  /**
   +   * Constructs a ProjectUpdateData object.
   ...
   +  public function __construct(array $project_data, array $releases) {
       

   There is still a mismatch between the class name and the doc block comment.

6. +  public function getCoverageInfo() {
   +    $info = [];
   +    if (!($this->projectData['project_type'] === 'core' && $this->projectData['name'] === 'drupal')) {
   +      // Only Drupal core has an explicit coverage range.
   +      return [];
   +    }
   +    $minor_version = explode('.', $this->projectData['existing_version'])[1];
   +    // Support for Drupal 8's LTS release and the version before are based on
   +    // specific dates.
   +    if ($this->projectData['existing_major'] === '8' && $minor_version === '8') {
       

   I think that $projectData['existing_major'] is the major version of Drupal that a project (in this case, Drupal core itself) is compatible with. Isn't that information likely to change now that other projects can be compatible with more than one version of Drupal core? Since this code just deals with Drupal core, I think it will be more reliable, and clearer, to get the major and minor versions from the same key as in ProjectSecurityRequirement::getVersionEndCoverageMessage():

         list($major_version, $minor_version) = explode('.', $this->projectData['existing_version']);
         if ($major_version === '8' && $minor_version === '8') {
         // ...
       

   Or maybe we can get the major and minor versions from $this->releases[$this->projectData['existing_version']]?

7. +      $info['support_ending_warn_date'] = '2020-6-02';
       

   Use "06" instead of "6".

8. +   * @return array
   +   *   The release information.
   +   */
   +  private function getSupportUntilReleaseInfo() {
       

   Document the keys and their types: version_major, version_minor, and version_patch are type int and version is string. If the information is not available, then the function returns an empty array.

9. +  /**
   +   * Gets the number of additional minor releases supported.
   +   *
   +   * @param array $security_supported_release_info
   +   *   The security supported release.
   +   *
   +   * @return int
   +   *   The number of additional supported minor releases.
   +   */
   +  private function getAdditionalSecuritySupportedMinors(array $security_supported_release_info) {
       

   The @param comment should include "as returned by getSupportUntilReleaseInfo()".

10. +    if (isset($latest_minor)) {
    +      if ($security_supported_release_info['version_minor'] <= $latest_minor) {
    +        return -1;
    +      }
    +      return $security_supported_release_info['version_minor'] - $latest_minor;
    +    }
    +    return NULL;
        

    I think this would be clearer:

        return isset($latest_minor)
          ? $security_supported_release_info['version_minor'] - $latest_minor
          : NULL;
        

    Then adjust the code where this value is used to test $value <= 0 instead of $value == -1. In fact, this value is checked in ProjectSecurityRequirement and the test is $value > 0, so perhaps no adjustment is needed.

11. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
    @@ -0,0 +1,214 @@
    +  /**
    +   * Drupal project data..
    ...
    +  protected $projectData;
        

    Nit: remove the extra '.'.

12. +  /**
    +   * Gets the security coverage requirement if any.
    +   */
    +  public function getRequirement() {
        

    Document the return value. The same comment as on getVersionEndRequirement() and getDateEndRequirement() would be fine.

13. +  public function getRequirement() {
    +    if ($this->projectData['project_type'] === 'core' && $this->projectData['name'] === 'drupal') {
    +      if (!empty($this->projectData['security_coverage_info'])) {
    ...
    +      }
    +    }
    +    return NULL;
    +  }
        

    I would rather have two early return NULL; lines.

14. Can you rearrange the helper functions in a more logical order? After the public getRequirement() refers to getVersionEndRequirement(), I expect to see that function next. Instead, it is at the end of the file.

15. +        $requirement['title'] = $this->t('Drupal core security coverage');
    +        if (isset($this->projectData['security_coverage_info']['support_end_version'])) {
    +          $requirement += $this->getVersionEndRequirement();
    +        }
    +        elseif (isset($this->projectData['security_coverage_info']['support_end_date'])) {
    +          $requirement += $this->getDateEndRequirement();
    +        }
        

    Maybe pass $requirement by reference? It would save a couple of lines in each of the helper functions and make this snippet a little simpler.

16. +   * @param string $minor_version
    +   *   The installed minor version.
    ...
    +   */
    +  private function getVersionNotSupportedMessage($minor_version) {
        

    Since this method is called with the argument "$major.$minor", I would call the variable $version.

17. +  private function getDateEndRequirement() {
    +    $requirement = [];
    +    list(, $minor_version) = explode('.', $this->projectData['existing_version']);
    +    $current_minor = "{$this->projectData['existing_major']}.$minor_version";
        

    Same comment as in #6: shouldn't we get both major and minor versions from $this->projectData['existing_version']? Also, I think $current_version would be a clearer variable name than $current_minor since it includes both major and minor versions.

    We might even consider making all of these (major, minor, combined) into class properties, set in the constructor.

18. +  private function getDateEndRequirement() {
    ...
    +    /** @var \Drupal\Component\Datetime\Time $time */
    +    $time = \Drupal::service('datetime.time');
    +    $request_time = $time->getRequestTime();
    ...
    +      /** @var \Drupal\Core\Datetime\DateFormatterInterface $date_formatter */
    +      $date_formatter = \Drupal::service('date.formatter');
        

    I see in the last few comments that you have been struggling with whether to inject the service container. Calling \Drupal::service() from OO code is discouraged, but I am not sure what the best solution is.

19. +    if ($end_timestamp <= $request_time) {
    ...
    +      $requirement['severity'] = SystemManager::REQUIREMENT_ERROR;
    ...
    +    else {
    ...
    +      $requirement['severity'] = SystemManager::REQUIREMENT_WARNING;
    ...
    +    }
        

    It looks as though getDateEndRequirement() always returns an error or a warning. Is this what we want? What if the installed version is the latest available?

@benjifisher thanks for the through review!

1. Added the documentation projectData in both classes and for releases
2. There is exception for control structures
   

   Control structure conditions may exceed 80 characters, if they are simple to read and understand:

   I think it is readable the way it is.

3. I share your dislike for this function.

   If the doc block described the existing keys, then adding the two new ones would be in scope for this issue. Maybe it is in scope for this issue to describe all the keys.

   But it adds by my count 14 keys title, link, status, extra, reason, project_status, fetch_status, also, releases, latest_version, dev_version, recommended, security updates, latest dev
   And some of these have sub-keys. Documenting all of those would be a major amount of work.

   I think we should create a follow up to either, 1) document all added keys, or 2) Create class and split the logic into multiple methods that each add a set of keys that are related. But probably before we did that we should make sure we have very comprehensive test coverage of the existing functionality so didn't accidently change anything.

   +++ b/core/modules/update/update.compare.inc
   @@ -296,6 +298,7 @@ function update_calculate_project_update_status(&$project_data, $available) {
   +      $project_data['existing_release'] = $release;
   

   I made a change to \Drupal\update\ProjectSecurityRequirement::getVersionEndCoverageMessage() which is the only place we read this so we don't actually need this change. I figure with the amount of keys this function already adds we should avoid adding more than we need to.

   Added a comment in the docblock to detail the remaining 'security_coverage_info' key.

   Same nit as in #2 (but leave it as is if you disagree): I would like to see the long line broken up.

   Changed to

   $security_data = new ProjectSecurityData($project_data, $available['releases']);
     $project_data['security_coverage_info'] = $security_data->getCoverageInfo();

   The first line is a couple chars over 80 but I think this is fine the standards mention exceptions for longer var names.
   I would rather have the var name be more descriptive then hit 80 exactly.

4. fixed
5. whoops. thought I fixed that. Fixed

6. I think that $projectData['existing_major'] is the major version of Drupal that a project (in this case, Drupal core itself) is compatible with.

   No this is the major version of the project itself. See update_process_project_info()

   // Figure out what the currently installed major version is. We need
   // to handle both contribution (e.g. "5.x-1.3", major = 1) and core
   // (e.g. "5.1", major = 5) version strings.
   $matches = [];
   if (preg_match('/^(\d+\.x-)?(\d+)\..*$/', $info['version'], $matches)) {
   $info['major'] = $matches[2];
   }

   At the end of the function it is assigned to 'existing_major'

   But we can as you point out get this info from $this->releases[$this->projectData['existing_version']]. So this avoids exploding the string here I think it is good.

   Also now have to add

   if (empty($this->releases[$this->projectData['existing_version']])) {
         // If the existing version does not have a release we cannot get the
         // security coverage information.
         return [];
       }

   Otherwise the test would have an exception -dev version don't have a release. They were already not getting a message because version_extra would not have been empty.
   Fixed.

   UPDATE: this actually exposed problem with the tests.
   core/modules/update/tests/modules/update_test/drupal.sec.9.0.xml
   didn't actually have a 8.8.0 release but it was being used to test sites being on 8.8.0. Because before it was using 'existing_version' we didn't actually check if the release existed. fixed this.

7. fixed
8. Added but removed version_patch because this never used this is private method.
9. fixed
10. NIce, much more concise. Fixed
11. Fixed
12. Fixed
13. I added 1 early return and realized I really didn't need
    if (!empty($this->projectData['security_coverage_info'])) {

    Because the next 2 ifs actually check if 2 other keys are set under 'security_coverage_info'. If they aren't it doesn't matter if security_coverage_info is empty or not. so return NULL after that.

    I think your intention was to get rid of the all code being nested in 2 ifs. which this does.

14. I looked at the order and I think just the one mentioned needs reordering. Fixed.
15. I rearranged the code in this block earlier in 13) here to not use += and not assign title until we are sure we have a requirement. Otherwise it looked like you could return a requirement with just a title. so I think passing by reference doesn't make sense anymore
16. Fixed and updated the comment.
17. Fixing here we don't have releases though so slightly different fix.
    

    We might even consider making all of these (major, minor, combined) into class properties, set in the constructor.

    I made existingMajor and existingMinor but then say were used multiple times as "{$this->existingMajor}.{$this->existingMinor} and the only other place to figure out the next minor.

    So I just made class properties existingVerion and nextVersion.

18. See @tim.plunkett's comment in #90.4 and my response in #92.4

    I think this better that using ContainerInjectionInterface which would get rid of \Drupal::service() but also not allow setting the project data in the constructor.

    I think the current solution in the lesser of 2 evils.

19. I am not sure about this. When I talked to @xjm she said that she didn't want to change the message with 8.9 lts as we got closer to ending support. So in this case I don't think we want to change from info to warning if we aren't changing the message.

    I don't think using info for the whole LTS period seems correct so I choose warning.

    If are using warning for the whole LTS I think we should also for 8.8 which only supported a year.

    My personal opinion is that we should change the message 6 months before LTS ends and change from info to warning.

Only nits left. The last couple interdiffs have been great.

1. +++ b/core/modules/update/src/ProjectSecurityData.php
   @@ -0,0 +1,167 @@
   + * @internal
   

   I think this is supposed to explain _why_ it's internal (here and elsewhere)

2. +++ b/core/modules/update/src/ProjectSecurityData.php
   @@ -0,0 +1,167 @@
   +   * @var array
   ...
   +   * @var array
   

   Can these be string[]? Not essential, just know that it might get asked by a committer.

3. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -0,0 +1,239 @@
   +      $translation_arguments = [
   +        '%project' => $this->projectData['name'],
   +        '%version' => $this->existingVersion,
   +        '%coverage_version' => $security_info['support_end_version'],
   +      ];
   +      $message = '<p>' . $this->t('The installed minor version of %project, %version, will receive security updates until the release of %coverage_version.', $translation_arguments) . '</p>';
   ...
   +      $requirement['description'] = '<p>' . $this->t(
   +          'The installed minor version of %project, %version, will receive security updates until %date.',
   +          [
   +            '%project' => $this->projectData['name'],
   +            '%version' => $this->existingVersion,
   +            '%date' => $date_formatter->format($end_timestamp, 'html_date'),
   +          ]
   +        ) . '</p>';
   

   The first one is SO much more legible than the second one... But that's just a nit.

1. fixed.
2. I don't think so. The keys in the array that we using are all strings but there so many places these get processed along the way. Some of the value are arrays themselves and with alter hook we can't guarantee anything.
3. legibility is good. Fixed

Only nits left. The last couple interdiffs have been great.

I agree. We are getting close!

1. Document the types (string and int) of the array returned by getCoverageInfo(). If you do not, then someone will inevitably ask whether @return array can be changed to @return string[]. ;)

2. It still seems odd to me that the LTS version is always marked as a warning.

My personal opinion is that we should change the message 6 months before LTS ends and change from info to warning.

I agree.

      if (isset($security_info['support_ending_warn_date']) && \DateTime::createFromFormat('Y-m-d', $security_info['support_ending_warn_date'])->getTimestamp() <= $request_time) {
        $requirement['description'] .= '<p>' . $this->t('Update to a supported minor version soon to continue receiving security updates.') . '</p>';
      }

This seems like the right time to use SystemManager::REQUIREMENT_WARNING instead of SystemManager::REQUIREMENT_INFO. But if you have already discussed it with @xjm, then I guess it is settled.

I have not looked at the tests at all, and I also want to do some manual testing.

1. fixed
2. @xjm is this what you meant we talked about this? Or should we use info the whole time or info and then warning at 6 months till end of LTS

@xjm yeah sorry that is not what I meant.

When I talked to @xjm she said that she didn't want to change the message with 8.9 lts as we got closer to ending support. So in this case I don't think we want to change from info to warning if we aren't changing the message.
I don't think using info for the whole LTS period seems correct so I choose warning.

I few questions

1. Should we change the message if a site is on LTS and if it is 6 months until the LTS period is over? My memory is you said no. Currently in the patch all other releases get additional message

   +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -0,0 +1,240 @@
   +        // If the installed minor version will only be supported for 1 newer
   +        // minor core version encourage the site owner to update soon.
   +        $message .= '<p>' . $this->t('Update to %next_minor or higher soon to continue receiving security updates.', ['%next_minor' => $this->nextVersion])
   +          . ' ' . static::getAvailableUpdatesMessage() . '</p>';
   +      }
   

   For versions that are supported for 2 minor version we warn when they are 1 minor version away from not being supported(roughly 6 months)

   +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -0,0 +1,240 @@
   +      if (isset($security_info['support_ending_warn_date']) && \DateTime::createFromFormat('Y-m-d', $security_info['support_ending_warn_date'])->getTimestamp() <= $request_time) {
   +        $requirement['description'] .= '<p>' . $this->t('Update to a supported minor version soon to continue receiving security updates.') . '</p>';
   +      }
   

   For 8.8.x which is supported for 1 year, not directly tied to other minor releases, we warn when support is 6 months from ending.

   If we follow this pattern we say "Update to a supported minor version soon to continue receiving security updates." when the LTS release is 6 months from ending.

   Of course we may want to change this message because there will be no more minor releases of Drupal 8(probably). Though we could leave it as 9.0.0 is a minor release.

2. If we don't want to add an extra ""Update to a supported...." message should the message still change from the info to warning section at 6 months?
3. if we don't want to change the message section from info to warning within 6 months if we aren't adding an extra message do we want always have the message be an Info message right up till the day before they lose support.

I wanted to address some questions in #109 but I found a testing error while I was looking at it. So fixing that first and then will come back to #109

1. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -345,6 +345,295 @@ public function securityUpdateAvailabilityProvider() {
   +      $assert_session->elementExists('css', "div:contains('$requirements_section_message') summary:contains('Drupal core security coverage')");
   

   I realize this is not testing what I thought it was.

   What I wanted to check was, make sure Drupal core security coverage was under the same div as the heading, $requirements_section_message('Warnings found', 'Errors found').

   What it was actually checking: Assert that there is at least 1 div that contains "Warnings found" and also contains a summary that contains Drupal core security coverage.

   Of course the top level div of the page would satisfy this regardless of whether "Warnings found" and Drupal core security coverage were under the same section.

   This can't be done 1 elementExists() call which is fine. This patch breaks it up into multiple asserts with comments.

   There are couple cases where $requirements_section_message was "Errors found" when it should have been "Warnings found". This was only passing because there was another message under "Errors found". In my case locally "Trusted Host Settings" I assume this is the same on Drupalci.
   I am also going to check $requirements_section_message

2. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -345,6 +345,295 @@ public function securityUpdateAvailabilityProvider() {
   +   * @param string $requirements_section_message
   +   *   The requirements section method.
   

   I am going to change this parameter name to $requirements_section_heading to be more clear.

3. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -345,6 +345,295 @@ public function securityUpdateAvailabilityProvider() {
   +        'requirements_section_message' => 'Warnings found',
   

   Also changing all of these to be 'requirements_section_heading'

4. I am including a patch that fixes the test assert but does not fix the "Errors found" -> "Warnings Found" to prove these test cases would fail.

Weird my patches didn't upload. Maybe because I left the tab open overnight?

Well anyways here they are 🕺

wow I am nailing it today 😜

1. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -350,6 +350,301 @@ public function securityUpdateAvailabilityProvider() {
   +      file_put_contents('/Users/ted.bowman/Sites/www/test.html', $this->getSession()->getPage()->getOuterHtml());
   

   Whoops. removing.

+++ b/core/drupalci.yml
@@ -3,57 +3,13 @@
+        testgroups: '--class "Drupal\Tests\update\Functional\UpdateCoreTest::testSecurityCoverageMessage"'


for interdiffs see #110