Problem/Motivation

Noted in #3256642: Introduce database driver extensions and autoload database drivers' dependencies, https://git.drupalcode.org/project/drupal/-/merge_requests/3169#note_182014.

The InstallerNonDefaultDatabaseDriverTest asserts on actual strings of the settings.php file, which is brittle.

Steps to reproduce

Proposed resolution

Refactor to assert on arrays instead (from connection info?)

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3366862

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.

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

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3433034: Refactor InstallerNonDefaultDatabaseDriverTest to avoid DatabaseDriverUninstallValidator re-fetch the database connection
mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Refactor InstallerNonDefaultDatabaseDriverTest to avoid asserting content of settings.php » [PP-1] Refactor InstallerNonDefaultDatabaseDriverTest to avoid asserting content of settings.php
Status: Active » Postponed

mondrake opened merge request !11814

mondrake’s picture

mondrake
Location 🇮🇹
commented

From https://www.php.net/manual/en/function.include.php

When a file is included, the code it contains inherits the variable scope of the line on which the include occurs. Any variables available at that line in the calling file will be available within the called file, from that point forward. However, all functions and classes defined in the included file have the global scope.

So in this case, $databases is a local variable of InstallerNonDefaultDatabaseDriverTest::getInstalledDatabaseSettings() coming from the evaluation of the code in the included settings.php file.

daffie’s picture

daffie commented

When a file is included, the code it contains inherits the variable scope of the line on which the include occurs. Any variables available at that line in the calling file will be available within the called file, from that point forward. However, all functions and classes defined in the included file have the global scope.

Now I get it. Thanks for the explanation. Could you remove the assert(isset($databases)); from the PR and add a comment with your explanation? After that it is RTBC for me.

mondrake’s picture

mondrake
Location 🇮🇹
commented

I had to add the assert since PHPStan was complaining more or less for the same reason of your comment - PHPStan cannot determine what's included in the include (pun intended), so we need to inform it that a variable with that name is existing at that point.

Once the parent is committed I will rebase and add a comment.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: [PP-1] Refactor InstallerNonDefaultDatabaseDriverTest to avoid asserting content of settings.php » Refactor InstallerNonDefaultDatabaseDriverTest to avoid asserting content of settings.php
Status: Postponed » Needs review

Done.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

All code change look good to me.
The test has been improved.
For me it is RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 11.1.x-dev
Status: Reviewed & tested by the community » Fixed

Backported to 11.1.x as test-only change.

Committed and pushed c8a7a5894c9 to 11.x and 5f901886979 to 11.2.x and bc806750d89 to 11.1.x. Thanks!

  • alexpott committed bc806750 on 11.1.x 
    Issue #3366862 by mondrake, daffie: Refactor...

  • alexpott committed 5f901886 on 11.2.x 
    Issue #3366862 by mondrake, daffie: Refactor...

  • alexpott committed c8a7a589 on 11.x 
    Issue #3366862 by mondrake, daffie: Refactor...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Status: Fixed » Closed (fixed)

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

mondrake closed merge request !11814