If no recommended update is found, Update Status recommends the latest release, even if it is unsupported

Let's see if we have explicit test coverage that expects the current behavior

A) Close this as a works-as-designed support request.

I agree this might be the best answer. If so I think we should then repurpose this issue to just add test coverage for the expected behavior. I think #2 passing shows we don't have test coverage for this "design".

Here I think is the correct fix

we shouldn't recommend it if it is not in a supported branch

There is only 2 places we set $project_data['recommended'] lets add this check there too.

The test failed because the xml core/modules/update/tests/modules/update_test/aaa_update_test.8.x-1.2.xml did not have supported_branches.

Originally the test passed because of

  else {
    // Malformed XML file? Stick with the current branch.
    $target_major = $existing_major;
  }

So since supported_branches wasn't set we hit this fallback.

Other XML test fixtures are malformed some for a long time a few from recent changes. I opened #3110917: [meta] Fix update XML fixtures bad data recently

Running #11 on 8.9.x too. It should pass and be committed to each branch

The issue needs to update. The real problem is recommended a non-supported branch. Besides the dev problem this won't be problem for core. It won't be problem for contrib until Semantic releases

removing this from the summary but if someone this is important than we should open another issue.

Decide if this is actually ever valid recommendation.
For instance:

1. There is contrib module and there is 8.x-1.x branch but no releases on the branch
2. The developer realizes the approach is wrong and starts a 8.x-2.x branch(maybe people are using 8.x-1.x and was asked to keep it for them)
3. The developer hasn't released any releases on 8.x-2.x releases either

In this case should 8.x-2.x-dev actually be recommended? Otherwise how the developer know that there is a 8.x-2.x branch at all. Maybe they would continue to use 8.x-1.x-dev and write custom code against it's API waiting for the 8.x-1.0 release.

It seems in that case maybe 8.x-2.x should be "also available"

1. Added 2 other checks for $is_in_supported_branch() when update_calculate_project_update_status() is setting the latest_version and setting dev_version. We should not be showing any download links to versions that are unsupported branches.

   For instance in core currently https://updates.drupal.org/release-history/drupal/current
   <supported_branches>8.7.,8.8.,8.9.,9.0.</supported_branches>
   So our future development branches are considered supported.

