The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
#3276255: Disable unattended updates until TUF integration is complete hardcoded \Drupal\automatic_updates\CronUpdater::$disabled = TRUE until PHP-TUF integration is complete.
It re-enables it in tests only using PHP reflection … but using a KernelEvents::REQUEST subscriber, which can result in race conditions, which we discovered in #3306283-23: If cron updates are disabled display a message if status checks fail after installing Automatic Updates.
Steps to reproduce
Proposed resolution
Lets not strictly use the config setting automatic_updates.settings.cron to determine what cron is in. This will still disable cron by default and we will not create a UI for updating this until we do #3284443: Enable unattended updates
So here is what I think we need todo.
- Remove the test module
automatic_updates_test_cron - remove the
\Drupal\automatic_updates\CronUpdater::$disabledand 1 place we check this in\Drupal\automatic_updates\CronUpdater::getMode - change automatic_updates.settings.yml to default the cron setting to
disable - Remove
automatic_updates_test_cronfrom\Drupal\Tests\automatic_updates\Kernel\AutomaticUpdatesKernelTestBase::$modulesand other base tests
Instead insetUp()of these tests do$this->config('automatic_updates.settings')->set('cron', CronUpdater::SECURITY);this will have the same effect as having the current test module - search
automatic_updates_test_cronto make sure it is not referenced else where
Remaining tasks
User interface changes
API changes
Data model changes
Issue fork automatic_updates-3321256
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:
changes
Comments
Comment #2
tedbowComment #3
tedbowComment #6
wim leersIMHO we should also add a
hook_requirements()here, just like #3068275: Add status report message about JSON:API's read-only mode did. Except that when it'sdisabled, it should be aREQUIREMENT_WARNING. (Note that that doesn't have test coverage even in core.Comment #7
omkar.podey commentedI am looking at \Drupal\Tests\automatic_updates\Functional\StatusCheckTest right now and while testing testStatusChecksOnAdminPages() i am running in to the error 'You do not have any administrative items.' while trying to access 'admin/structure' .
Comment #8
omkar.podey commentedComment #9
wim leersComment #10
wim leers… if that's tricky to figure out, put a breakpoint on the place where that string exists and observe why it reached that point: which access check failed to grant access → this will tell you what kind of access needs to be granted.
Comment #11
tedbowComment #12
omkar.podey commentedComment #13
wim leersWhile pairing with @omkar.podey, we hit a particularly nasty/complicated race condition, so I asked @omkar.podey to assign this to me 👍
Comment #14
wim leersThis is extremely tricky because
\Drupal\automatic_updates\EventSubscriber\ConfigSubscriber::onConfigSave()always calls$this->statusChecker->clearStoredResults();, which means that any prior state is always wiped, both on module install and on$this->config('automatic_updates.settings')->set('cron', CronUpdater::SECURITY)->save();…… and that's only a problem because
\Drupal\Tests\automatic_updates\Kernel\StatusCheck\StatusCheckerTest::testRunOnInstall()is trying to test a scenario that will some day be the default (once #3284443: Enable unattended updates is done), but for now is forcibly disabled, so we need to forcibly re-enable it in the test, temporarily. Will sleep on it.Comment #15
wim leersGreen!
Left 5 more remarks on the MR for @omkar.podey to address.
Comment #16
omkar.podey commentedComment #17
wim leers3 remarks on the merge request that require changes: missing update path (+test), the form alter, and a nit about minimizing diff size.
Comment #18
omkar.podey commentedComment #19
wim leers6 remarks, including me realizing that #6 is not yet addressed!
Also note that this still needs a change record. You can find hundreds of examples at http://drupal.org/list-changes/drupal
Comment #20
omkar.podey commentedNot sure about the hook implementation, does the error needs to be suppressed or do we want to update assertions in
\Drupal\Tests\automatic_updates\Functional\StatusCheckTest::testStatusChecksOnStatusReportComment #21
omkar.podey commentedComment #22
wim leers4 remarks on the MR, I'm pretty sure that one of those contains the solution to the failing test! 🤓
Comment #23
omkar.podey commentedComment #24
wim leersNo more remarks! The one thing we still need here, is a change record. See the examples I linked to in #19.
Comment #25
omkar.podey commentedCreated, The change record. saved as draft for now, can someone review that too ?
Comment #26
omkar.podey commentedComment #27
wim leers🚀
Comment #28
tedbowNeeds work for MR comments
Comment #29
wim leersThere are only two comments that @omkar.podey can address (i) the language nit, ii) for adding
@todocomments to more places).The last comment I posted on the MR is something that @tedbow should clarify first as the project lead, it's not currently actionable by @omkar.podey yet.
Comment #30
omkar.podey commentedComment #31
tedbow🏓
Comment #32
wim leers🏓
Comment #33
omkar.podey commentedComment #34
omkar.podey commentedComment #35
wim leersReady now, with all of @tedbow's concerns addressed! 👍
Comment #36
tedbowNeeds work for a strong warning if cron updates are enabled. Assign to @Wim Leers because he may have an idea for a stronger or clearer message.
Comment #37
wim leersPosted a suggestion.
Comment #38
tedbowComment #39
wim leersComment #40
tedbowMerged in 8.x-2.x again. assigning to myself to merge on green
Comment #42
tedbow@Wim Leers & @omkar.podey thanks for sticking with this one!
Comment #43
tedbow@Wim Leers & @omkar.podey thanks for sticking with this one!
Comment #44
wim leersChange record published. 👍