Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
update.settings.yml says:
check:
disabled_extensions: false
interval_days: 1
If this value is set to 0 or lower, the logic in update_cron() pulls down update data from d.o on every cron run, for every multisite that has update module enabled.
This is a costly for d.o which has to handle these requests for all sites everywhere.
Proposed resolution
Add a usefulness check on the days-before-next-check value so that it never falls below one.
Allow arbitrary days-until-check settings to encourage even longer intervals, which means less bandwidth cost for the Drupal Association.
Document or somehow encourage users to disable update module on redundant sites, such as multisite.
Remaining tasks
There will likely be a follow-up where the update API is redesigned to be more efficient and scalable.
User interface changes
The settings form for the days-until-check setting will change from a 1 or 7 day dropdown to a numeric setting if the user has changed the value in some other way.
The user can issue a Drush command such as drush cset update.settings check.interval_days 23 and this value will then be reflected in the UI. If they change it back to 1 or 7, the dropdown will return.
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | numeric_settings.png | 197.91 KB | Mile23 |
Issue fork drupal-2853729
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:
compare
Comments
Comment #13
quietone CreditAttribution: quietone at PreviousNext commentedThis was a random bug for triage at DrupalSouth 2022. I searched core for interval_days and didn't find anything to suggest that this has been fixed. Leaving as an active bug.
Comment #14
Mile23Hah. I remember having this conversation with @Mixologic.
Comment #16
Mile23OK, so how do you get 0 in that config?
Perhaps you found the YAML in your config directory and modified it.
Perhaps you issued a command like this:
drush cset update.settings check.interval_days 0Perhaps it's left over from a bad migration.
What matters is that we stop the bleeding for the Drupal Association.
What do we do about it?
We can add a value check to update_cron() which will make sure that a value of 0 is treated as a value of 1. We should include a log entry about this, so the log entries pile up and become visible. This might even prompt the user to uninstall update module from duplicate sites if they realize what’s happening.
We can add a value check to update_requirements(), which will show the user that they need to change the value of this config. Also we should link them to the admin page to do it.
So now the user is looking at the admin page. They are given two choices for update frequency: Daily and weekly. If they previously had it set to 0, they'll default to daily. They can just hit submit and they're done.
Still to do:
Figure out documentation for this so we can link it from the log entry and requirements page.
Comment #17
Mile23Comment #18
larowlanWe could also consider adding an update hook here to convert it from <1 to 1 but that complicates things (update path tests, minor only etc)
Comment #19
Mile23Concerns about 7+ gave me pause as well, but I didn't feel like researching the discussion that landed on 1 or 7 exclusively. :-)
I thought about switching the form to use a number field if it's not 1 or 7, but that would be a big change. Otherwise the form never validates if it's still radio buttons.
Comment #20
Mile23The Update settings form now switches to a numeric input if you have a value other than daily or weekly.
Comment #21
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.
Reading the modules and the MR there appear to be some unresolved threads in the MR so moving back to NW for that.
Comment #22
Mile23Updated IS. Still NW.
Comment #23
Mile23Addressed @larowlan's concerns: Frequency is coerced to integer, test is now a kernel test instead of a functional one.
Comment #24
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.
Removing credit from myself as I had to rebase the MR to get the tests to run now that the composer issue is resolved.
Reviewing the MR and it does appear 2 of 3 threads are resolved the one open is
But see an explanation #23 addresses that.
While the tests run question can this move forward while tagged Needs documentation?
Comment #25
smustgrave CreditAttribution: smustgrave at Mobomo commentedTests appear all green. The one failure seems to be random ckeditor javascript one.
Moving to NW for the "needs documentation" tag. If no longer needed then +1 for RTBC.
Comment #26
Mile23Updated the Update module docs: https://www.drupal.org/node/178772/revisions/view/11521523/12932366
Comment #27
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks for the follow up
That was the only blocker from my original review so this looks good to me.
Comment #28
alexpottAdded some review comments to the MR.
Comment #29
Mile23The future we want: #2164373: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs … :-)