@pwolanin raised this here #3284443-19: Enable unattended updates and @catch also mentioned something similar in #3346707: Add Alpha level Experimental Package Manager module
Basically currently this module requires the file system to be writable by the webserver. Some hosts will not support that.
But if we had a terminal command that could run cron updates then it could be set up to be triggered externally by a more privileged user for example via nix crontab.
For the cron merge request this would need to be a standalone script. We would have to determine how hard it would be to bootstrap drupal in a console command. \Drupal\Core\Command\ServerCommand::boot does appear to be doing this but I am not positive
So we should first try to see if this is possible via just a symphony console command.
If that is not possible right now and we would need some Drupal core changes we could add a drush script to the contrib module in the meantime. If we can only do this via drush we should add the functionality in sub-module to avoid adding things to the main module that would have to be removed for the core merge request.
We would likely need to extend CronUpdater for this. At the very least we would need to override \Drupal\automatic_updates\CronUpdater::triggerPostApply
Right now at the end of \Drupal\automatic_updates\CronUpdater::performUpdate we have
// Perform a subrequest to run ::postApply(), which needs to be done in a
// separate request.
// @see parent::apply()
$url = Url::fromRoute('automatic_updates.cron.post_apply', [
'stage_id' => $stage_id,
'installed_version' => $installed_version,
'target_version' => $target_version,
'key' => $this->state->get('system.cron_key'),
]);
$this->triggerPostApply($url);
It probably would make sense in this issue to change this to
$this->triggerPostApply($stage_id, $installed_version, $target_version);
So we don't assume post apply happens via a http request. Then \Drupal\automatic_updates\CronUpdater::triggerPostApply could handle creating the URL and the new CommandCronUpdater::triggerPostApply would trigger post apply in some other way.
CommandCronUpdater::triggerPostApplyWe
| Comment | File | Size | Author |
|---|---|---|---|
| #41 | 3351895-41-update-core-MR-converter-do-not-test.patch | 651 bytes | wim leers |
Show commandsStart 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 #5
wim leersClarifying that this is meant to address @pwolanin's request at #3284443-19: Enable unattended updates and @xjm's request at #3352210-15: Security review of secure signing components for package manager:
Comment #6
wim leersComment #7
dwwI’m also very keen on this part of the problem. We spent a lot of effort in Update Manager to support both writable and not-writable web server configurations. It’d be a real step backwards if all the new slickness only supported the writable case.
Comment #9
catchI can see advantages for having a separate command so it can be run more frequently than cron and is guaranteed to be run from the cli, but also doesn't drush cron also cover the basic use case?
Comment #10
tedbow@catch yes I think
drush cronwould probably be fine for the basic use case. See @phenaproxima's question on #3284443-22: Enable unattended updates to see if just documenting thatdrush croncould be used used to unblock that issue for the the contrib case.For the core version of Automatic Updates I would think that would be product decision as to whether we could still rely on drush for this use case or if we would have to have a solution that relied on core only.
I think if we did a need solution for the core version that relied on core alone there would be 2 options.
drush crondoes for contribI am not sure if 2) would give us extra advantages beyond AutoUpdates. Presumably some people are relying on
drush cronthat could just rely on core for this.Also if we did 2) it might be possible as follow-up to set an option flag to only run particular module. For instance
core/scripts/drupal cron --module=automatic_updates. This would allow running Automatic Updates more frequently that other modules' cron jobsComment #11
wim leersFirst round of review posted. I have some questions 😊
Comment #12
tedbowThis is failing for me with manual testing. The composer update appear to work but I get "Automatic updates failed to apply, and the site is in an indeterminate state. Consider restoring the code and database from a backup."
Also the UX is not great because I guessing there is message from Composer STager I am not seeing. I need to check if that is case from the UI too.
Comment #13
wim leersGiven this, should we block this on #3354701: Ensure exceptions thrown by event subscribers are logged, which should be trivial to do and would make the testing here simpler?
Comment #14
tedbowre #13
In this case the problem is not coming from any event subscribers. I created #3355074: Failure marker message does not contain exception message + backtrace if available
Comment #15
wim leersYou linked to this issue 😅 Which did you mean to link to?
Comment #16
tedbow🫢Whoops, fixed
Comment #17
tedbow🤦🏼
Comment #18
tedbowSo I manually tested this with the changes in #3355074: Failure marker message does not contain exception message + backtrace if available and the message change to a much more meaningful
The actual problem here of trying to copy the .git files I think is an existing issue but I will search the issue queue.
I will probably manually test by just deleting the .git directory first as I am pretty sure this error has nothing to with it being a drush command. Usually when I manually test I am not using a Composer
vcsrepo. So need to confirm but I think this would happen via the Update form also.Comment #19
wim leers#18: wonderful news!
Comment #20
wim leersClarifying that this is blocked on #3355074: Failure marker message does not contain exception message + backtrace if available.
Comment #21
tedbowI don't think we need to block on #3355074: Failure marker message does not contain exception message + backtrace if available. It is an existing issue I just happen to find in this issue it would happen in any case where you had the marker file
Comment #22
tedbowComment #23
tedbow@phenaproxima was going to change the drush command to have only 1 command and have a
--post-apply=[STAGE_KEY]option.Comment #24
tedbowI manually tested and it updated from 10.0.8 to 10.0.9. I actually had to manually change
StagedDatabaseUpdateValidatorbecause it flagged a change to system_requirments has a DB update when it was not. But that is known issue and comment on #3253828: Use static analysis to detect new update functions, to reduce false positives in StagedDBUpdateValidator with not about this caseI think we still don't want to run DB updates in this drush command as MVP because we intended it to be run in server crontab in an unattended way. But we may want a follow-up that would allowing running DB updates.
I will manually test again once @phenaproxima has made further changes
Comment #26
wim leers@tedbow in #21: sure — but your comment in #18 made it sound like it was a hard blocker to continue here. So I was just trying to be helpful 😅🙈
@tedbow in #24: EXCITING!!!!!!! 😄
Also, this MR is starting to look great!
One thing I'm missing is documentation that says you MUST run two commands:AFAICT the post-apply gets called automatically thanks todrush auto-updatefollowed bydrush auto-update --post-apply=….ConsoleCronUpdateStage::handleCron()calling\Drupal\automatic_updates\CronUpdateStage::handleCron()which calls\Drupal\automatic_updates\CronUpdateStage::performUpdate()which calls$this->triggerPostApply()all the way at the end? 🤔 What am I missing?Comment #27
wim leersComment #28
phenaproximaComment #29
tedbowLooking good! Just a few suggestions
Comment #30
phenaproximaComment #31
tedbowNeeds follow-upfor timeout issue in the MR discussionsComment #32
tedbowI think this issue is good. We already have #3357969: For web server dependent unattended updates run the entire life cycle in a separate process that will not be affected by hosting time limits which researching the different places the code can time out.
I am going to manually test this again
Comment #33
tedbowManual testing didn't work
Comment #34
tedbowGot a different error manually testing
guessing
$command[0] = $this->fileSystem->realpath($command[0]);didn't work. Maybe `$command[0]` just is "drush"?
Maybe we need the standard drush way for firing off another command. I think maybe
Drush::drush(Drush::aliasManager()->getSelf(), $command, $args);Comment #35
phenaproximaI agree, but that means we should probably pass the fully built-out Process object to
Stage::triggerPostApply(), which I'm not entirely sure how to do...Comment #36
phenaproximaComment #37
tedbowI manually tested if from /web/sites/default and it worked update it but the only messages I saw was
So it seems like the
iois still not coming back from post apply.I have an idea on this
Comment #38
tedbowLooks good to me if tests pass!
Comment #40
phenaproxima🚢
Comment #41
wim leers🥳
I'm pleasantly surprised this landed yesterday! 🤩
But … it seems a bunch of threads were either left open or closed without explanation? 🙈 That makes it difficult for me (or anybody else) to understand the decision making process…
I've noticed that at least one of the remarks in the threads which did not get resolved/explained did start being addressed in another issue: #3357969: For web server dependent unattended updates run the entire life cycle in a separate process that will not be affected by hosting time limits. Hence linking that.
\Drupal\automatic_updates\Development\Converterto avoid addingdrush.services.ymlsrc/Commands/AutomaticUpdatesCommands.phpsrc/DrushUpdateStage.phpto the core MR. Feels like that's easiest to do here? Patch attached.
Comment #43
tedbowadded #3359727: Add new setting for how unattended updates will be run as remaining task
Comment #44
wim leersI identified 3 follow-ups when I reopened this in #41:
Or am I just overlooking an already existing issue for making this work as a Symfony console command? 🤔
Comment #45
tedbowAdded #3360485: Add Symfony Console command to allow running cron updates via console and by a separate user, for defense-in-depth
Comment #46
wim leersThanks!
Comment #47
wim leers