2. I updated the XMLtest fixtures that \Drupal\Tests\update\Functional\UpdateCoreTest::testNormalUpdateAvailable() uses for the cases where the expected recommend release is in the 8.0.x minor branch. I added an extra release in the 8.1.x branch.

   For some reason(probably my copy/paste error in #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates) in the XML it previously had <supported_branches>8.0.,8.1.</supported_branches> even there are no 8.1.x releases in the XML. I remove move 8.1.

   Therefore the add 8.1.0 release should be ignored by the test because it is not in a support branch.

3. I also updated the XML test fixtures for \Drupal\Tests\update\Functional\UpdateContribTest::testNormalUpdateAvailable that expect a 8.x-1.x release and added 8.x-2.x release. It should also be ignored.
4. I am adding

Here is simpler change. It just tests each release to see if it is in a supported branch if it is not then it goes to the next release.

changing to critical because the problem involves the user seeing downloads that aren't in supported branches.

This solution is the not the full solution to #3100115: The update module should recommend updates based on supported_branches rather than major versions majors because the user would still be shown and recommend the most recent full release in major and not a support branch. But I believe this issue is the critical part of #3100115

@tim.plunkett thanks for the review

1. Still needs tests though. NW for that.

   +++ b/core/modules/update/tests/modules/update_test/drupal.0.0-alpha1.xml
   @@ -3,13 +3,27 @@
   +  <release>
   +    <!-- This release is not in a supported branch; therefore it should not be recommended. -->
   +    <name>Drupal 8.1.0</name>
   +    <version>8.1.0</version>
   +    <tag>DRUPAL-8-1-0</tag>
   +    <status>published</status>
   +    <release_link>http://example.com/drupal-8-1-0-release</release_link>
   +    <download_link>http://example.com/drupal-8-1-0.tar.gz</download_link>
   +    <date>1250425521</date>
   +    <terms>
   +      <term><name>Release type</name><value>New features</value></term>
   +      <term><name>Release type</name><value>Bug fixes</value></term>
   +    </terms>
   + </release>
   

   So by adding this new release which is not in supported branch it adds test coverage that this release is ignored in \Drupal\Tests\update\Functional\UpdateCoreTest::testNormalUpdateAvailable()

   This is what I was trying to explain in #15.2 but obviously this is not super clear. I thought about just adding a comment about this but I think it is clearer to assert the release is name is nowhere on the page.

   I switched this 8.2.0 which is also not in a supported branch and would be recommended if it were in a supported branch since it is most recent release for the major.

   Also because 8.2.0 is newer than all expected release in testNormalUpdateAvailable it can be added to all the fixtures used by this test method.

   In addition I am adding an explicit assert and comment to explained why this is not being shown on the available updates.

   // The XML test fixtures for this method all contain the '8.2.0' release
   // but because '8.2.0' is not in a supported branch it will not be in
   // the available updates.
   $this->assertNoRaw('8.2.0');
   

   I did the same for the contrib version.

2. +++ b/core/modules/update/tests/modules/update_test/drupal.0.0-alpha1.xml
   @@ -3,13 +3,27 @@
   +  <release>
   +    <!-- This release is not in a supported branch; therefore it should not be recommended. -->
   +    <name>Drupal 8.1.0</name>
   +    <version>8.1.0</version>
   +    <tag>DRUPAL-8-1-0</tag>
   +    <status>published</status>
   +    <release_link>http://example.com/drupal-8-1-0-release</release_link>
   +    <download_link>http://example.com/drupal-8-1-0.tar.gz</download_link>
   +    <date>1250425521</date>
   +    <terms>
   +      <term><name>Release type</name><value>New features</value></term>
   +      <term><name>Release type</name><value>Bug fixes</value></term>
   +    </terms>
   + </release>
   

   So by adding this new release which is not in supported branch it adds test coverage that this release is ignored in \Drupal\Tests\update\Functional\UpdateCoreTest::testNormalUpdateAvailable()

   This is what I was trying to explain in #15.2 but obviously this is not super clear. I thought about just adding a comment about this but I think it is clearer to assert the release is name is nowhere on the page.

   I switched this 8.2.0 which is also not in a supported branch and would be recommended if it were in a supported branch since it is most recent release for the major.

   Also because 8.2.0 is newer than all expected release in testNormalUpdateAvailable it can be added to all the fixtures used by this test method.

   In addition I am adding an explicit assert and comment to explained why this is not being shown on the available updates.

   // The XML test fixtures for this method all contain the '8.2.0' release
   // but because '8.2.0' is not in a supported branch it will not be in
   // the available updates.
   $this->assertNoRaw('8.2.0');
   

   I did the same for the contrib version.

   In addition I added the same assert and comment to \Drupal\Tests\update\Functional\UpdateCoreTest::testNoUpdatesAvailable() which uses these same XML fixtures. Because this is asserting that there no available updates and 8.2.0 is newer than all test installed versions I think this is even more clear that 8.2.0 is being skipped because it is in an unsupported branch.

3. 3111929-15-no-fix-FAIL.patch I was attempting to show with the fix the release in the unsupported branch would be recommend.

   I will upload new test-only patch and with the new assert it should be clearer because the core tests will fail on the 2 new assert statements.

   The contrib version will not fail in test-only version because supported branches don't specify a minor.

4. +++ b/core/modules/update/tests/modules/update_test/drupal.0.0.xml
   @@ -3,13 +3,27 @@
   -<supported_branches>8.0.,8.1.</supported_branches>
   +<supported_branches>8.0.</supported_branches>
   

   Since we are now using the 8.2.0 release we no longer have to change these lines.

   I commented in #3110917-3: [meta] Fix update XML fixtures bad data that we really don't need 8.1. here.

5. Is there a difference between $version and $release['version']?

   No difference the releases array is keyed by version number and they contain the same in the version key.

   But I will change this to use $release['version'] because this comes more directly from the XML.

ok #21 was an unexpected fail but not because of problem with this patch with the commit in #3096078: Display core compatibility ranges for available module updates

The test that failed here Drupal\Tests\update\Functional\UpdateContribTest::testCoreCompatibilityMessage() also uses the some of the core XML test fixtures that have been update.
Basically it tests that a contrib module will display the core compatibility range

This module is compatible with Drupal core: 8.0.0 to 8.1.1

But because 8.2.0 was added it now shows

This module is compatible with Drupal core: 8.0.0 to 8.2.0

But actually because we are looking at the available updates 8.2.0 should have been ignored here also. So it was problem in #3096078 it didn't ignore it. I will comment on that issue.

I created a new issue #3112962: Core compatibility messages on contrib available updates should consider supported branches for #24

1. fixing the fail explained in #24

   I made duplicates of the drupal.1.1.*.xml files that are also used in \Drupal\Tests\update\Functional\UpdateContribTest::testCoreCompatibilityMessage()
   in the copies I didn't add the 8.2.0 release that was adding in this issue. I added a @todo to remove this files in #3112962: Core compatibility messages on contrib available updates should consider supported branches

2. re #26 I don't think it is a duplicate of #310011: Accessing cron.php from localhost. In that issue we still need to determine what is the 'recommended' release

   Basically the problem for core in that issue is:

   1. a site is on 9.0.0
   2. there is a release 9.0.1
   3. there is also a release 9.1.1.
   4. Currently the recommended release will be 9.1.1
   5. If want to recommend that latest in the current branch we would also recommend 9.0.1

   I think #310011 could be tricker than this issue it might need wording changes and discussion as to whether the above behavior is also correct for contrib once it is using semantic versioning.

   So hopefully we can get this issue sooner and make that issue smaller.

1. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -62,6 +62,11 @@ protected function setSystemInfo($version) {
   +   * The XML fixtures for this method with expected releases in the '8.0.'
   +   * branch have only '8.0.' set for 'supported_branches'. These fixtures also
   +   * contain the '8.1.0' release but because '8.1.0' is not in a supported
   +   * branch it will not be in the available updates.
   

   This comment is now wrong because now all the XML fixtures have the have new 8.2.0 release

   Removing this comment because now there is comment before the new assert that explains this.

2. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -564,18 +568,21 @@ public function testCoreCompatibilityMessage() {
   + // delight the '*-core_compatibility.xml' files in suffix in
   

   'delete' not 'delight' 😂

3. re #28. yes I think this issue can be beta blocking and #3100115: The update module should recommend updates based on supported_branches rather than major versions majors. Once this issue is fixed then we won't recommend releases in unsupported branches.

@dww thanks for the reviews and patches.

re #31
1-4 yes we should fix the problem with xml fixtures. Just copied from another release which were. i opened #3110917: [meta] Fix update XML fixtures bad data but no need to make more problems here
5. Yes I think you fix is correct and it points to the fact that we have missing test coverage. But I think the test coverage for the current problem is good just need to the missing in #3113292: Update module has no tests for changes to status of the installed release (revoked, etc)

re #43
@dww thanks for getting this started again.

1. #31.3 is probably still needed here.

   Yep, \Drupal\Tests\update\Functional\UpdateContribTest::testCoreCompatibilityMessage() will fail without this change because it will not longer be a recommended release since it is not in a recommended branch. Yes we missed it before but because of the bug this issue addresses the test passed because it was still the recommended release.

2. The fails in #29 are expected now that we have the new tests

I removed the xml @todo's as per #36.
We have

+++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
@@ -564,18 +568,21 @@ public function testCoreCompatibilityMessage() {
+    // @todo Use the drupal-1.1*.xml files without the '-core_compatibility' and
+    // delete the '*-core_compatibility.xml' files in suffix in
+    // https://www.drupal.org/project/drupal/issues/3112962.

This @todo also tell us delete the files so I think this is ok.

RE #48 Yes we still need at least 1 todo for #3112962: Core compatibility messages on contrib available updates should consider supported branches

I updated the comment to hopefully make it more clear what we need to do in that issue.

whoops here is the todo comment change I mentioned in #49

@tim.plunkett thanks for reviewing

just a reroll

@Gábor Hojtsy thanks for committing this! Thanks @webchick for finding the 🐜, thanks everyone for helping getting it done!

Yes I think we want all the related test coverage backported. I also sent it back for testing but canceled mine. The patch still applies

re me in #56

Yes I think we want all the related test coverage backported

So many issues getting committed I was confused. This is not a test-only patch.

Yes, I think this is critical to backport so that under no circumstances do we recommend and unsupported release.

Marking RTBC because previous patch applies and passes on 8.8.x se #52