Problem/Motivation

In order to allow InstallerNonDefaultDatabaseDriverTest execute, DatabaseDriverUninstallValidator::validate() needs to get a new connection set by the test than the one passed in construction.

We should not have to alter runtime execution paths to fulfill test requirements.

Proposed resolution

Refactor InstallerNonDefaultDatabaseDriverTest so that the injected connection be used.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3433034

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:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake opened merge request !11805

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs work
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
Status: Needs work » Needs review

IMO the test should have not been using the test container, that remains linked to the db driver passed in to the test. Since the essence of the test is to diverge from it in the SUT, let’s check the SUT’s GUI for the modules statuses.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left a comment in the MR.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Small comment then good to RTBC I think

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Ah right.

Then rest seems good

  • catch committed e3894f6a on 11.x 
    Issue #3433034 by mondrake, smustgrave: Refactor...
mondrake’s picture

mondrake
Location 🇮🇹
commented

This should be marked 'fixed' now, right?

mondrake closed merge request !11805

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Yes it should, looks like I lost track of the tab...

Committed/pushed to 11.x, thanks!

Status: Fixed » Closed (fixed)

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