Problem/Motivation

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.

Proposed resolution

  1. Implement a listener to the StatusCheckEvent to add a warning if there are db updates in the updated packages.
  2. Create a kernel test to prove the warning is added. see \Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase::assertStatusCheckResults

To make the listener we can start by:

  1. copying \Drupal\automatic_updates\Validator\StagedDatabaseUpdateValidator and moving it into package manager.
  2. in the copy change `getSubscribedEvents` to only subscribe to `StatusCheckEvent
  3. in the copy remove the if (!$stage instanceof CronUpdater) { this check will apply to all stages.
  4. instead of addError use addWarning because this should not indicate that the update should proceed just that the user should be warned.
  5. This is the first StatusCheckEvent subscriber that should only run its check if the update is already staged.

    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

Remaining tasks

  1. Determine if \Drupal\automatic_updates\Validator\StagedDatabaseUpdateValidator should now extend this new class. We will probably do this in a follow-up
  2. In a follow-up remove the database update specific checking in \Drupal\automatic_updates\Form\UpdateReady::buildForm and just fire the StatusCheckEvent and display the warnings
Command icon 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

narendraR created an issue. See original summary.

tedbow’s picture

Project: Project Browser » Automatic Updates
Version: 1.0.x-dev » 8.x-2.x-dev
Issue summary: View changes
tedbow’s picture

Title: Create a validator to ensure user respond to updated modules with DB updates » Create a validator to add a warning if update extensions have DB updates
Issue summary: View changes
narendrar’s picture

Assigned: Unassigned » narendrar

tedbow’s picture

Status: Active » Needs work
narendrar’s picture

Assigned: narendrar » Unassigned
Status: Needs work » Needs review
tedbow’s picture

Status: Needs review » Needs work

Thanks @narendraR

Needs work for MR comments.

I pushed 1 small change so remember to pull that down

tedbow’s picture

narendrar’s picture

^^ This test is failing in the same way on local and DrupalCI.

narendrar’s picture

Status: Needs work » Needs review

I missed adding views extension in protected function createVirtualProject and hence tests were failing.

narendrar’s picture

All review points addressed. Ready for re-review.

srishtiiee made their first commit to this issue’s fork.

srishtiiee’s picture

Status: Needs review » Reviewed & tested by the community

Tested the validator. Marking RTBC as all the feedback is addressed👍🏼

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for final review.

TravisCarden made their first commit to this issue’s fork.

traviscarden’s picture

Rebased. (Apparently it wasn't mergeable.)

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

Surely 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.

srishtiiee’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

I 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.

srishtiiee’s picture

srishtiiee’s picture

Status: Needs work » Needs review

Addressed 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.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
phenaproxima’s picture

Title: Create a validator to add a warning if update extensions have DB updates » Create a validator to add a warning if updated extensions have database updates

phenaproxima’s picture

Status: Reviewed & tested by the community » Fixed

Merged into 8.x-2.x. Thanks for great work on this!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

tedbow’s picture

Issue tags: +core-mvp