Problem/Motivation

Right now the Update module will always recommend the latest version in the current major that is installable, meaning published, secure, in a supported branch and support itself.

though there are some bugs, like #3227518: Never show a "Not supported!" version as "Recommended"

This is going to be problem for Automatic Updates when it is in core because the MVP will only support patch release not upgrading to different minor. see #3252187: [Meta] Deal with core Update module recommending the latest release in the major version not the latest in the Minor version

For example if:

  • the site is on Drupal core 9.5.1
  • 9.5.x and 9.6.x are the 2 supported branches of core
  • The latest release in each branch is 9.5.2 and 9.6.2
  • Then the UPdate module will use 9.6.2 as the "Recommended version"
  • This will not be supported by the MVP for Automatic Updates because this would be updating to different minor. We will only support patch updates

I am setting to major priority because this blocks the core Auto UPdates initiative from being able to recommend patch releases for any sites that are on a support minor that is not the most recent. Therefore it would stop Auto Updates from enabling cron automatic updates for a year on 1 branch

This also stops any site that is on the current minor from knowing about any patch release except the most recent. So if we restrict cron updates to only 1 patch level update at a time if you fell behind for some reason cron would not be able to get you to the latest patch release by updating incrementally over multiple cron runs.

Steps to reproduce

currently based on https://updates.drupal.org/release-history/drupal/current

If a site where on 9.1.13 the Update would not recommend 9.1.14. It would recommend 9.2.10. You can test by faking the drupal version Drupal.php

Proposed resolution

in the long term it would be good if the Update module recommend a version in the current minor and then in any other minor that is supported(for core that will usually be 2) in the same major. But this will major need refactoring of the Update reports.

For now we should simple change update_calculate_project_update_status() to store all the info about installable releases in $project_data which it modifies. This will allow other modules like Automatic Updates in contrib and in the core version which should be started soon to pick a version in the current minor that we know is secure and supported.

update_calculate_project_update_status() already has all the information it needs to do this.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3252190

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow’s picture

Status: Active » Needs work
Issue tags: +Needs tests
Related issues: +#3100110: Convert update_calculate_project_update_status() into a class

We needs tests to prove the extra releases are in the array and that releases that are secure , published etc are NOT in the array.

Since this will not affect the report this should probably be a kernel test. See this automatic updates base kernel test for how to set the core version and set the metadata files in kernel test https://git.drupalcode.org/project/automatic_updates/-/blob/8.x-2.x/test.... Should be able to copy code from there

Look at the issue #3100110: Convert update_calculate_project_update_status() into a class . I wrote kernel test there. The test is CompareOldNew. Here we could need to assert that $projects[$project_name]['releases'] had all of the releases in the XML that were not insecure, revoked, etc. We would also to make sure it didn't have any of insecure releases.

For example using the xml core/modules/update/tests/fixtures/release-history/drupal.sec.0.2.xml $projects[drupal]['releases'] should not have 8.0.1 because this is insecure.

kunal.sachdev made their first commit to this issue’s fork.

kunal.sachdev’s picture

Status: Needs work » Needs review
tedbow’s picture

Status: Needs review » Needs work

Setting to needs work based on Merge request comments

kunal.sachdev’s picture

Status: Needs work » Needs review
tedbow’s picture

Status: Needs review » Needs work

@kunal.sachdev thanks. very close now. Just a few more points

kunal.sachdev’s picture

Status: Needs work » Needs review
tedbow’s picture

Issue summary: View changes
Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

Thanks @kunal.sachdev. Looks good now!
I am changing to major priority because this blocks the core Auto UPdates initiative from being able to recommend patch releases for any sites that are on a supported minor that is not the most recent. Therefore it would stop Auto Updates from enabling cron automatic updates for a year on 1 branch and would only allow 6 months of updates.

This also stops any site that is on the current minor from knowing about any patch release except the most recent. So if we restrict cron updates to only 1 patch level update at a time if you fell behind for some reason cron would not be able to get you to the latest patch release by updating incrementally over multiple cron runs.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests

This looks pretty good, and it's clear. However, I did find a few things in my review, so kicking back for that.

Also, this has test coverage so I'm removing the "needs tests" tag.

kunal.sachdev’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

I think this is really close to done; just needs a few relatively simple comment changes in the kernel test.

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

No objections. In my view, this is good to go when tests pass.

dww’s picture

I haven't closely reviewed the code here. But just going on the issue summary and my knowledge of the inner workings of both update_calculate_project_update_status() and updates.d.o release history feeds, my "holy RAM usage, batman" flags are going off.

Once upon a time, we put quite some effort into Update Manager to avoid keeping all of the release history feed in RAM. Now that we have stopped doing major-version-specific feeds, and are doing the "/current" feeds, the XML is going to grow indefinitely. By design, we're keeping a huge number of stale releases in the same feeds, since a given contrib might be compatible with N different major versions of core.

I was already concerned about the /current thing when we rolled it out, and that we didn't seem to have a plan for mitigating the resource impact.

Now it sounds like this issue is saying "we could really use more of that stuff in a few cases, let's store it all in RAM".

While that might not immediately explode our test bots or the local dev boxes where we're trying this on a small, imaginary use case, what's going to happen if we release this in core and folks with 200 contrib modules try to visit their available updates report? Are we going to need 2 gigs of RAM to prevent a PHP fatal? Or more?

I'm tempted to tag this for "Needs profiling" and set back to NR, but I'm not sure I want to get that far. Curious to hear what Adam and Ted think of this.

I could also, you know, actually look at the MR and see what's happening. ;) Explosive end-of-sprint at $day_job permitting, I'll do that ASAP. But I wanted to raise this concern before a committer hits this and decides to merge it.

Thanks/sorry,
-Derek

dww’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +needs profiling

Per Slack chat w/ Ted + Adam, tagging this for some profiling before commit.

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

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

phenaproxima’s picture

StatusFileSize
new83.81 KB
new94.23 KB

Did some light profiling of this on my local machine. I'm running PHP 8.1.7 using Laravel Valet, which is a wrapper around nginx and php-fpm. I installed core from 9.4.x HEAD, Standard profile, no additional modules; when I checked for available updates, here's what I got in the first run:

Profiling results with 9.4.x HEAD

Now, I don't precisely know how to read this, but it looks like over all runs of update_calculate_project_update_status(), where most of the effort was made, PHP ended up using about 70 MB of memory. Again, that's with 9.4.x HEAD.

I then applied the diff from the merge request and tried again. These were the results:

Profiling results with the merge request applied

Looks like we used about 77 MB of memory that time.

So that's not an unreasonably large increase, by itself. But as far as I know, it was only checking for updates to Drupal core, since that's the only project which was enabled. If I'd had 100 modules enabled, it would have been a whole lot more. But then again...this was a batch job so, as far as I can tell, we're not doing all of this work in a single request.

All of this leads me to the (initial) conclusion that, even though this change does cause a reasonable, but noticeable, increase in memory usage, it's not a problem as long as we're not checking for updates in all installed projects during a single request.

tedbow’s picture

@phenaproxima thanks for profiling this. It might be good to test with a couple contrib projects in enabled that have a very long history where you change the info.yml files to set version to the very earliest version that is compatible with core >8 so the last release in something like this https://updates.drupal.org/release-history/webform/current would be 8.x-5.0-beta1

relating #3274269: Ensure all installable releases are exposed because since that issue we working around Automatic Updates in contrib

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

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

nod_’s picture

Status: Needs review » Needs work

Setting need work per #23 to profile with modules having a long release history.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone made their first commit to this issue’s fork.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.