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
| Comment | File | Size | Author |
|---|---|---|---|
| #22 | 3252190-22-branch.png | 94.23 KB | phenaproxima |
| #22 | 3252190-22-9.4.x.png | 83.81 KB | phenaproxima |
Issue fork drupal-3252190
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
Comment #2
tedbowComment #6
tedbowComment #7
tedbowWe 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 have8.0.1because this is insecure.Comment #9
kunal.sachdev commentedComment #10
tedbowSetting to needs work based on Merge request comments
Comment #11
kunal.sachdev commentedComment #12
tedbow@kunal.sachdev thanks. very close now. Just a few more points
Comment #13
kunal.sachdev commentedComment #14
tedbowThanks @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.
Comment #15
phenaproximaThis 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.
Comment #16
kunal.sachdev commentedComment #17
phenaproximaI think this is really close to done; just needs a few relatively simple comment changes in the kernel test.
Comment #18
phenaproximaNo objections. In my view, this is good to go when tests pass.
Comment #19
dwwI 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
Comment #20
dwwPer Slack chat w/ Ted + Adam, tagging this for some profiling before commit.
Comment #22
phenaproximaDid 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:
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:
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.
Comment #23
tedbow@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-beta1relating #3274269: Ensure all installable releases are exposed because since that issue we working around Automatic Updates in contrib
Comment #25
nod_Setting need work per #23 to profile with modules having a long release history.