Closed (fixed)
Project:
Automatic Updates
Version:
8.x-2.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
2 Sep 2022 at 16:07 UTC
Updated:
15 Dec 2022 at 20:33 UTC
Jump to comment: Most recent
For Project Browser #3300061-10: [policy, no patch] Determine Composer validation rules for installing through Composer
it was determined that the user should be made aware that there are database updates in updated packages and consent is required to proceed further.
StatusCheckEvent to add a warning if there are db updates in the updated packages.\Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase::assertStatusCheckResultsTo make the listener we can start by:
\Drupal\automatic_updates\Validator\StagedDatabaseUpdateValidator and moving it into package manager. StatusCheckEventif (!$stage instanceof CronUpdater) { this check will apply to all stages.addError use addWarning because this should not indicate that the update should proceed just that the user should be warned.I think how to check that would be use \Drupal\package_manager\Stage::getStageDirectory
so maybe something like
if (!file_exists($event->getStage()->getStageDirectory())) {
// No staged updates exist, therefore we don't need to run this check.
return;
}
for the test you can also start by copying \Drupal\Tests\automatic_updates\Kernel\ReadinessValidation\StagedDatabaseUpdateValidatorTest
\Drupal\automatic_updates\Validator\StagedDatabaseUpdateValidator should now extend this new class. We will probably do this in a follow-up\Drupal\automatic_updates\Form\UpdateReady::buildForm and just fire the StatusCheckEvent and display the warningsStart 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 #3
tedbowComment #4
narendrarComment #6
tedbowComment #7
narendrarComment #8
tedbowThanks @narendraR
Needs work for MR comments.
I pushed 1 small change so remember to pull that down
Comment #9
tedbowcreate follow-up #3308861: Consolidate for StagedDatabaseUpdateValidator validators and tests
Comment #10
narendrar^^ This test is failing in the same way on local and DrupalCI.
Comment #11
narendrarI missed adding
viewsextension inprotected function createVirtualProjectand hence tests were failing.Comment #12
narendrarAll review points addressed. Ready for re-review.
Comment #14
srishtiiee commentedTested the validator. Marking RTBC as all the feedback is addressed👍🏼
Comment #15
phenaproximaSelf-assigning for final review.
Comment #17
traviscarden commentedRebased. (Apparently it wasn't mergeable.)
Comment #18
phenaproximaSurely we should be removing the version of this validator that exists in Automatic Updates?
I do see that this one is meant to run on StatusCheckEvent, and the AU one is meant to run on pre-apply for cron updates only. But if that's the case, then we should hollow out the AU version and make it a decorator that invokes the Package Manager validator on pre-apply.
Comment #19
srishtiiee commentedComment #20
phenaproximaI think this is making a lot of sense.
I believe we can probably remove a few somewhat out-of-scope changes, and maybe not add new fixtures if we can avoid it. Otherwise I don't think I have any major objections to this.
Comment #21
srishtiiee commentedThe tests should pass once #3312085: UpdateReady hides the Continue button if StatusCheckEvent only returns warnings is fixed.
Comment #22
srishtiiee commentedAddressed the feedback. I can't resolve threads on this MR as it wasn't opened by me but all the changes are done and needs review.
Comment #23
phenaproximaComment #24
phenaproximaComment #26
phenaproximaMerged into 8.x-2.x. Thanks for great work on this!
Comment #28
tedbow