Update report incorrectly recommends security releases for old minors when a security update is needed and a secure version of the old minor is also available

Downgrades should never be shown as a Security update.

Well, I would say "Downgrades within the same major release should never be shown as security updates." There are times that an older security release really could be the only supported option, if newer releases have been marked insecure/revoked/etc. without newer security releases being created. This also applies to the case where the site is on 8.x-3.0-beta2 of a module and the most recent security release is 8.x-2.2. In that scenario, 2.2 really is the most recommended version for a stable site.

This is at least major and possibly critical because of its relationship to #2990476: If the site is on an insecure version of an old minor and there is a secure version of that old minor available, the update status report should link that release. Bumping to critical for now.

I think I just ran into another example of this in the real world.

I'm trying to test #3114707: Add color to summary element to improve UX of core compatibility info on available updates report and I searched for a contrib that's got a requirement on 8.8.x core and a security update required. I found VBO (side note, see #3116581: Use core_version_requirement, not a version dependency on drupal:views).

Right now with a clean 8.9.x install of core, if you install VBO 8.x-3.3, your available update report will look like this:

This is the same bug, right?

I re-rolled #7 (no longer applies), and now my available updates report for VBO looks as expected:

Not entirely clear what else is needed here. I'll more closely review when I can and see if there's any reason not to RTBC this.

Thanks,
-Derek

p.s. Interdiff is all kinds of confused by the re-roll, so here's a raw diff of the two patch files.

p.p.s. EDIT: changed the embedded screenshots to use the ones from comment #10.

Sorry for the noise, but my test site was a bit screwy when I took those screenshots. Needed to clear caches to get the Download links. ;) These are the accurate screenshots.

Initial review:

1. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -357,24 +365,25 @@ public function securityUpdateAvailabilityProvider() {
   +      // If the site is on an alpha/beta/RC of an upcoming minor and there is
   +      // RC versions with a security update it should be recommended.
   

   Comment grammar doesn't parse. Should be:

   // If the site is on an alpha/beta/RC of an upcoming minor and there is
   // an RC version with a security update, it should be recommended.

2. +++ b/core/modules/update/update.compare.inc
   @@ -10,6 +10,8 @@
    use Drupal\update\ModuleVersion;
    use Drupal\update\ProjectCoreCompatibility;
    
   +use Drupal\Component\Version\Constraint;
   +
   

   I botched the spacing on the reroll. I think Component should go first, without a newline between it and the others...

3. +++ b/core/modules/update/update.compare.inc
   @@ -473,7 +475,14 @@ function update_calculate_project_update_status(&$project_data, $available) {
   +      // Remove the major version prefix, for example '8.x-', if any.
   +      $constraint_testable_version = str_replace(\Drupal::CORE_COMPATIBILITY . '-', '', $release['version']);
   

   We shouldn't use str_replace() for this, or we screw up things like 8.x-8.x-dev releases. We should be more careful and do a preg_replace() anchored to the start of the string. Or something...

Bigger picture... I'm confused why we have this bug at all. Once upon a time, update_calculate_project_update_status() would stop searching the release history as soon as it found your currently installed release. So I'm not sure how it gets to this state where it's finding security releases that are older than your installed release. I'd have to dig a lot deeper to figure out why that changed. But it makes me nervous that there might be other bugs lurking if update_calculate_project_update_status() keeps processing releases that are older than what you've got...

Yeah, weird, it's still there:

    // Stop searching once we hit the currently installed version.
    if ($project_data['existing_version'] === $version) {
      break;
    }

So, yeah, needs further investigation to understand why this patch is needed at all. ;)

p.s. #3085662: Remove Drupal::CORE_COMPATIBILITY because it is not accurate when modules can be compatible with multiple core branches -- sad to keep adding usages of \Drupal::CORE_COMPATIBILITY

I have updated patch wrt #11, point 1 and point 2.

I'm looking into the "bigger picture" from #10. Stay tuned...

Ok, here's the deal...

B/c of this:

      // Otherwise, ignore unpublished, insecure, or unsupported releases.
      if ($release['status'] == 'unpublished' ||
          !$is_in_supported_branch($release['version']) ||
          (isset($release['terms']['Release type']) &&
           (in_array('Insecure', $release['terms']['Release type']) ||
            in_array('Unsupported', $release['terms']['Release type'])))) {
        continue;
      }

if your currently-installed version is one of those conditions (e.g. insecure), then we never hit this code block later:

    // Stop searching once we hit the currently installed version.
    if ($project_data['existing_version'] === $version) {
      break;
    }

So this function loops over every possible release. :( In most cases, we skip things that are older, but not for security updates. That's what the fix in #7 is addressing -- special-case the security release stuff to ignore older releases.

IMHO, the better solution is to not skip the current version, even if insecure, so that we properly bail out of this function when we mean to.

I first tried adding the break; at the end of the initial code block that notices if we're on the current release:

    // First, if this is the existing release, check a few conditions.
    if ($project_data['existing_version'] === $version) {
      ...
    }

However, if we break that soon, then we never set recommend or latest_version, and if you've got a module that's up-to-date (current should be recommended) eventually we hit this:

  // If we don't know what to recommend, there's nothing we can report.
  // Bail out early.
  if (!isset($project_data['recommended'])) {
    $project_data['status'] = UpdateFetcherInterface::UNKNOWN;
    $project_data['reason'] = t('No available releases found');
    return;
  }

So that's no good, either...

Therefore, I think the smallest, safest fix for this bug (which also prevents us from needlessly looping over a ton of releases we don't care about) is the attached patch. We only do the "otherwise, ignore stuff..." code block in an else of testing if we're looking at the current release.

Identical test changes from #7. In fact, attaching those as a test-only patch so we see the failures from the new test coverage here. Fails locally. With the full patch, all update tests are passing locally. I hope the bot agrees.

Interdiff is pretty meaningless, since it's an entirely different approach to the changes for update.compare.inc.

p.s. @swatichouhan012 re: #12: thanks for helping. Unfortunately you didn't fix #11.2 as intended. Thankfully, it doesn't matter since this new approach doesn't need any new use statements at all. ;)

p.p.s. Adding #3100110: Convert update_calculate_project_update_status() into a class as related. I'll be the first to admit that this function is way too long and complicated for anyone other than the original author (me) to really understand all the subtleties and nuances for the kind of analysis / fix I just did. :( Yes, we should re-factor / re-organize it into a class and make it easier for other people to make sense of.

Self-review:

+++ b/core/modules/update/update.compare.inc
@@ -379,14 +379,15 @@ function update_calculate_project_update_status(&$project_data, $available) {
+    else {
+      // Otherwise, ignore unpublished, insecure, or unsupported releases.
+      if ($release['status'] == 'unpublished' ||
+          !$is_in_supported_branch($release['version']) ||
+          (isset($release['terms']['Release type']) &&
+           (in_array('Insecure', $release['terms']['Release type']) ||
+            in_array('Unsupported', $release['terms']['Release type'])))) {
+        continue;
+      }

This can be elseif(), then we don't have to indent it all. Although we'd still have to change the indentation of all the conditions for if vs. elseif. :/ So it'd only save the indentation of the continue and would eliminate the need for the extra }. ;)

Wish we had a better standard for multi-line complex conditionals like this. Sadly #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines wouldn't help and we'd still need to indent the conditions differently to account for if vs. elseif. /shrug

Meanwhile, I believe this is back-portable to 8.8.x, so moving the version selector accordingly. Patch still applies cleanly to 8.8.x and 9.0.x, too.

#17 was just a random fail, but I realized I uploaded the wrong version of the formatting I was aiming for, anyway.

Re: #19: 👍

How's this?

See also my latest thoughts at #3100110-21: Convert update_calculate_project_update_status() into a class

9.1.x was random fail in Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest -- requeued.

$ composer require drupal/metatag:1.5.0 drupal/profile:1.0.0-rc1 drupal/views_bulk_operations:3.3.0 -vvv

Manually tested the patch in #18 against 8.9.x#225d565b9060b0f7357d5fd3232d90fd5d7e766b

The patch works as expected, RTBC +1.

Meanwhile, I am thinking of the following

1. Change Recommended version: to Recommended version by maintainer. To me, if I am using a branch, recommended version from the same branch would make more sense. Take the screenshot in #22 as an example, I was using Drupal core 8.9.x-dev, why recommended me 8.8.4. I'd like 8.9.0-beta1 being recommended rather than 8.8.4.

   To distinguish the recommended version by the maintainer and by the update system is unnecessary. But we could make it more clear that it's by the maintainer.

2. Should we display all releases available which > current version? Take the VBO module in #22 as example, the current version is 3.3.0 and the latest version is 3.6.0. My point is that version 3.4.0, 3.5.0 and 3.6.0 should be listed too. With all available releases listed, it may push/encourage people to do the update. It looks like saying that, hey, site owner, you are too far away from the latest version.

3. Email notification threshold

   • All newer versions
   • Only security updates

   I think we should introduce a filter or setting which similar to the Email notification threshold Under the Update Manager settings

   To allow end-users care about Stable releases only, or RC/beta/alpha/dev releases are ok. The use case is that some people only care about the stable releases, some people may care of RC, beta, even alpha releases. We should provide such an option.

Of course, all the above can be done in separate issues if they will be proceeding

LGTM; tests seem to be passing and I can't find anything wrong with the proposed changes.

Committed e1b9321 and pushed to 9.1.x. Thanks!

I'm going ping the release managers about backport.

This looks okay to me. Hard t review but it makes sense once I groked what

    // Stop searching once we hit the currently installed version.
    if ($project_data['existing_version'] === $version) {
      break;
    }

was doing.

The code is definitely over-complex and hard to review / test. So it's great that our coverage is improving.

Sweet, thanks! FYI: #20 applies cleanly all the way back to 8.8.x branch, so this should be cherry-pickable and doesn't require any new patches.

Cheers,
-Derek

Discussed with @catch and we agreed to backport this all the way back to 8.8.x

Sweet, thanks!